Bug 185796 – [INTL] Call Typed Array elements toLocaleString with locale and options

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

[INTL] Call Typed Array elements toLocaleString with locale and options

[Andy VanWagoner]

Created attachment 340768 [details]
Patch

[Andy VanWagoner]

This behavior is currently observable in SpiderMonkey. I don't believe v8 has yet adopted it. test262/intl402 specifically tests for this behavior.

I would imagine eventually TypedArray toLocaleString and Array toLocaleString will use some of the semantics of ListFormat[1], but for now this at least passes NumberFormat locale and options to each number's toLocaleString in a typed array.

[1] https://github.com/tc39/proposal-intl-list-format

[Andy VanWagoner]

Is there anything else this patch needs before it can be considered?

[Andy VanWagoner]

Created attachment 343721 [details]
Rebased

[WebKit Commit Bot]

Comment on attachment 343721 [details]
Rebased

Rejecting attachment 343721 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 343721, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=343721&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=185796&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 343721 from bug 185796.
Fetching: https://bugs.webkit.org/attachment.cgi?id=343721
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Keith Miller']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 4 diffs from patch file(s).
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/builtins/TypedArrayPrototype.js
patching file JSTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file JSTests/test262/expectations.yaml
Hunk #1 FAILED at 1825.
1 out of 1 hunk FAILED -- saving rejects to file JSTests/test262/expectations.yaml.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Keith Miller']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8651222

[Andy VanWagoner]

Created attachment 345765 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345765 [details]
Patch

Clearing flags on attachment: 345765

Committed r234207: <https://trac.webkit.org/changeset/234207>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42589763>

[Darin Adler]

Comment on attachment 345765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345765&action=review

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:410
> -    var string = this[0].toLocaleString();
> +    var string = this[0].toLocaleString(@argument(0), @argument(1));
>      for (var i = 1; i < length; i++)
> -        string += "," + this[i].toLocaleString();
> +        string += "," + this[i].toLocaleString(@argument(0), @argument(1));

Presumably the toLocaleString function can do any arbitrary work, so it can examine its arguments any way it likes. This will turn "no arguments" into "undefined, undefined", and will turn three arguments into two; these behaviors might be OK and might not be. I’m not clear what the specification calls for here. Do we want, instead, to forward the entire array of arguments, without changing its length?

[Keith Miller]

Comment on attachment 345765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345765&action=review

>> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:410
>> +        string += "," + this[i].toLocaleString(@argument(0), @argument(1));
> 
> Presumably the toLocaleString function can do any arbitrary work, so it can examine its arguments any way it likes. This will turn "no arguments" into "undefined, undefined", and will turn three arguments into two; these behaviors might be OK and might not be. I’m not clear what the specification calls for here. Do we want, instead, to forward the entire array of arguments, without changing its length?

This appears to be what the specification in Intl 402 asks for:

https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring.

[Andy VanWagoner]

I read the spec in the way the patch has it, but at least for the default Number.prototype.toLocaleString, passing along the arguments unaltered, either by using apply, or with rest args and spread args would have the same effect.

For places where the Number.prototype.toLocaleString has been altered, I'd expect it to receive [ undefined, undefined ] from the text of the spec.

As a quick test I ran the following in Firefox:
```
Number.prototype.toLocaleString = function () { console.log(arguments) }
new Uint8Array([ 5 ]).toLocaleString()
```

which gave me:
```
Arguments { 0: undefined, 1: undefined }
```

That seems to match the implementation in this patch.

[Darin Adler]

(In reply to Keith Miller from comment #11)
> This appears to be what the specification in Intl 402 asks for:
> 
> https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring.

OK, great. I read it over and it is what’s called for.

(In reply to Andy VanWagoner from comment #12)
> for the default Number.prototype.toLocaleString

Right, my comment was not about the default function.

> Arguments { 0: undefined, 1: undefined }

Good, Firefox implementation matches.