Last Comment Bug 769871 - Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString per ECMA-402
: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString ...
Status: RESOLVED FIXED
[js:t][DocArea=JS]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla27
Assigned To: Norbert Lindenberg
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 724533 847000
Blocks: 724529 es-intl 1215247 853301 864843 866301 866305 1215252
  Show dependency treegraph
 
Reported: 2012-06-29 18:09 PDT by Norbert Lindenberg
Modified: 2015-10-15 12:07 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP) (33.20 KB, patch)
2012-09-11 23:40 PDT, Norbert Lindenberg
no flags Details | Diff | Splinter Review
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP) (35.46 KB, patch)
2012-09-30 19:06 PDT, Norbert Lindenberg
no flags Details | Diff | Splinter Review
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP) (35.90 KB, patch)
2012-10-21 22:37 PDT, Norbert Lindenberg
no flags Details | Diff | Splinter Review
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP) (35.98 KB, patch)
2012-11-25 22:56 PST, Norbert Lindenberg
no flags Details | Diff | Splinter Review
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP) (31.10 KB, patch)
2013-01-09 12:19 PST, Norbert Lindenberg
no flags Details | Diff | Splinter Review
Reimplement String.localeCompare per ECMA-402. (13.33 KB, patch)
2013-03-19 10:37 PDT, Norbert Lindenberg
jwalden+bmo: review+
Details | Diff | Splinter Review
Reimplement Number.toLocaleString per ECMA-402. (16.36 KB, patch)
2013-03-19 10:38 PDT, Norbert Lindenberg
jwalden+bmo: review+
Details | Diff | Splinter Review
Reimplement Date.toLocaleString per ECMA-402. (23.17 KB, patch)
2013-03-19 10:39 PDT, Norbert Lindenberg
jwalden+bmo: review+
Details | Diff | Splinter Review
Reimplement Number.toLocaleString per ECMA-402. (16.40 KB, patch)
2013-03-20 01:08 PDT, Norbert Lindenberg
mozillabugs: review+
Details | Diff | Splinter Review
Reimplement Date.toLocaleString per ECMA-402. (23.21 KB, patch)
2013-03-20 01:09 PDT, Norbert Lindenberg
mozillabugs: review+
Details | Diff | Splinter Review
Remove x_CheckThisX methods, as explained before (5.58 KB, patch)
2013-03-21 18:55 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review+
Details | Diff | Splinter Review

Description Norbert Lindenberg 2012-06-29 18:09:44 PDT
This is one step towards a full implementation of the ECMAScript Internationalization API: Reimplement the three existing locale sensitive functions that the ECMAScript Internationalization spec redefines directly on top of ICU, using the default locale and default options per spec.
Comment 1 Norbert Lindenberg 2012-09-11 23:23:39 PDT
The current implementations of these functions use different mechanisms to let software outside of SpiderMonkey influence their behavior, and we have to decide whether any of this has to be maintained in the future.

- For String.prototype.localeCompare, SpiderMonkey by default uses the dumb Unicode code unit comparison specified in ES5 11.8.5 (which is neither locale sensitive nor conforms to the specification of localeCompare in 15.5.4.9). Client software can provide better implementations via JS_SetLocaleCallbacks, and XPConnect does so to install implementations from the intl/ directory in the Mozilla tree. The implementations I've tested on Mac and Windows do not conform to ES5 15.5.4.9 either (they don't implement canonical equivalence).

- For Number.prototype.toLocaleString, SpiderMonkey has its own implementation, but obtains localized decimal separator, thousands separator, and grouping information from either the operating system or from environment variables. The latter mechanism is used by the Android GeckoAppShell.java classes. 

- For Date.prototype.toLocaleString, SpiderMonkey relies on the operating system's strftime function, and makes formatting based on strftime patterns available to applications through a non-standard method toLocaleFormat.


Proposal:

