Closed
Bug 769871
Opened 12 years ago
Closed 12 years ago
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString per ECMA-402
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: mozillabugs, Assigned: mozillabugs)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [js:t][DocArea=JS])
Attachments
(4 files, 7 obsolete files)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozillabugs
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozillabugs
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozillabugs
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Whiteboard: [js:t]
Assignee | ||
Comment 1•12 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•12 years ago
|
||
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 | ||
Comment 3•12 years ago
|
||
Attachment #660330 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #666413 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #673765 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #685049 -
Attachment is obsolete: true
Assignee | ||
Updated•12 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•12 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•12 years ago
|
||
Attachment #726759 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #726761 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #726765 -
Flags: review?(jwalden+bmo)
Updated•12 years ago
|
Attachment #726759 -
Flags: review?(jwalden+bmo) → review+
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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•12 years ago
|
Attachment #726759 -
Flags: checkin?(jwalden+bmo)
Assignee | ||
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
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)
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
Attachment #728003 -
Flags: review?(mozillabugs)
Comment 21•12 years ago
|
||
Also pretend I removed the methods' additions from SelfHosting.cpp, turns out that's necessary to compile. :-)
Comment 22•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 23•12 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•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•12 years ago
|
||
Comment 25•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
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•12 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
Comment 28•11 years ago
|
||
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•11 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
Updated•11 years ago
|
Whiteboard: [js:t] → [js:t][DocArea=JS]
Comment 30•11 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•