RESOLVED FIXED 215984
[JSC] Use -2 for grouping options in IntlRelativeTimeFormat 
https://bugs.webkit.org/show_bug.cgi?id=215984
Summary [JSC] Use -2 for grouping options in IntlRelativeTimeFormat
Yusuke Suzuki
Reported 2020-08-30 00:01:00 PDT
[JSC] Use -2 for grouping options in IntlRelativeTimeFormat
Attachments
Patch (3.75 KB, patch)
2020-08-30 00:03 PDT, Yusuke Suzuki 		ross.kirsling: review+
DetailsFormatted DiffDiff
Patch (7.78 KB, patch)
2020-08-30 15:46 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2020-08-30 00:03:08 PDT
Created attachment 407563 [details] Patch
Ross Kirsling
Comment 2 2020-08-30 00:57:44 PDT
Comment on attachment 407563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407563&action=review > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:150 > + // Align to IntlNumberFormat's default. > + unum_setAttribute(m_numberFormat.get(), UNUM_MIN_INTEGER_DIGITS, 1); > + unum_setAttribute(m_numberFormat.get(), UNUM_MIN_FRACTION_DIGITS, 0); > + unum_setAttribute(m_numberFormat.get(), UNUM_MAX_FRACTION_DIGITS, 3); > + unum_setAttribute(m_numberFormat.get(), UNUM_GROUPING_USED, true); Do these need to be set explicitly?
Yusuke Suzuki
Comment 3 2020-08-30 01:04:49 PDT
Comment on attachment 407563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407563&action=review >> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:150 >> + unum_setAttribute(m_numberFormat.get(), UNUM_GROUPING_USED, true); > > Do these need to be set explicitly? Currently, setting these options to align well to the spec (https://tc39.es/ecma402/#sec-InitializeRelativeTimeFormat uses Intl.NumberFormat). I think this is safer, because ICU could change default values.
Ross Kirsling
Comment 4 2020-08-30 01:30:30 PDT
Comment on attachment 407563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407563&action=review > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:162 > + unum_setAttribute(m_numberFormat.get(), UNUM_GROUPING_SIZE, useLocaleDefault); > + unum_setAttribute(m_numberFormat.get(), UNUM_SECONDARY_GROUPING_SIZE, useLocaleDefault); Er wait, one more question. Does that enum value really apply to these two attributes as well? It's not very clear from the linked ICU commit.
Yusuke Suzuki
Comment 5 2020-08-30 01:36:13 PDT
Comment on attachment 407563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407563&action=review >> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:162 >> + unum_setAttribute(m_numberFormat.get(), UNUM_SECONDARY_GROUPING_SIZE, useLocaleDefault); > > Er wait, one more question. Does that enum value really apply to these two attributes as well? It's not very clear from the linked ICU commit. Yes, all three are required. See ICU code for fGrouping1, fGrouping2, and fMinGrouping.
Yusuke Suzuki
Comment 6 2020-08-30 15:46:35 PDT
Created attachment 407576 [details] Patch Patch for landing
EWS
Comment 7 2020-08-30 18:12:46 PDT
ChangeLog entry in JSTests/ChangeLog contains OOPS!.
Yusuke Suzuki
Comment 8 2020-08-30 18:29:17 PDT
Committed r266341: <https://trac.webkit.org/changeset/266341>
Radar WebKit Bug Importer
Comment 9 2020-08-30 18:30:16 PDT
<rdar://problem/68046059>
Note You need to log in before you can comment on or make changes to this bug.