Bug 213822 - [JSC] Implement Intl.DateTimeFormat.formatRangeToParts
Summary: [JSC] Implement Intl.DateTimeFormat.formatRangeToParts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 213425
  Show dependency tree / graph
 
Reported: 2020-06-30 17:06 PDT by Yusuke Suzuki
Modified: 2020-11-13 13:50 PST (History)
13 users (show)

See Also:

Attachments
Patch (25.85 KB, patch)
2020-11-09 03:07 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (25.90 KB, patch)
2020-11-09 03:10 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (25.94 KB, patch)
2020-11-09 03:24 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (26.02 KB, patch)
2020-11-09 03:32 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (26.09 KB, patch)
2020-11-09 03:43 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (26.09 KB, patch)
2020-11-09 11:40 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (33.38 KB, patch)
2020-11-09 20:06 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (35.44 KB, patch)
2020-11-10 00:21 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (252.97 KB, patch)
2020-11-10 21:59 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (254.72 KB, patch)
2020-11-10 22:35 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (254.78 KB, patch)
2020-11-10 23:41 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (254.79 KB, patch)
2020-11-11 03:06 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (253.94 KB, patch)
2020-11-11 14:08 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch for landing (254.73 KB, patch)
2020-11-11 14:12 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (254.83 KB, patch)
2020-11-11 15:42 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch for landing (255.27 KB, patch)
2020-11-11 16:30 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Show Obsolete (15) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>