Bug 209776 – [ECMA-402] Intl.DateTimeFormat dateStyle/timeStyle missing in WebKit

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

[Shane Carr]

The dateStyle/timeStyle options for Intl.DateTimeFormat are at Stage 3 in TC39; the proposal is on track to reach Stage 4 in the first half of this year, and the other major browser engines (V8 and SpiderMonkey) have had it ready for some time.

As usage of these features of 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 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 dateStyle/timeStyle.  Implementing dateStyle/timeStyle 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/tc39/proposal-intl-datetime-style

[Radar WebKit Bug Importer]

<rdar://problem/61079779>

[Yusuke Suzuki]

Created attachment 402447 [details]
Patch

WIP

[Yusuke Suzuki]

Comment on attachment 402447 [details]
Patch

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

> JSTests/test262/expectations.yaml:1643
> +  strict mode: 'Test262Error: Result for full with {} Expected SameValue(«14:12:47 PM Coordinated Universal Time», «2:12:47 PM Coordinated Universal Time») to be true'

I think, this and the below test requires "udatpg_getDefaultHourCycle" to fix, which is introduced in ICU 67.

[Yusuke Suzuki]

Comment on attachment 402447 [details]
Patch

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

>> JSTests/test262/expectations.yaml:1643
>> +  strict mode: 'Test262Error: Result for full with {} Expected SameValue(«14:12:47 PM Coordinated Universal Time», «2:12:47 PM Coordinated Universal Time») to be true'
> 
> I think, this and the below test requires "udatpg_getDefaultHourCycle" to fix, which is introduced in ICU 67.

Or, I think we should first generate some pattern from generator to determine which is the right default hour-cycle.

[Yusuke Suzuki]

Created attachment 402450 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 402451 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 402469 [details]
Patch

[Yusuke Suzuki]

Created attachment 402470 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 402470 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:584
> +    }

Even if `hour` is not specified `timeStyle` can use m_hourCycle. So we do not clear it at this point. Later, we will clear it if hour is null or timeStyle is null.

[Darin Adler]

Comment on attachment 402470 [details]
Patch

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

I probably should’ve waited to review until the test results from EWS were in. R=me assuming everything passes.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:641
> +        auto toUDateFormatStyle = [](const String& style) {

We often use the word parse for functions that interpret a string and return a parsed form of it. I think it would be good to name this parse instead of to.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642
> +            if (style.isNull())

I don’t think we need to optimize the null case. I suggest leaving out this if statement.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:644
> +            if (style == "full"_s)

I am not sure that String == ASCIILiteral is properly implemented/optimized. In the past, in fact, sometimes it involved  creating and then destroying a String. Could you check on this?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:680
> +        UDateTimePatternGenerator* generator = udatpg_open(dataLocaleWithExtensions.data(), &status);

auto?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:748
> +    auto toDateTimeStyle = [](const String& style) {

Ditto.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:749
> +        if (style.isNull())

Ditto.

[Yusuke Suzuki]

Comment on attachment 402470 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:641
>> +        auto toUDateFormatStyle = [](const String& style) {
> 
> We often use the word parse for functions that interpret a string and return a parsed form of it. I think it would be good to name this parse instead of to.

Nice. Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642
>> +            if (style.isNull())
> 
> I don’t think we need to optimize the null case. I suggest leaving out this if statement.

Sounds nice. Done.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:644
>> +            if (style == "full"_s)
> 
> I am not sure that String == ASCIILiteral is properly implemented/optimized. In the past, in fact, sometimes it involved  creating and then destroying a String. Could you check on this?

Right! I thought the exact same thing while writing this code. So..., I ensured it :D

397 inline bool operator==(const String& a, const char* b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b)); }
398 inline bool operator==(const String& a, ASCIILiteral b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b.characters())); }

407 inline bool operator!=(const String& a, const char* b) { return !equal(a.impl(), reinterpret_cast<const LChar*>(b)); }
408 inline bool operator!=(const String& a, ASCIILiteral b) { return !equal(a.impl(), reinterpret_cast<const LChar*>(b.characters())); }

