Bug 216521

Created attachment 408781 [details] Patch

Created attachment 408791 [details] Patch

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

Created attachment 408793 [details] Patch

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

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

<rdar://problem/68947322>