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

[ci...@chromium.org]

Please take a look.

https://codereview.appspot.com/6591072/

[jung...@google.com]

Will you add a pointer to v8 bug 459 to the CL description?



https://codereview.appspot.com/6591072/

[jung...@google.com]

With a reference to V8 bug 459 added to the description, LGTM


https://codereview.appspot.com/6591072/

[jung...@google.com]

On 2012/10/03 23:01:48, jungshik wrote:
> With a reference to V8 bug 459 added to the description, LGTM

BTW, you can make the comparison page at
http://i18n.kaziprst.org/comparison.html
more 'dramatic' by showing what toLocaleXXX does in a non-English locale
(before and after).


https://codereview.appspot.com/6591072/

[a...@chromium.org]

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 enumerability.

Use Object.defineProperty instead

https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode24
src/overrides.js:24: String.prototype.localeCompare = function(that,
locales, options) {
This no longer reports the function as native.

assertEquals(String(String.prototype.toLocalString), 'function
toLocaleString() { [native code] }')

https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode26
src/overrides.js:26: return new Intl.Collator(locales,
options).compare(this, that);
How do we ensure that we are using the original Intl and Intl.Collator?

https://codereview.appspot.com/6591072/

[ci...@chromium.org]

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
> src/overrides.js:24: String.prototype.localeCompare = function(that,
locales,
> options) {
> This changes the enumerability.

> Use Object.defineProperty instead

I can do that, but Object.defineProperty is slow in v8.

https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode24
> src/overrides.js:24: String.prototype.localeCompare = function(that,
locales,
> options) {
> This no longer reports the function as native.

> assertEquals(String(String.prototype.toLocalString), 'function
toLocaleString()
> { [native code] }')

Is that breaking the spec or just v8 test expectation? We can fix v8
test if latter.
I could make it native, but it would complicate code needlessly.

https://codereview.appspot.com/6591072/diff/1008/src/overrides.js#newcode26
> src/overrides.js:26: return new Intl.Collator(locales,
options).compare(this,
> that);
> How do we ensure that we are using the original Intl and
Intl.Collator?

I am not. I'll save references to methods I use on startup (hijacking
that would be hard) and then use those references in the future.



https://codereview.appspot.com/6591072/

[ci...@chromium.org]

On 2012/10/04 23:47:31, cira1 wrote:
> Save a reference to Intl.Constructor for security.

Erik, please take another look.

1. Fixed enumerable issue - I hope we don't regress too much on startup
speed because of Object.defineProperty.

2. Saved reference to the constructors and added test for it.

3. I couldn't save reference to the XXX.prototype.(compare|format).
Those are getters and I couldn't find a way to get to it without
executing it, which then fails.

4. I don't think I can do much about functions not being native issue at
this point.


https://codereview.appspot.com/6591072/

[a...@chromium.org]

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 fix indentation


Object.defineProperty(String.prototype, 'localeCompare', {
value: function(that, locales, options) {
// Call internal method.
return compare(new collator(locales, options), this, that);
},
writable: true,
configurable: true,
enumerable: false
});

https://codereview.appspot.com/6591072/diff/22001/tests/intl/overrides/security.js
File tests/intl/overrides/security.js (right):

https://codereview.appspot.com/6591072/diff/22001/tests/intl/overrides/security.js#newcode28
tests/intl/overrides/security.js:28: var throws = false;
Isn't there an assertThrows?

https://codereview.appspot.com/6591072/

[Erik Arvidsson]

On Thu, Oct 4, 2012 at 7:52 PM, <ci...@chromium.org> wrote:
> 3. I couldn't save reference to the XXX.prototype.(compare|format).
> Those are getters and I couldn't find a way to get to it without
> executing it, which then fails.

You should be able to do Object.getOwnPropertyDescriptor to get the
getter/setter functions.

--
erik

[Nebojša Ćirić]

I've missed that one - I always thought of it returning only info about 'writable, configurable and enumerable' forgetting about the 'value' part...

Thanks for the pointer.

2012/10/8 Erik Arvidsson <a...@chromium.org>

--
Nebojša Ćirić