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

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Florian Scholz (Open Web Docs)]

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

Comment 1

[André Bargull [:anba]]

Attachment: bug1130636.patch

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.

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[André Bargull [:anba]]

Attachment: bug1130636.patch

Updated patch, carrying r+ from Waldo.

Comment 4

[André Bargull [:anba]]

Attachment: bug1130636.patch

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6506191e894
Reimplement Array.prototype.toLocaleString as per ECMA-402, 2nd edition. r=Waldo

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a6506191e894

Comment 7

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString
https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript