Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(804)

Issue 6591072: Implementing String, Number and Date toLocaleXXXString overrides as per ECMA 402, Chapter 13. Small… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by cira1
Modified:
1 year, 5 months ago
Reviewers:
jungshik, arv
CC:
v8-dev_googlegroups.com
Base URL:
http://v8-i18n.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Implementing String, Number and Date toLocaleString, localeCompare overrides as
per ECMA 402, Chapter 13 - see
http://wiki.ecmascript.org/doku.php?id=globalization:specification_drafts for
details.

Small bug fix in date-format.js.

To see the changes that would happen for users, take a look at:
http://i18n.kaziprst.org/comparison.html

BUG=v8-i18n:29, v8:459

Patch Set 1 : Simplify Number.prototype.toLocaleString #

Total comments: 3

Patch Set 2 : Save a reference to Intl.Constructor for security. #

Patch Set 3 : Calls internal methods instead of the prototype methods - for security. #

Total comments: 2

Patch Set 4 : Using assertThrow, assertDoesNotThrow. Added both util methods to assert.js file in tests/. #

Patch Set 5 : Fixed indentation in overrides.js. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+347 lines, -3 lines) Patch
M build/all.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M src/date-format.js View 2 chunks +3 lines, -3 lines 0 comments Download
A src/overrides.js View 1 2 3 4 1 chunk +116 lines, -0 lines 0 comments Download
M tests/assert.js View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
A tests/intl/overrides/date.js View 1 chunk +52 lines, -0 lines 0 comments Download
A tests/intl/overrides/number.js View 1 chunk +40 lines, -0 lines 0 comments Download
A tests/intl/overrides/security.js View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A tests/intl/overrides/string.js View 1 chunk +56 lines, -0 lines 0 comments Download

Messages

Total messages: 15
cira1
Simplify localeCompare.
1 year, 6 months ago #1
cira1
Small changes
1 year, 6 months ago #2
cira1
Please take a look.
1 year, 6 months ago #3
cira1
Simplify Number.prototype.toLocaleString
1 year, 6 months ago #4
jungshik
Will you add a pointer to v8 bug 459 to the CL description?
1 year, 6 months ago #5
jungshik
With a reference to V8 bug 459 added to the description, LGTM
1 year, 6 months ago #6
jungshik
On 2012/10/03 23:01:48, jungshik wrote: > With a reference to V8 bug 459 added to ...
1 year, 6 months ago #7
arv
https://codereview.appspot.com/6591072/diff/1008/src/overrides.js File src/overrides.js (right): https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode24 src/overrides.js:24: String.prototype.localeCompare = function(that, locales, options) { This changes the ...
1 year, 6 months ago #8
cira1
On 2012/10/03 23:57:34, arv-chromium wrote: > https://codereview.appspot.com/6591072/diff/1008/src/overrides.js > File src/overrides.js (right): > > https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode24 > ...
1 year, 6 months ago #9
cira1
Save a reference to Intl.Constructor for security.
1 year, 6 months ago #10
cira1
On 2012/10/04 23:47:31, cira1 wrote: > Save a reference to Intl.Constructor for security. Erik, please ...
1 year, 6 months ago #11
cira1
Calls internal methods instead of the prototype methods - for security.
1 year, 6 months ago #12
arv
LGTM with nits https://codereview.appspot.com/6591072/diff/22001/src/overrides.js File src/overrides.js (right): https://codereview.appspot.com/6591072/diff/22001/src/overrides.js#newcode32 src/overrides.js:32: {value: function(that, locales, options) { Please ...
1 year, 6 months ago #13
cira1
Using assertThrow, assertDoesNotThrow. Added both util methods to assert.js file in tests/.
1 year, 6 months ago #14
cira1
1 year, 6 months ago #15
Fixed indentation in overrides.js.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5