Bug 168178 – [INTL] Change Intl prototypes to plain objects

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

Change Intl prototypes to plain objects

[Andy VanWagoner]

Created attachment 301274 [details]
Patch

[Andy VanWagoner]

https://github.com/tc39/ecma402/issues/122

[Build Bot]

Comment on attachment 301274 [details]
Patch

Attachment 301274 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3148910

New failing tests:
cdjs-tests.yaml/main.js.ftl-no-cjit
cdjs-tests.yaml/main.js.ftl-eager-no-cjit

[Yusuke Suzuki]

Comment on attachment 301274 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301274&action=review

r=me

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Could you add tests to ensure that the prototypes become plain objects correctly?

[Andy VanWagoner]

(In reply to comment #4)
> Comment on attachment 301274 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301274&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Could you add tests to ensure that the prototypes become plain objects
> correctly?

Do you have any suggestions on how to do this? The following are true before this change.

shouldBe("Object.getPrototypeOf(Intl.Collator.prototype)", "Object.prototype");
shouldBe("Object.prototype.toString.call(Intl.Collator.prototype)", "'[object Object]'");

There are already tests in this patch that throw when trying to use the prototype object with compare, format, and resolvedOptions.

[Andy VanWagoner]

(In reply to comment #3)
> Comment on attachment 301274 [details]
> Patch
> 
> Attachment 301274 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3148910
> 
> New failing tests:
> cdjs-tests.yaml/main.js.ftl-no-cjit
> cdjs-tests.yaml/main.js.ftl-eager-no-cjit

How do I go about finding the specific cause of the performance regression? My gut feeling is the removal of the invalidated optimization in String#localeCompare.

[Andy VanWagoner]

Created attachment 302762 [details]
Patch

[Andy VanWagoner]

Comment on attachment 302762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302762&action=review

> Source/JavaScriptCore/builtins/StringPrototype.js:240
> +    

Normally I'd just have a simple variable, but I'm not confident I can construct a Collator yet, and I've only ever seen functions at the top level of a builtins file.

  var defaultCollator = new @Collator();

> LayoutTests/js/script-tests/intl-collator.js:213
> +shouldBe("Object.prototype.toString.call(Intl.Collator.prototype)", "'[object Object]'");

Here's the check to observe the new behavior. Only the constructor property would have failed before this patch, though.

> LayoutTests/js/script-tests/intl-collator.js:235
> +shouldThrow("Intl.Collator.prototype.compare", "'TypeError: Intl.Collator.prototype.compare called on value that\\'s not an object initialized as a Collator'");

Here is the real change in behavior. Trying to get the bound compare function, or resolvedOptions on the prototype directly will throw now.

> LayoutTests/js/script-tests/intl-numberformat.js:283
> +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'");

I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character.

[JF Bastien]

Comment on attachment 302762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302762&action=review

r=me

>> LayoutTests/js/script-tests/intl-numberformat.js:283
>> +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'");
> 
> I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character.

One might say your editor broke the non-breaking spaces.

[Andy VanWagoner]

(In reply to JF Bastien from comment #9)
> Comment on attachment 302762 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302762&action=review
> 
> r=me
> 
> >> LayoutTests/js/script-tests/intl-numberformat.js:283
> >> +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'");
> > 
> > I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character.
> 
> One might say your editor broke the non-breaking spaces.

:)

I'm not a committer, so if this is good to go, can you add the commit-queue+ flag?

[JF Bastien]

Comment on attachment 302762 [details]
Patch

cq+

[WebKit Commit Bot]

Comment on attachment 302762 [details]
Patch

Clearing flags on attachment: 302762

Committed r215349: <http://trac.webkit.org/changeset/215349>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.