Bug 216521 – [JSC] Apply Intl.DateTimeFormat hour-cycle correctly when timeStyle is used

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 408781 [details]
Patch

[Yusuke Suzuki]

Created attachment 408791 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 408793 [details]
Patch

[Ross Kirsling]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r267108: <https://trac.webkit.org/changeset/267108>

[Radar WebKit Bug Importer]

<rdar://problem/68947322>