Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString per ECMA-402

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Norbert Lindenberg, Assigned: Norbert Lindenberg)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla27
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t][DocArea=JS])

Attachments

(4 attachments, 7 obsolete attachments)

13.33 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.40 KB, patch
Norbert Lindenberg
: review+
Details | Diff | Splinter Review
23.21 KB, patch
Norbert Lindenberg
: review+
Details | Diff | Splinter Review
5.58 KB, patch
Norbert Lindenberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
Whiteboard: [js:t]
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 769872
(Assignee)

Comment 3

5 years ago
Created attachment 666413 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Attachment #660330 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 673765 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Attachment #666413 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 685049 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Attachment #673765 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 699997 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)
Attachment #685049 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 837963
(Assignee)

Updated

5 years ago
No longer blocks: 769872
(Assignee)

Updated

5 years ago
Depends on: 847000
(Assignee)

Updated

5 years ago
Summary: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU → Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString per ECMA-402
(Assignee)

Comment 7

5 years ago
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.
Attachment #699997 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 726759 [details] [diff] [review]
Reimplement String.localeCompare per ECMA-402.
Attachment #726759 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

5 years ago
Created attachment 726761 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.
Attachment #726761 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

5 years ago
Created attachment 726765 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.
Attachment #726765 - Flags: review?(jwalden+bmo)
Attachment #726759 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #726761 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #726765 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

5 years ago
Attachment #726759 - Flags: checkin?(jwalden+bmo)
(Assignee)

Comment 13

5 years ago
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().
Attachment #726761 - Attachment is obsolete: true
Attachment #727091 - Flags: review+
Attachment #727091 - Flags: checkin?(jwalden+bmo)
(Assignee)

Comment 14

5 years ago
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.
Attachment #726765 - Attachment is obsolete: true
Attachment #727093 - Flags: review+
Attachment #727093 - Flags: checkin?(jwalden+bmo)
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 853301
Comment on attachment 726759 [details] [diff] [review]
Reimplement String.localeCompare per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71b015ae13c3
Attachment #726759 - Flags: checkin?(jwalden+bmo)
Comment on attachment 727091 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bebe0562ab3
Attachment #727091 - Flags: checkin?(jwalden+bmo)
Comment on attachment 727093 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d550a01e0ab2
Attachment #727093 - Flags: checkin?(jwalden+bmo)
(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.
Created attachment 728003 [details] [diff] [review]
Remove x_CheckThisX methods, as explained before
Attachment #728003 - Flags: review?(mozillabugs)
Also pretend I removed the methods' additions from SelfHosting.cpp, turns out that's necessary to compile.  :-)
https://hg.mozilla.org/mozilla-central/rev/71b015ae13c3
https://hg.mozilla.org/mozilla-central/rev/5bebe0562ab3
https://hg.mozilla.org/mozilla-central/rev/d550a01e0ab2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Comment 23

5 years ago
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.
Attachment #728003 - Flags: review?(mozillabugs) → review+
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/195a2038433d
https://hg.mozilla.org/mozilla-central/rev/195a2038433d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Also, please update the following docs:
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
Keywords: dev-doc-needed
(Assignee)

Comment 27

4 years ago
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
Target Milestone: mozilla22 → mozilla23
(Assignee)

Updated

4 years ago
Blocks: 864843
(Assignee)

Updated

4 years ago
Blocks: 866301
(Assignee)

Updated

4 years ago
Blocks: 866305
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!
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 29

4 years ago
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.
Keywords: dev-doc-complete → dev-doc-needed
Target Milestone: mozilla23 → mozilla27
Whiteboard: [js:t] → [js:t][DocArea=JS]
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.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1215247
Blocks: 1215252
You need to log in before you can comment on or make changes to this bug.