[webkit-reviews] review granted: [Bug 216521] [JSC] Apply Intl.DateTimeFormat hour-cycle correctly when timeStyle is used : [Attachment 408793] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 216521: [JSC] Apply Intl.DateTimeFormat hour-cycle correctly when timeStyle
is used
https://bugs.webkit.org/show_bug.cgi?id=216521

Attachment 408793: Patch

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




--- Comment #6 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 408793
  --> https://bugs.webkit.org/attachment.cgi?id=408793
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