Bug 213822 – [JSC] Implement Intl.DateTimeFormat.formatRangeToParts

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

[Yusuke Suzuki]

Implement it once ICU udtitvfmt_formatToResult becomes stable.

[Radar WebKit Bug Importer]

<rdar://problem/69328711>

[Yusuke Suzuki]

Created attachment 413573 [details]
Patch

[Yusuke Suzuki]

Created attachment 413574 [details]
Patch

[Yusuke Suzuki]

Created attachment 413575 [details]
Patch

[Yusuke Suzuki]

Created attachment 413576 [details]
Patch

[Yusuke Suzuki]

Created attachment 413578 [details]
Patch

[Yusuke Suzuki]

Created attachment 413611 [details]
Patch

[Yusuke Suzuki]

Created attachment 413657 [details]
Patch

[Yusuke Suzuki]

Created attachment 413674 [details]
Patch

[Yusuke Suzuki]

Created attachment 413780 [details]
Patch

[Yusuke Suzuki]

Created attachment 413783 [details]
Patch

[Yusuke Suzuki]

Created attachment 413788 [details]
Patch

[Yusuke Suzuki]

Created attachment 413805 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 413805 [details]
Patch

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

> JSTests/stress/intl-datetimeformat-formatrangetoparts.js:454
> +if ($vm.icuHeaderVersion() >= 64)

I need to insert `useAppleInternalSDK()` check here.

[Ross Kirsling]

Comment on attachment 413783 [details]
Patch

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

r=me. Such a complex feature, even before having to worry about ICU versions and Gregorian vs. Julian...
I really appreciate the detailed comments and thorough tests. #マジ乙

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1647
> +    auto sourceType = [&](int32_t index) -> JSString* {

Seems like you could actually move this into createPart?

> Source/WTF/wtf/Range.h:118
> +    bool subsumes(const Range& other) const

Is this used implicitly somewhere, or did it end up unnecessary?

> JSTests/ChangeLog:33
> +        * test262/expectations.yaml:

Let's add a note here about the version-dependent failures.

[Ross Kirsling]

Comment on attachment 413805 [details]
Patch

Oops, I r+ed an obsolete patch. r+ing the latest one, contingent on making EWS happy.

[Yusuke Suzuki]

Comment on attachment 413783 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1647
>> +    auto sourceType = [&](int32_t index) -> JSString* {
> 
> Seems like you could actually move this into createPart?

That's good. Changed.

>> Source/WTF/wtf/Range.h:118
>> +    bool subsumes(const Range& other) const
> 
> Is this used implicitly somewhere, or did it end up unnecessary?

Yeah, originally, I thought ICU was producing right interval and we can split "literal" into ranges, but I ensured it isn't. So this function is not used. I'll drop it for now.

>> JSTests/ChangeLog:33
>> +        * test262/expectations.yaml:
> 
> Let's add a note here about the version-dependent failures.

Added.

[Yusuke Suzuki]

Created attachment 413862 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 413862 [details]
Patch

Oops, cleared Ross's r+ accidentally.

[Yusuke Suzuki]

Created attachment 413866 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 413876 [details]
Patch

[Yusuke Suzuki]

Only failed test was stress/intl-datetimeformat-formatrangetoparts.js, and this is because it is using formatRangeToParts in OSS WebKit.
I've fixed the test, and ensured it pass.

[Yusuke Suzuki]

Created attachment 413883 [details]
Patch for landing

[Yusuke Suzuki]

https://trac.webkit.org/changeset/269706/webkit

[Yusuke Suzuki]

Committed r269707: <https://trac.webkit.org/changeset/269707>

[Michael Catanzaro]

Hi, this reverted r269667. Was that an accident? It breaks all the tests that commit fixed. All the new tests are broken too (on Fedora 32 with ICU 65.1) since the code checks for ICU >= 67 rather than >= 65. E.g.:

Running stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: Exception: Error: bad value:  –  expected value:  – 
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: [e-mail]:3:24
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: [e-mail]:10:17
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: [e-mail]:57:17
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: global [e-mail]:1776:9
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default

[Caio Lima]

Comment on attachment 413876 [details]
Patch

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

> JSTests/stress/intl-datetimeformat-formatrange.js:40
> +    if ($vm.icuVersion() >= 67)

This branch is making such test fail for ARMv7 and MIPS. This test passes if we have line #41 assignment running. We are using a very old ICU version (ICU 60) on buildbots and EWS.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #26)
> Hi, this reverted r269667. Was that an accident? It breaks all the tests
> that commit fixed. All the new tests are broken too (on Fedora 32 with ICU
> 65.1) since the code checks for ICU >= 67 rather than >= 65. E.g.:
> 
> Running
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> Exception: Error: bad value:  –  expected value:  – 
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> [e-mail]:3:24
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> [e-mail]:
> 10:17
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> [e-mail]:57:17
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> global
> [e-mail]:1776:9
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> ERROR: Unexpected exit code: 3
> FAIL:
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default

Please skip them, or just adjust it.
I have no ability to support all ICU versions. The supported ones are ICU 64, 66, and 67 in my patch.

[Yusuke Suzuki]

Committed r269791: <https://trac.webkit.org/changeset/269791>