Bug 209776

<rdar://problem/61079779>

Created attachment 402447 [details]
Patch

WIP

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.

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.

Created attachment 402450 [details]
Patch

WIP

Created attachment 402451 [details]
Patch

WIP

Created attachment 402469 [details]
Patch

Created attachment 402470 [details]
Patch

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.

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.

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.

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?

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 :)

(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.

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.

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 :)

Created attachment 402531 [details]
Patch

Created attachment 407055 [details]
Patch

Patch for landing

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

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