Bug 216521

Summary: [JSC] Apply Intl.DateTimeFormat hour-cycle correctly when timeStyle is used
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213425    
Attachments:
Description Flags
Patch
none
Patch
none
Patch ross.kirsling: review+

Description Yusuke Suzuki 2020-09-14 19:53:38 PDT
...
Comment 1 Yusuke Suzuki 2020-09-14 19:56:41 PDT
Created attachment 408781 [details]
Patch
Comment 2 Yusuke Suzuki 2020-09-14 22:11:39 PDT
Created attachment 408791 [details]
Patch
Comment 3 Yusuke Suzuki 2020-09-14 22:13:47 PDT
Comment on attachment 408791 [details]
Patch

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

> JSTests/stress/intl-date-time-format-date-time-style-hour-cycle.js:14
> +    shouldBe(o.format(now1), `00:00`);

If it is h23, we should not have AM.

> JSTests/stress/intl-date-time-format-date-time-style-hour-cycle.js:23
> +    shouldBe(o.format(now1), `24:00`);

If it is h24, we should not have AM.

> JSTests/stress/intl-date-time-format-date-time-style-hour-cycle.js:50
> +    shouldBe(o.format(now2), `12:00`);

If it is h23, we should not have PM.

> JSTests/stress/intl-date-time-format-date-time-style-hour-cycle.js:59
> +    shouldBe(o.format(now2), `12:00`);

If it is h24, we should not have PM.
Comment 4 Yusuke Suzuki 2020-09-14 22:16:11 PDT
Comment on attachment 408791 [details]
Patch

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

> JSTests/test262/expectations.yaml:1457
> +  default: 'Test262Error: Result for date=medium and time=full Expected SameValue(«May 1, 1886 at 2:12:47 PM Coordinated Universal Time», «May 1, 1886, 2:12:47 PM Coordinated Universal Time») to be true'
> +  strict mode: 'Test262Error: Result for date=medium and time=full Expected SameValue(«May 1, 1886 at 2:12:47 PM Coordinated Universal Time», «May 1, 1886, 2:12:47 PM Coordinated Universal Time») to be true'

This failure is due to ICU. I wonder if this is due to AppleICU since it passed in ICU67 on Linux. If it is AppleICU's intentional overriding, then we should adjust test262 side.
Comment 5 Yusuke Suzuki 2020-09-14 22:30:30 PDT
Created attachment 408793 [details]
Patch
Comment 6 Ross Kirsling 2020-09-15 11:58:32 PDT
Comment on attachment 408793 [details]
Patch

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

r=me. I don't love the idea of implementing behavior that isn't spec yet, but I understand that SM is committed to their implementation and that the currently spec'ed behavior is confusing for users.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:397
> +            switch (currentCharacter) {

Seems like this could easily share code with hourCycleFromPattern, but whether that ends up easier to read is up to you.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:-478
> -        // Set hour12 here to simplify hour logic later.

I'm happy this is being updated. I remember laughing at this comment before because the logic is really rather confusing.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:572
> +        //
> +        // 14. If hour12 is not undefined, then
> +        //     a. Let hourCycle be null.

Hmm, in this particular case, I'm not sure that the spec text is adding anything to your comment.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:719
> +        // [1]: https://github.com/tc39/ecma402/issues/402
> +        // [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1045791
> +        // [3]: https://github.com/tc39/ecma402/pull/436
> +        // [4]: https://github.com/tc39/ecma402/issues/402#issuecomment-623628320

This is excellent info for a ChangeLog, but feels like too much transient detail for a comment...
That said, I do think it's important to reference what we're aligning with.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:724
> +                skeletonCharacter = 'j';

Seems like we wouldn't need to actually set anything here if we're having 'j' be the default value.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:908
> +                    throwTypeError(globalObject, scope, "failed to initialize DateIntervalFormat"_s);

You mean DateTimeFormat? :D

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:917
> +                    throwTypeError(globalObject, scope, "failed to initialize DateIntervalFormat"_s);

Ditto.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:932
> +    // After generating pattern from skeleton, we need to change h11 v.s. h12 and h23 v.s. h24 if hourCycle is specified.

nittiest of nits: vs., not v.s.

> JSTests/stress/intl-datetimeformat-hour-cycle.js:1
> +// This test is specifically focusing on hour-cycle behavior of Intl.DateTimeFormat.

I think that's technically clear from the filename. :P
Comment 7 Yusuke Suzuki 2020-09-15 15:51:01 PDT
Comment on attachment 408793 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:397
>> +            switch (currentCharacter) {
> 
> Seems like this could easily share code with hourCycleFromPattern, but whether that ends up easier to read is up to you.

Sounds nice. I created hourCycleFromSymbol for this purpose.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:-478
>> -        // Set hour12 here to simplify hour logic later.
> 
> I'm happy this is being updated. I remember laughing at this comment before because the logic is really rather confusing.

lol, agree :D

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:572
>> +        //     a. Let hourCycle be null.
> 
> Hmm, in this particular case, I'm not sure that the spec text is adding anything to your comment.

OK, removed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:719
>> +        // [4]: https://github.com/tc39/ecma402/issues/402#issuecomment-623628320
> 
> This is excellent info for a ChangeLog, but feels like too much transient detail for a comment...
> That said, I do think it's important to reference what we're aligning with.

OK, removed these links while stating that this is aligned to SpiderMonkey's behavior.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:724
>> +                skeletonCharacter = 'j';
> 
> Seems like we wouldn't need to actually set anything here if we're having 'j' be the default value.

Removed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:908
>> +                    throwTypeError(globalObject, scope, "failed to initialize DateIntervalFormat"_s);
> 
> You mean DateTimeFormat? :D

Oops, lol. Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:917
>> +                    throwTypeError(globalObject, scope, "failed to initialize DateIntervalFormat"_s);
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:932
>> +    // After generating pattern from skeleton, we need to change h11 v.s. h12 and h23 v.s. h24 if hourCycle is specified.
> 
> nittiest of nits: vs., not v.s.

Fixed.

>> JSTests/stress/intl-datetimeformat-hour-cycle.js:1
>> +// This test is specifically focusing on hour-cycle behavior of Intl.DateTimeFormat.
> 
> I think that's technically clear from the filename. :P

Dropped :)
Comment 8 Yusuke Suzuki 2020-09-15 15:56:38 PDT
Committed r267108: <https://trac.webkit.org/changeset/267108>
Comment 9 Radar WebKit Bug Importer 2020-09-15 15:57:17 PDT
<rdar://problem/68947322>