- Ignore localeCompare implementations provided by SpiderMonkey clients. No such implementation could conform to the ECMAScript Internationalization API specification because it doesn't have access to the new locales and options arguments. Such implementations will also no longer be necessary once SpiderMonkey relies on ICU to implement localeCompare. The field in the JSLocaleCallbacks struct remains in order to provide minimal binary compatibility with existing clients.

- Ignore decimal separator, thousands separator, and grouping information provided via environment variables. While such information could theoretically be merged into ICU output, it won't improve the results because the provider doesn't have access to the locales argument and ICU has all necessary information.

- Maintain Date.prototype.toLocaleFormat with its current functionality for compatibility with existing applications. However, this function will not benefit from the improvements provided by the ECMAScript Internationalization API (such as control over the language used). Date.prototype.toLocaleString will rely on ICU rather than strftime.

It may make sense to give SpiderMonkey containers a way to control the default locale used by locale sensitive functions; this is currently being discussed in bug 769872.
Comment 2 Norbert Lindenberg 2012-09-11 23:40:47 PDT
Created attachment 660330 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

First functional version. Doesn't cache any ICU objects yet, which it likely will have to do to achieve reasonable performance. Uses file names intl.h and intl.cpp to avoid collisions with the jsintl.h and jsintl.cpp being added via bug 769872 - in the end, code will be merged into jsintl. Tested only in js shell.
Comment 3 Norbert Lindenberg 2012-09-30 19:06:15 PDT
Created attachment 666413 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Comment 4 Norbert Lindenberg 2012-10-21 22:37:27 PDT
Created attachment 673765 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Comment 5 Norbert Lindenberg 2012-11-25 22:56:02 PST
Created attachment 685049 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Comment 6 Norbert Lindenberg 2013-01-09 12:19:08 PST
Created attachment 699997 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Comment 7 Norbert Lindenberg 2013-03-19 10:36:17 PDT
Comment on attachment 699997 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

The upcoming attachments use a different design from the previous ones: They implement the locale sensitive functions in String, Number, and Date as self-hosted JavaScript on top of Intl.Collator, Intl.NumberFormat, and Intl.DateTimeFormat.
Comment 8 Norbert Lindenberg 2013-03-19 10:37:32 PDT
Created attachment 726759 [details] [diff] [review]
Reimplement String.localeCompare per ECMA-402.
Comment 9 Norbert Lindenberg 2013-03-19 10:38:25 PDT
Created attachment 726761 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.
Comment 10 Norbert Lindenberg 2013-03-19 10:39:13 PDT
Created attachment 726765 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-19 15:47:11 PDT
Comment on attachment 726761 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

Review of attachment 726761 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.cpp
@@ +448,5 @@
> +JS_ALWAYS_INLINE bool
> +num_nop(JSContext *cx, CallArgs args)
> +{
> +    JS_ASSERT(IsNumber(args.thisv()));
> +    return true;

It'll eventually be an error to return successfully from a JSNative without setting a return value, so add this, please:

args.rval().setUndefined();

@@ +457,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    // CallNonGenericMethod will handle proxies correctly and throw exceptions
> +    // in the right circumstances, but will report date_CheckThisDate as the

I imagine this date*Date bit is copypasta, please fix.  :-)

@@ +916,5 @@
>      JS_FN(js_valueOf_str,        js_num_valueOf,        0, 0),
>      JS_FN("toFixed",             num_toFixed,           1, 0),
>      JS_FN("toExponential",       num_toExponential,     1, 0),
>      JS_FN("toPrecision",         num_toPrecision,       1, 0),
> +// ??? must be last - functions listed after it aren't available in self-hosted code

Given the mixing we have in jsarray.cpp array_methods, I don't think this is necessary.  What's the behavior you see that causes you to think it is?

