Bug 209778 – [ECMA-402] Implement Intl.DateTimeFormat.prototype.formatRange

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

[Shane Carr]

The formatRange function for Intl.DateTimeFormat is at Stage 3 in TC39; the proposal is on track to reach Stage 4 in 2020.  V8 has had it ready for some time, and SpiderMonkey plans to add it in the first half of this year.

As usage of formatRange in Intl.DateTimeFormat increases throughout the ecosystem, WebKit users will be left behind with legacy polyfills, leading to broken web sites and/or poorer performance relative to other browsers.  At Google, we are currently weighing our options for calling these features of formatRange in Intl.DateTimeFormat in supported environments in order to give users better performance and smaller download sizes.

ICU4C exposes C and C++ APIs that can be used to implement formatRange in Intl.DateTimeFormat.  Implementing formatRange in Intl.DateTimeFormat is largely a matter of adding the glue between JavaScript and ICU4C, as WebKit already does for other Intl types.

Proposal repo: https://github.com/fabalbon/proposal-intl-DateTimeFormat-formatRange

[Radar WebKit Bug Importer]

<rdar://problem/61079767>

[Yusuke Suzuki]

Use UDateIntervalFormat to implement this.
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/udateintervalformat_8h.html

[Yusuke Suzuki]

formatRangeToParts requires ICU API (udtitvfmt_formatToResult ) which is added in 67 and it is draft state. Possibly we will skip it.

[Yusuke Suzuki]

Spawned https://bugs.webkit.org/show_bug.cgi?id=213822

[Yusuke Suzuki]

Created attachment 403255 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 403260 [details]
Patch

[Ross Kirsling]

Comment on attachment 403260 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:649
> +    dataLogLnIf(IntlDateTimeFormatInternal::verbose, "locale:(", m_locale, "),dataLocale:(", dataLocaleWithExtensions, "),pattern:(", pattern, ")");

I feel kind of unclear about when these verbose logs should actually be landed, but I don't object if you think it'll continue to be useful.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1054
> +    // http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-partitiondatetimerangepattern

Should this be at the top of the function for consistency?

> JSTests/ChangeLog:13
> +        * stress/intl-datetimeformat-formatrange-relevant-extensions.js: Added.

Seems a little arbitrary that this is split out into a separate file, but glad to see we've tested various options and extensions.
I think we need to cover a non-English locale too though.

> JSTests/test262/expectations.yaml:1612
> +  default: 'Test262Error: no args Expected a RangeError but got a TypeError'

Is this one really related to ICU version?
We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`.

[Yusuke Suzuki]

Comment on attachment 403260 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:649
>> +    dataLogLnIf(IntlDateTimeFormatInternal::verbose, "locale:(", m_locale, "),dataLocale:(", dataLocaleWithExtensions, "),pattern:(", pattern, ")");
> 
> I feel kind of unclear about when these verbose logs should actually be landed, but I don't object if you think it'll continue to be useful.

Yeah, this is very useful to check what locale is passed and what pattern is generated, since this is the input of ICU.
When I see some problems, I check this first. If this is reasonable, then we need to look into ICU. If these inputs looks non-reasonable, then our frontend to ICU is not correct :)

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1054
>> +    // http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-partitiondatetimerangepattern
> 
> Should this be at the top of the function for consistency?

I think here is better since this URL is not for formatRange, this is for PartitionDateTimeRangePattern abstract operation called by formatRange.

>> JSTests/ChangeLog:13
>> +        * stress/intl-datetimeformat-formatrange-relevant-extensions.js: Added.
> 
> Seems a little arbitrary that this is split out into a separate file, but glad to see we've tested various options and extensions.
> I think we need to cover a non-English locale too though.

Right. I'll add some more.

>> JSTests/test262/expectations.yaml:1612
>> +  default: 'Test262Error: no args Expected a RangeError but got a TypeError'
> 
> Is this one really related to ICU version?
> We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`.

Oops, right!!! Fixed.

[Yusuke Suzuki]

Comment on attachment 403260 [details]
Patch

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

>>> JSTests/test262/expectations.yaml:1612
>>> +  default: 'Test262Error: no args Expected a RangeError but got a TypeError'
>> 
>> Is this one really related to ICU version?
>> We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`.
> 
> Oops, right!!! Fixed.

Oh, interesting. This is test262's bug.
http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-intl.datetimeformat.prototype.formatRange
"If startDate is undefined or endDate is undefined, throw a TypeError exception."

 20 var dtf = new Intl.DateTimeFormat();
 21 
 22 assert.throws(RangeError, function() {
 23   dtf.formatRange(); // Not possible to poison this one
 24 }, "no args");

[Yusuke Suzuki]

Created attachment 403266 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 403271 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 403260 [details]
Patch

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

>>>> JSTests/test262/expectations.yaml:1612
>>>> +  default: 'Test262Error: no args Expected a RangeError but got a TypeError'
>>> 
>>> Is this one really related to ICU version?
>>> We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`.
>> 
>> Oops, right!!! Fixed.
> 
> Oh, interesting. This is test262's bug.
> http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-intl.datetimeformat.prototype.formatRange
> "If startDate is undefined or endDate is undefined, throw a TypeError exception."
> 
>  20 var dtf = new Intl.DateTimeFormat();
>  21 
>  22 assert.throws(RangeError, function() {
>  23   dtf.formatRange(); // Not possible to poison this one
>  24 }, "no args");

Uploaded the PR https://github.com/tc39/test262/pull/2685

[Yusuke Suzuki]

Created attachment 403272 [details]
Patch

[Ross Kirsling]

Comment on attachment 403272 [details]
Patch

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

r=me.

> JSTests/test262/expectations.yaml:1617
> +test/intl402/DateTimeFormat/prototype/formatRangeToParts/argument-date-string.js:

Assuming we're not passing any tests in this directory, we should just add a path skip to config.yaml (with a comment linking to bug 213822).

[Yusuke Suzuki]

Comment on attachment 403272 [details]
Patch

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

>> JSTests/test262/expectations.yaml:1617
>> +test/intl402/DateTimeFormat/prototype/formatRangeToParts/argument-date-string.js:
> 
> Assuming we're not passing any tests in this directory, we should just add a path skip to config.yaml (with a comment linking to bug 213822).

Thats nice!

[Yusuke Suzuki]

Created attachment 403275 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403300 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403304 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403317 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403319 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403324 [details]
Patch for landing

[Yusuke Suzuki]

Committed r266033: <https://trac.webkit.org/changeset/266033>