We can use ASCIILiteral :)

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:680
>> +        UDateTimePatternGenerator* generator = udatpg_open(dataLocaleWithExtensions.data(), &status);
> 
> auto?

Sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:748
>> +    auto toDateTimeStyle = [](const String& style) {
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:749
>> +        if (style.isNull())
> 
> Ditto.

Fixed.

[Ross Kirsling]

Comment on attachment 402470 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:631
> +            throwTypeError(globalObject, scope, "dateStyle or timeStyle requires all the date and time formats are not specified"_s);

Maybe "dateStyle and timeStyle may not be used with other DateTimeFormat options"?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:81
> +    enum class DateTimeStyle : uint8_t { None, Full, Long, Medium, Short };

What about just using UDateFormatStyle directly?
This is what I did in RelativeTimeFormat (with PluralRules as precedent).

> JSTests/test262/expectations.yaml:1641
> +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js:

Is this failure due to ICU version? Can you explain in the ChangeLog?

[Yusuke Suzuki]

Comment on attachment 402470 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:631
>> +            throwTypeError(globalObject, scope, "dateStyle or timeStyle requires all the date and time formats are not specified"_s);
> 
> Maybe "dateStyle and timeStyle may not be used with other DateTimeFormat options"?

Sounds better! Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:81
>> +    enum class DateTimeStyle : uint8_t { None, Full, Long, Medium, Short };
> 
> What about just using UDateFormatStyle directly?
> This is what I did in RelativeTimeFormat (with PluralRules as precedent).

One of the sad thing is that UDateFormatStyle is C enum. This means that it takes `int` size instead of `uint8_t` size, and it can affect on the sizeof(IntlDateTimeFormat). So I would like to stick on the above enum to keep size small :)

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #13)
> One of the sad thing is that UDateFormatStyle is C enum. This means that it
> takes `int` size instead of `uint8_t` size, and it can affect on the
> sizeof(IntlDateTimeFormat). So I would like to stick on the above enum to
> keep size small :)

Oh that's a good point. I'd only been thinking about the time cost of converting back and forth.

[Yusuke Suzuki]

Comment on attachment 402470 [details]
Patch

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

>> JSTests/test262/expectations.yaml:1641
>> +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js:
> 
> Is this failure due to ICU version? Can you explain in the ChangeLog?

This is hcDefault's bug. Our IntlDateTimeFormat is not spec compliant yet about hcDefault. We are guessing hcDefault value by parsing the resulted pattern. But I found that this returns incorrect hcDefault value in some cases, and this is one of the case.test/intl402/DateTimeFormat/prototype/resolvedOptions/hourCycle-default.js is also derived from the same hcDefault issue.

[Yusuke Suzuki]

Comment on attachment 402470 [details]
Patch

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

>>> JSTests/test262/expectations.yaml:1641
>>> +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js:
>> 
>> Is this failure due to ICU version? Can you explain in the ChangeLog?
> 
> This is hcDefault's bug. Our IntlDateTimeFormat is not spec compliant yet about hcDefault. We are guessing hcDefault value by parsing the resulted pattern. But I found that this returns incorrect hcDefault value in some cases, and this is one of the case.test/intl402/DateTimeFormat/prototype/resolvedOptions/hourCycle-default.js is also derived from the same hcDefault issue.

And this FIXME

    // FIXME: We should cache `hourCycleDefault` value obtained by generating "jjmm" pattern from UDateTimePatternGenerator.
    // https://bugs.webkit.org/show_bug.cgi?id=213459

is for that issue :)

[Yusuke Suzuki]

Created attachment 402531 [details]
Patch

[Yusuke Suzuki]

Created attachment 407055 [details]
Patch

Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 407055 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Yusuke Suzuki]

Committed r266035: <https://trac.webkit.org/changeset/266035>