Summary: | [ECMA-402] Implement Intl.DateTimeFormat.prototype.formatRange | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shane Carr <sffc> | ||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | andy, ews-watchlist, 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 | ||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=213822 | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 213425 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Shane Carr
2020-03-30 14:50:13 PDT
Use UDateIntervalFormat to implement this. https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/udateintervalformat_8h.html formatRangeToParts requires ICU API (udtitvfmt_formatToResult ) which is added in 67 and it is draft state. Possibly we will skip it. Created attachment 403255 [details]
Patch
WIP
Created attachment 403260 [details]
Patch
Comment on attachment 403260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403260&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:649 > + dataLogLnIf(IntlDateTimeFormatInternal::verbose, "locale:(", m_locale, "),dataLocale:(", dataLocaleWithExtensions, "),pattern:(", pattern, ")"); I feel kind of unclear about when these verbose logs should actually be landed, but I don't object if you think it'll continue to be useful. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1054 > + // http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-partitiondatetimerangepattern Should this be at the top of the function for consistency? > JSTests/ChangeLog:13 > + * stress/intl-datetimeformat-formatrange-relevant-extensions.js: Added. Seems a little arbitrary that this is split out into a separate file, but glad to see we've tested various options and extensions. I think we need to cover a non-English locale too though. > JSTests/test262/expectations.yaml:1612 > + default: 'Test262Error: no args Expected a RangeError but got a TypeError' Is this one really related to ICU version? We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`. Comment on attachment 403260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403260&action=review >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:649 >> + dataLogLnIf(IntlDateTimeFormatInternal::verbose, "locale:(", m_locale, "),dataLocale:(", dataLocaleWithExtensions, "),pattern:(", pattern, ")"); > > I feel kind of unclear about when these verbose logs should actually be landed, but I don't object if you think it'll continue to be useful. Yeah, this is very useful to check what locale is passed and what pattern is generated, since this is the input of ICU. When I see some problems, I check this first. If this is reasonable, then we need to look into ICU. If these inputs looks non-reasonable, then our frontend to ICU is not correct :) >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:1054 >> + // http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-partitiondatetimerangepattern > > Should this be at the top of the function for consistency? I think here is better since this URL is not for formatRange, this is for PartitionDateTimeRangePattern abstract operation called by formatRange. >> JSTests/ChangeLog:13 >> + * stress/intl-datetimeformat-formatrange-relevant-extensions.js: Added. > > Seems a little arbitrary that this is split out into a separate file, but glad to see we've tested various options and extensions. > I think we need to cover a non-English locale too though. Right. I'll add some more. >> JSTests/test262/expectations.yaml:1612 >> + default: 'Test262Error: no args Expected a RangeError but got a TypeError' > > Is this one really related to ICU version? > We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`. Oops, right!!! Fixed. Comment on attachment 403260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403260&action=review >>> JSTests/test262/expectations.yaml:1612 >>> + default: 'Test262Error: no args Expected a RangeError but got a TypeError' >> >> Is this one really related to ICU version? >> We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`. > > Oops, right!!! Fixed. Oh, interesting. This is test262's bug. http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-intl.datetimeformat.prototype.formatRange "If startDate is undefined or endDate is undefined, throw a TypeError exception." 20 var dtf = new Intl.DateTimeFormat(); 21 22 assert.throws(RangeError, function() { 23 dtf.formatRange(); // Not possible to poison this one 24 }, "no args"); Created attachment 403266 [details]
Patch
WIP
Created attachment 403271 [details]
Patch
Comment on attachment 403260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403260&action=review >>>> JSTests/test262/expectations.yaml:1612 >>>> + default: 'Test262Error: no args Expected a RangeError but got a TypeError' >>> >>> Is this one really related to ICU version? >>> We're throwing the wrong type of error for `new Intl.DateTimeFormat().formatRange()`. >> >> Oops, right!!! Fixed. > > Oh, interesting. This is test262's bug. > http://tc39.es/proposal-intl-DateTimeFormat-formatRange/#sec-intl.datetimeformat.prototype.formatRange > "If startDate is undefined or endDate is undefined, throw a TypeError exception." > > 20 var dtf = new Intl.DateTimeFormat(); > 21 > 22 assert.throws(RangeError, function() { > 23 dtf.formatRange(); // Not possible to poison this one > 24 }, "no args"); Uploaded the PR https://github.com/tc39/test262/pull/2685 Created attachment 403272 [details]
Patch
Comment on attachment 403272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403272&action=review r=me. > JSTests/test262/expectations.yaml:1617 > +test/intl402/DateTimeFormat/prototype/formatRangeToParts/argument-date-string.js: Assuming we're not passing any tests in this directory, we should just add a path skip to config.yaml (with a comment linking to bug 213822). Comment on attachment 403272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403272&action=review >> JSTests/test262/expectations.yaml:1617 >> +test/intl402/DateTimeFormat/prototype/formatRangeToParts/argument-date-string.js: > > Assuming we're not passing any tests in this directory, we should just add a path skip to config.yaml (with a comment linking to bug 213822). Thats nice! Created attachment 403275 [details]
Patch for landing
Created attachment 403300 [details]
Patch for landing
Created attachment 403304 [details]
Patch for landing
Created attachment 403317 [details]
Patch for landing
Created attachment 403319 [details]
Patch for landing
Created attachment 403324 [details]
Patch for landing
Committed r266033: <https://trac.webkit.org/changeset/266033> |