Bug 213822

Summary: [JSC] Implement Intl.DateTimeFormat.formatRangeToParts
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, ross.kirsling, saam, ticaiolima, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209778
Bug Depends on:    
Bug Blocks: 213425    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch for landing none

Description Yusuke Suzuki 2020-06-30 17:06:15 PDT 
Implement it once ICU udtitvfmt_formatToResult becomes stable.
Comment 1 Radar WebKit Bug Importer 2020-09-21 14:43:56 PDT 
<rdar://problem/69328711>
Comment 2 Yusuke Suzuki 2020-11-09 03:07:07 PST 
Created attachment 413573 [details]
Patch
Comment 3 Yusuke Suzuki 2020-11-09 03:10:35 PST 
Created attachment 413574 [details]
Patch
Comment 4 Yusuke Suzuki 2020-11-09 03:24:43 PST 
Created attachment 413575 [details]
Patch
Comment 5 Yusuke Suzuki 2020-11-09 03:32:54 PST 
Created attachment 413576 [details]
Patch
Comment 6 Yusuke Suzuki 2020-11-09 03:43:57 PST 
Created attachment 413578 [details]
Patch
Comment 7 Yusuke Suzuki 2020-11-09 11:40:33 PST 
Created attachment 413611 [details]
Patch
Comment 8 Yusuke Suzuki 2020-11-09 20:06:25 PST 
Created attachment 413657 [details]
Patch
Comment 9 Yusuke Suzuki 2020-11-10 00:21:25 PST 
Created attachment 413674 [details]
Patch
Comment 10 Yusuke Suzuki 2020-11-10 21:59:09 PST 
Created attachment 413780 [details]
Patch
Comment 11 Yusuke Suzuki 2020-11-10 22:35:42 PST 
Created attachment 413783 [details]
Patch
Comment 12 Yusuke Suzuki 2020-11-10 23:41:01 PST 
Created attachment 413788 [details]
Patch
Comment 13 Yusuke Suzuki 2020-11-11 03:06:06 PST 
Created attachment 413805 [details]
Patch
Comment 14 Yusuke Suzuki 2020-11-11 11:09:04 PST 
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.
Comment 15 Ross Kirsling 2020-11-11 11:43:10 PST 
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.
Comment 16 Ross Kirsling 2020-11-11 11:46:08 PST 
Comment on attachment 413805 [details]
Patch

Oops, I r+ed an obsolete patch. r+ing the latest one, contingent on making EWS happy.
Comment 17 Yusuke Suzuki 2020-11-11 14:03:43 PST 
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.
Comment 18 Yusuke Suzuki 2020-11-11 14:08:49 PST 
Created attachment 413862 [details]
Patch
Comment 19 Yusuke Suzuki 2020-11-11 14:09:31 PST 
Comment on attachment 413862 [details]
Patch

Oops, cleared Ross's r+ accidentally.
Comment 20 Yusuke Suzuki 2020-11-11 14:12:55 PST 
Created attachment 413866 [details]
Patch for landing
Comment 21 Yusuke Suzuki 2020-11-11 15:42:08 PST 
Created attachment 413876 [details]
Patch
Comment 22 Yusuke Suzuki 2020-11-11 16:23:44 PST 
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.
Comment 23 Yusuke Suzuki 2020-11-11 16:30:33 PST 
Created attachment 413883 [details]
Patch for landing
Comment 24 Yusuke Suzuki 2020-11-11 16:37:56 PST 
https://trac.webkit.org/changeset/269706/webkit
Comment 25 Yusuke Suzuki 2020-11-11 16:38:22 PST 
Committed r269707: <https://trac.webkit.org/changeset/269707>
Comment 26 Michael Catanzaro 2020-11-12 07:02:37 PST 
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: shouldBe@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:3:24
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: compareParts@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:10:17
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: test@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:57:17
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: global code@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:1776:9
stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default
Comment 27 Caio Lima 2020-11-12 09:48:43 PST 
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.
Comment 28 Yusuke Suzuki 2020-11-13 13:38:02 PST 
(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:
> shouldBe@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:3:24
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> compareParts@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:
> 10:17
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> test@intl-datetimeformat-formatrangetoparts-relevant-extensions.js:57:17
> stress/intl-datetimeformat-formatrangetoparts-relevant-extensions.js.default:
> global
> code@intl-datetimeformat-formatrangetoparts-relevant-extensions.js: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.
Comment 29 Yusuke Suzuki 2020-11-13 13:50:34 PST 
Committed r269791: <https://trac.webkit.org/changeset/269791>