Reimplement Array.prototype.toLocaleString as per ECMA-402 Edition 2.0 (Intl)

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: fscholz, Assigned: anba)

Tracking

({dev-doc-complete})

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [DocArea=JS], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
ECMA-402 Edition 1.0 reimplemented String.localeCompare, Number.toLocaleString, Date.toLocaleString (bug 769871). Now with ECMA-402 Edition 2.0, a reimplementation of Array.prototype.toLocaleString gets introduced as well.

Array.prototype.toLocaleString([locales = undefined [, options = undefined ]])

Spec bug: https://bugs.ecmascript.org/show_bug.cgi?id=2383
ECMA-402 2.0 Draft: http://wiki.ecmascript.org/doku.php?id=globalization:specification_drafts
Discussion: https://esdiscuss.org/topic/tolocalestring-in-object-and-array
(Reporter)

Updated

3 years ago
Component: JavaScript: Standard Library → JavaScript: Internationalization API
(Assignee)

Comment 1

3 years ago
Posted patch bug1130636.patch (obsolete) — Splinter Review
More or less the same as bug 1121938.

I've moved the implementation to self-hosted code to avoid unnecessary round-trips from native to script code (all current .toLocaleString methods are already self-hosted). To avoid a (somewhat) noticeable regression for the empty array case, I had to handle that case in the cpp code. I've also reordered the sub-steps of step 9 to avoid adding an empty string if the element is undefined/null.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8771672 - Flags: review?(jwalden+bmo)
Comment on attachment 8771672 [details] [diff] [review]
bug1130636.patch

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

We should move the cycle-handling into the self-hosted code eventually (and probably into the spec?), but this is a good step for now.

::: js/src/builtin/Array.js
@@ +906,5 @@
> +        return "";
> +
> +    // Step 3 (reordered).
> +    // We don't (yet?) implement locale-dependent separators.
> +    var separator = ",";

Move this step down before the loop in steps 8-9, since it's not observable what it does (for us).

::: js/src/tests/Intl/Array/toLocaleString-date.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("Intl"))

Overall |if|.

::: js/src/tests/Intl/Array/toLocaleString-number.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("Intl"))

Still the same overall-if thing.

::: js/src/tests/Intl/Array/toLocaleString.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("Intl"))

And this, I'd guard the test body with |if (typeof Intl === "object)|.

::: js/src/tests/ecma_6/Array/toLocaleString-nointl.js
@@ +1,1 @@
> +// |reftest| skip-if(this.hasOwnProperty("Intl"))

I might surround the test in |if (typeof Intl !== "object")|, because manifest comments make for tests that aren't as straightforward, IMO.
Attachment #8771672 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 3

3 years ago
Posted patch bug1130636.patch (obsolete) — Splinter Review
Updated patch, carrying r+ from Waldo.
Attachment #8771672 - Attachment is obsolete: true
Attachment #8774847 - Flags: review+
(Assignee)

Comment 4

3 years ago
Patch needed bit-unrotting, carrying r+ from Waldo.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945
Attachment #8774847 - Attachment is obsolete: true
Attachment #8798067 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 5

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6506191e894
Reimplement Array.prototype.toLocaleString as per ECMA-402, 2nd edition. r=Waldo
Keywords: checkin-needed

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6506191e894
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.