::: js/src/jsnum.h
@@ +152,5 @@
> +/**
> + * Checks that the this value provided meets the requirements for
> + * "this Number object" in ES5.1, 15.7.4, and throws a TypeError if not.
> + *
> + * Usage: callFunction(date_CheckThisNumber, this)

This could just be date_CheckThisNumber(this), right?  Don't see how callFunction is necessary for this case.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-19 16:02:22 PDT
Comment on attachment 726765 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

Review of attachment 726765 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Date.js
@@ +16,5 @@
> + * Spec: ECMAScript Internationalization API Specification, 13.3.1.
> + */
> +function Date_toLocaleString() {
> +    // Steps 1-2.
> +    callFunction(date_CheckThisDate, this);

date_checkThisDate(this);

Make the same change to the previous patches, in the self-hosted methods, where I previously only noted this for the header comment.  And to all the methods here, not just this one.

::: js/src/jsdate.cpp
@@ +1395,5 @@
> +JS_ALWAYS_INLINE bool
> +date_nop(JSContext *cx, CallArgs args)
> +{
> +    JS_ASSERT(IsDate(args.thisv()));
> +    return true;

More args.rval().setUndefined();.

@@ +3059,5 @@
>      JS_FN(js_toSource_str,       date_toSource,           0,0),
>  #endif
>      JS_FN(js_toString_str,       date_toString,           0,0),
>      JS_FN(js_valueOf_str,        date_valueOf,            0,0),
> +// ??? must be last - functions listed after it aren't available in self-hosted code

Still confused about this.

::: js/src/jsdate.h
@@ +68,5 @@
> +/**
> + * Checks that the this value provided meets the requirements for
> + * "this Date object" in ES5.1, 15.9.5, and throws a TypeError if not.
> + *
> + * Usage: callFunction(date_CheckThisDate, this)

Same date_CheckThisDate(this) again.
Comment 13 Norbert Lindenberg 2013-03-20 01:08:03 PDT
Created attachment 727091 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

Updated per comment 11. Carrying r+jwalden.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)

> @@ +916,5 @@
> >      JS_FN(js_valueOf_str,        js_num_valueOf,        0, 0),
> >      JS_FN("toFixed",             num_toFixed,           1, 0),
> >      JS_FN("toExponential",       num_toExponential,     1, 0),
> >      JS_FN("toPrecision",         num_toPrecision,       1, 0),
> > +// ??? must be last - functions listed after it aren't available in self-hosted code
> 
> Given the mixing we have in jsarray.cpp array_methods, I don't think this is
> necessary.  What's the behavior you see that causes you to think it is?

For String, my self-hosted code couldn't find split anymore - it looked like the function didn't exist. For Number, I don't remember exactly, but it may have been valueOf where it'd find only the one in Object.prototype. I'll file a ticket when this code has landed and is more available for testing.

The only Array methods that show up after the self-hosted entries in the method table are filter and iterator, neither of which are used in self-hosted code.

> > + * Usage: callFunction(date_CheckThisNumber, this)
> 
> This could just be date_CheckThisNumber(this), right?  Don't see how
> callFunction is necessary for this case.

date_CheckThisNumber(this) would be equivalent to callFunction(date_CheckThisNumber, undefined, this). CallNonGenericMethod expects the this value as args.thisv().
Comment 14 Norbert Lindenberg 2013-03-20 01:09:43 PDT
Created attachment 727093 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

Updated per comment 12. Carrying r+jwalden.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)

Same replies to same comments as above.
Comment 15 Norbert Lindenberg 2013-03-20 11:47:02 PDT
(In reply to Norbert Lindenberg from comment #13)

> > > +// ??? must be last - functions listed after it aren't available in self-hosted code
> > 
> > Given the mixing we have in jsarray.cpp array_methods, I don't think this is
> > necessary.  What's the behavior you see that causes you to think it is?
> 
> For String, my self-hosted code couldn't find split anymore - it looked like
> the function didn't exist. For Number, I don't remember exactly, but it may
> have been valueOf where it'd find only the one in Object.prototype. I'll
> file a ticket when this code has landed and is more available for testing.
> 
> The only Array methods that show up after the self-hosted entries in the
> method table are filter and iterator, neither of which are used in
> self-hosted code.

My previous comment provided me with the idea for a simple test case, so there's now bug 853075.
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 18:40:59 PDT
Comment on attachment 726759 [details] [diff] [review]
Reimplement String.localeCompare per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71b015ae13c3
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 18:41:15 PDT
Comment on attachment 727091 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bebe0562ab3
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 18:41:32 PDT
Comment on attachment 727093 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d550a01e0ab2
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 18:44:45 PDT
(In reply to Norbert Lindenberg from comment #13)
> > > + * Usage: callFunction(date_CheckThisNumber, this)
> > 
> > This could just be date_CheckThisNumber(this), right?  Don't see how
> > callFunction is necessary for this case.
> 
> date_CheckThisNumber(this) would be equivalent to
> callFunction(date_CheckThisNumber, undefined, this). CallNonGenericMethod
> expects the this value as args.thisv().

Oh, right, I'm dumb.

Looking at this closer, it happens we get this non-generic test performed when we call Date.prototype.valueOf, actually.  So technically the helper, and the call to it, are unnecessary.  It was very tempting to just remove it...but probably that's enough change to want an actual patch and review.  I'll do that and post them here for you to test out, in a sec.

(In reply to Norbert Lindenberg from comment #15)
> My previous comment provided me with the idea for a simple test case, so
> there's now bug 853075.

Excellent!  Thank you for this.  I tweaked the comments on these to mention this bug, too.
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 18:55:43 PDT
Created attachment 728003 [details] [diff] [review]
Remove x_CheckThisX methods, as explained before
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-21 19:19:57 PDT
Also pretend I removed the methods' additions from SelfHosting.cpp, turns out that's necessary to compile.  :-)
Comment 23 Norbert Lindenberg 2013-03-22 11:45:58 PDT
Comment on attachment 728003 [details] [diff] [review]
Remove x_CheckThisX methods, as explained before

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Created attachment 728003 [details] [diff] [review]
> Remove x_CheckThisX methods, as explained before

Most excellent! After pretending of course that the entries are removed from SelfHosting.cpp and those picky compilers are happy :-)

Related Test262 tests all pass.
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-22 12:59:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/195a2038433d
Comment 25 Joe Drew (not getting mail) 2013-03-23 16:01:56 PDT
https://hg.mozilla.org/mozilla-central/rev/195a2038433d
Comment 27 Norbert Lindenberg 2013-04-07 13:17:29 PDT
While the code changes for this bug made it into m-c in time for Firefox 22, they are disabled, and the build system changes necessary to actually enable them are still under review (bug 724533). I've updated the target release to reflect this.

I moved the section from the Firefox 22 compatibility document to that for Firefox 23, and added more information.

I also updated the reference pages - would you like to review them?
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/localeCompare
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number/toLocaleString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString
Comment 28 Florian Scholz [:fscholz] (MDN) 2013-10-17 15:53:20 PDT
Added a note to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23#JavaScript as well.
Marking this as complete, looks good. Thanks Norbert!
Comment 29 Norbert Lindenberg 2013-10-17 19:21:04 PDT
Sorry for the confusion, but the internationalization API and thus the changes in this bug have remained disabled for all Firefox releases at least up to 26. They're now enabled in nightly, so if all goes well they may get into Firefox 27. I've removed the note from the Firefox 23 release notes, to be added to the notes for the release in which the API actually ships. The bug that really determines whether the API is in or not is bug 853301.
Comment 30 Florian Scholz [:fscholz] (MDN) 2014-02-05 03:24:37 PST
Going to mark this as complete as the reference docs for String.localeCompare, Number.toLocaleString, Date.toLocaleString are in place (see comment 27). I will use bug 853301 to update the dev doc release notes and compat tables as that bug is actually about enabling the Internationalization API and has less confusing version info.

Note You need to log in before you can comment on or make changes to this bug.