Bug 209776

Summary: [ECMA-402] Intl.DateTimeFormat dateStyle/timeStyle missing in WebKit
Product: WebKit Reporter: Shane Carr <sffc>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: andy, darin, ews-watchlist, joepeck, keith_miller, mark.lam, mmaxfield, msaboff, ross.kirsling, saam, sffc, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213425    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
darin: review+
Patch
none
Patch ews-feeder: commit-queue-

Shane Carr
Reported 2020-03-30 14:47:58 PDT
The dateStyle/timeStyle options for Intl.DateTimeFormat are at Stage 3 in TC39; the proposal is on track to reach Stage 4 in the first half of this year, and the other major browser engines (V8 and SpiderMonkey) have had it ready for some time. As usage of these features of Intl.DateTimeFormat increases throughout the ecosystem, WebKit users will be left behind with legacy polyfills, leading to broken web sites and/or poorer performance relative to other browsers. At Google, we are currently weighing our options for calling these features of Intl.DateTimeFormat in supported environments in order to give users better performance and smaller download sizes. ICU4C exposes C and C++ APIs that can be used to implement dateStyle/timeStyle. Implementing dateStyle/timeStyle is largely a matter of adding the glue between JavaScript and ICU4C, as WebKit already does for other Intl types. Proposal repo: https://github.com/tc39/proposal-intl-datetime-style
Attachments
Patch (19.98 KB, patch)
2020-06-21 23:30 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (19.93 KB, patch)
2020-06-22 01:17 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (19.93 KB, patch)
2020-06-22 01:26 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (23.59 KB, patch)
2020-06-22 07:37 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (25.71 KB, patch)
2020-06-22 07:43 PDT, Yusuke Suzuki 		darin: review+
DetailsFormatted DiffDiff
Patch (25.57 KB, patch)
2020-06-22 20:52 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (24.92 KB, patch)
2020-08-22 12:14 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-03-30 16:39:19 PDT
<rdar://problem/61079779>
Yusuke Suzuki
Comment 2 2020-06-21 23:30:46 PDT
Created attachment 402447 [details] Patch WIP
Yusuke Suzuki
Comment 3 2020-06-22 00:29:50 PDT
Comment on attachment 402447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402447&action=review > JSTests/test262/expectations.yaml:1643 > + strict mode: 'Test262Error: Result for full with {} Expected SameValue(«14:12:47 PM Coordinated Universal Time», «2:12:47 PM Coordinated Universal Time») to be true' I think, this and the below test requires "udatpg_getDefaultHourCycle" to fix, which is introduced in ICU 67.
Yusuke Suzuki
Comment 4 2020-06-22 00:50:54 PDT
Comment on attachment 402447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402447&action=review >> JSTests/test262/expectations.yaml:1643 >> + strict mode: 'Test262Error: Result for full with {} Expected SameValue(«14:12:47 PM Coordinated Universal Time», «2:12:47 PM Coordinated Universal Time») to be true' > > I think, this and the below test requires "udatpg_getDefaultHourCycle" to fix, which is introduced in ICU 67. Or, I think we should first generate some pattern from generator to determine which is the right default hour-cycle.
Yusuke Suzuki
Comment 5 2020-06-22 01:17:53 PDT
Created attachment 402450 [details] Patch WIP
Yusuke Suzuki
Comment 6 2020-06-22 01:26:55 PDT
Created attachment 402451 [details] Patch WIP
Yusuke Suzuki
Comment 7 2020-06-22 07:37:33 PDT
Created attachment 402469 [details] Patch
Yusuke Suzuki
Comment 8 2020-06-22 07:43:56 PDT
Created attachment 402470 [details] Patch
Yusuke Suzuki
Comment 9 2020-06-22 07:50:22 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:584 > + } Even if `hour` is not specified `timeStyle` can use m_hourCycle. So we do not clear it at this point. Later, we will clear it if hour is null or timeStyle is null.
Darin Adler
Comment 10 2020-06-22 08:01:18 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review I probably should’ve waited to review until the test results from EWS were in. R=me assuming everything passes. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:641 > + auto toUDateFormatStyle = [](const String& style) { We often use the word parse for functions that interpret a string and return a parsed form of it. I think it would be good to name this parse instead of to. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642 > + if (style.isNull()) I don’t think we need to optimize the null case. I suggest leaving out this if statement. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:644 > + if (style == "full"_s) I am not sure that String == ASCIILiteral is properly implemented/optimized. In the past, in fact, sometimes it involved creating and then destroying a String. Could you check on this? > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:680 > + UDateTimePatternGenerator* generator = udatpg_open(dataLocaleWithExtensions.data(), &status); auto? > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:748 > + auto toDateTimeStyle = [](const String& style) { Ditto. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:749 > + if (style.isNull()) Ditto.
Yusuke Suzuki
Comment 11 2020-06-22 10:32:26 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:641 >> + auto toUDateFormatStyle = [](const String& style) { > > We often use the word parse for functions that interpret a string and return a parsed form of it. I think it would be good to name this parse instead of to. Nice. Fixed. >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642 >> + if (style.isNull()) > > I don’t think we need to optimize the null case. I suggest leaving out this if statement. Sounds nice. Done. >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:644 >> + if (style == "full"_s) > > I am not sure that String == ASCIILiteral is properly implemented/optimized. In the past, in fact, sometimes it involved creating and then destroying a String. Could you check on this? Right! I thought the exact same thing while writing this code. So..., I ensured it :D 397 inline bool operator==(const String& a, const char* b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b)); } 398 inline bool operator==(const String& a, ASCIILiteral b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b.characters())); } 407 inline bool operator!=(const String& a, const char* b) { return !equal(a.impl(), reinterpret_cast<const LChar*>(b)); } 408 inline bool operator!=(const String& a, ASCIILiteral b) { return !equal(a.impl(), reinterpret_cast<const LChar*>(b.characters())); } We can use ASCIILiteral :) >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:680 >> + UDateTimePatternGenerator* generator = udatpg_open(dataLocaleWithExtensions.data(), &status); > > auto? Sounds nice. Fixed. >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:748 >> + auto toDateTimeStyle = [](const String& style) { > > Ditto. Fixed. >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:749 >> + if (style.isNull()) > > Ditto. Fixed.
Ross Kirsling
Comment 12 2020-06-22 14:02:21 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:631 > + throwTypeError(globalObject, scope, "dateStyle or timeStyle requires all the date and time formats are not specified"_s); Maybe "dateStyle and timeStyle may not be used with other DateTimeFormat options"? > Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:81 > + enum class DateTimeStyle : uint8_t { None, Full, Long, Medium, Short }; What about just using UDateFormatStyle directly? This is what I did in RelativeTimeFormat (with PluralRules as precedent). > JSTests/test262/expectations.yaml:1641 > +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js: Is this failure due to ICU version? Can you explain in the ChangeLog?
Yusuke Suzuki
Comment 13 2020-06-22 16:16:31 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:631 >> + throwTypeError(globalObject, scope, "dateStyle or timeStyle requires all the date and time formats are not specified"_s); > > Maybe "dateStyle and timeStyle may not be used with other DateTimeFormat options"? Sounds better! Fixed. >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:81 >> + enum class DateTimeStyle : uint8_t { None, Full, Long, Medium, Short }; > > What about just using UDateFormatStyle directly? > This is what I did in RelativeTimeFormat (with PluralRules as precedent). One of the sad thing is that UDateFormatStyle is C enum. This means that it takes `int` size instead of `uint8_t` size, and it can affect on the sizeof(IntlDateTimeFormat). So I would like to stick on the above enum to keep size small :)
Ross Kirsling
Comment 14 2020-06-22 16:18:32 PDT
(In reply to Yusuke Suzuki from comment #13) > One of the sad thing is that UDateFormatStyle is C enum. This means that it > takes `int` size instead of `uint8_t` size, and it can affect on the > sizeof(IntlDateTimeFormat). So I would like to stick on the above enum to > keep size small :) Oh that's a good point. I'd only been thinking about the time cost of converting back and forth.
Yusuke Suzuki
Comment 15 2020-06-22 19:29:27 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review >> JSTests/test262/expectations.yaml:1641 >> +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js: > > Is this failure due to ICU version? Can you explain in the ChangeLog? This is hcDefault's bug. Our IntlDateTimeFormat is not spec compliant yet about hcDefault. We are guessing hcDefault value by parsing the resulted pattern. But I found that this returns incorrect hcDefault value in some cases, and this is one of the case.test/intl402/DateTimeFormat/prototype/resolvedOptions/hourCycle-default.js is also derived from the same hcDefault issue.
Yusuke Suzuki
Comment 16 2020-06-22 19:30:26 PDT
Comment on attachment 402470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review >>> JSTests/test262/expectations.yaml:1641 >>> +test/intl402/DateTimeFormat/prototype/format/timedatestyle-en.js: >> >> Is this failure due to ICU version? Can you explain in the ChangeLog? > > This is hcDefault's bug. Our IntlDateTimeFormat is not spec compliant yet about hcDefault. We are guessing hcDefault value by parsing the resulted pattern. But I found that this returns incorrect hcDefault value in some cases, and this is one of the case.test/intl402/DateTimeFormat/prototype/resolvedOptions/hourCycle-default.js is also derived from the same hcDefault issue. And this FIXME // FIXME: We should cache `hourCycleDefault` value obtained by generating "jjmm" pattern from UDateTimePatternGenerator. // https://bugs.webkit.org/show_bug.cgi?id=213459 is for that issue :)
Yusuke Suzuki
Comment 17 2020-06-22 20:52:09 PDT
Created attachment 402531 [details] Patch
Yusuke Suzuki
Comment 18 2020-08-22 12:14:02 PDT
Created attachment 407055 [details] Patch Patch for landing
EWS
Comment 19 2020-08-22 13:36:11 PDT
Tools/Scripts/svn-apply failed to apply attachment 407055 [details] to trunk. Please resolve the conflicts and upload a new patch.
Yusuke Suzuki
Comment 20 2020-08-22 13:41:25 PDT
Committed r266035: <https://trac.webkit.org/changeset/266035>
Note You need to log in before you can comment on or make changes to this bug.