I’d love to see formatToParts supported - at the moment we’re having to add a polyfill for Safari (and Edge, although native support is now there in Firefox and Chrome). This lets us use Intl’s numberFormat to produce currency strings with all the global display inconsistencies, but also lets us support target markets in customisations they want to do (e.g. use a custom currency marker or replace the decimals with a dash when it’s a round number).
MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/formatToParts
ECMA 402 issue: https://github.com/tc39/ecma402/issues/30
Attachment 340092[details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340101[details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 340102[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 340108[details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 340110[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 340145[details]
Separate tests for formatToParts, disabled on macOS
Since this feature relies on ICU v59, expect it to fail in macOS for now.
Created attachment 340155[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 340145[details]
Separate tests for formatToParts, disabled on macOS
Attachment 340145[details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7645521
New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-no-cjit
apiTests
Created attachment 340158[details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340176[details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340183[details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340191[details]
Set test expectations properly
View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> + for (const auto &candidate : fields) {
> + if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size()))
> + field = candidate;
> + if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex)
> + nextStartIndex = candidate.beginIndex;
> + }
I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?
> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> + parts->push(&exec, part);
Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
Use putDirectIndex instead.
> Source/JavaScriptCore/runtime/IntlNumberFormat.h:34
> +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59)
You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`.
> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86
> + JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts);
> + putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum));
Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead.
> Source/cmake/WebKitFeatures.cmake:128
> + WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF)
Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake).
Comment on attachment 340191[details]
Set test expectations properly
View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:631
> + auto value = jsString(&exec, resultString.substring(currentIndex, nextIndex - currentIndex));
Use `jsSubstring(VM*, const String&, unsigned offset, unsigned length)`.
(In reply to Yusuke Suzuki from comment #26)
> Comment on attachment 340191[details]
> Set test expectations properly
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340191&action=review
>
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> > + for (const auto &candidate : fields) {
> > + if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size()))
> > + field = candidate;
> > + if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex)
> > + nextStartIndex = candidate.beginIndex;
> > + }
>
> I think if we sort this fields before iterating, we can iteratively accesses
> fields. In that case, time is O(NlogN), correct?
The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.
I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.
>
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> > + parts->push(&exec, part);
>
> Do not use `push()` since it is used for ArrayPush and it is observable to
> users (it uses [[Put]]).
> Use putDirectIndex instead.
Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?
>
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:34
> > +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59)
>
> You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as
> `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`.
That looks much nicer. Will do.
>
> > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86
> > + JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts);
> > + putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum));
>
> Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead.
Will do.
>
> > Source/cmake/WebKitFeatures.cmake:128
> > + WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF)
>
> Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See
> ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake).
Will do.
Comment on attachment 340191[details]
Set test expectations properly
View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review>>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
>>> + }
>>
>> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?
>
> The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.
>
> I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.
Even if they are nested, you can sort an expected order by using beginIndex, and endIndex.
[ A ]
[ B ]
[ C ]
In this case, we can sort [A, B, C] if we use sort fields with beginIndex and then compare endIndex if beginIndex is the same.
Could you add FIXME, bug URL, and note about O(N^2) complexity?
>>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
>>> + parts->push(&exec, part);
>>
>> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
>> Use putDirectIndex instead.
>
> Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?
Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`.
(In reply to Yusuke Suzuki from comment #30)
> Comment on attachment 340191[details]
> Set test expectations properly
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340191&action=review
>
> >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628
> >>> + }
> >>
> >> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct?
> >
> > The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator.
> >
> > I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields.
>
> Even if they are nested, you can sort an expected order by using beginIndex,
> and endIndex.
>
> [ A ]
> [ B ]
> [ C ]
>
> In this case, we can sort [A, B, C] if we use sort fields with beginIndex
> and then compare endIndex if beginIndex is the same.
> Could you add FIXME, bug URL, and note about O(N^2) complexity?
I spent some time trying to rework it to an O(N log N) algorithm, first sorting the fields, and then iterating through them, but I was having a rough time with it, so I'll add the FIXME and revisit another time: https://bugs.webkit.org/show_bug.cgi?id=185557>
> >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635
> >>> + parts->push(&exec, part);
> >>
> >> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]).
> >> Use putDirectIndex instead.
> >
> > Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ?
>
> Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`.
k, I'll fix that.
Created attachment 340337[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Robin Whittleton from comment #46)
> Thanks Andy for all the hard work, that was much quicker than I was
> expecting :)
Happy to help. It's still not enabled on macOS, so it could still take a long while before it ends up in a Safari release.
This bug has been marked as RESOLVED FIXED for months, now, and the fix is already available in Safari Technology Preview. I was really hoping to see it included in Safari with macOS 10.14.4 and iOS 12.2, but it wasn't. Is there any ETA for this? What is holding this back?
2018-05-10 08:58 PDT, Andy VanWagoner
2018-05-10 09:15 PDT, Andy VanWagoner
2018-05-10 10:39 PDT, EWS Watchlist
2018-05-10 10:44 PDT, EWS Watchlist
2018-05-10 11:07 PDT, EWS Watchlist
2018-05-10 11:11 PDT, EWS Watchlist
2018-05-10 16:30 PDT, Andy VanWagoner
2018-05-10 18:28 PDT, EWS Watchlist
2018-05-10 19:20 PDT, EWS Watchlist
2018-05-10 21:00 PDT, Andy VanWagoner
2018-05-11 01:07 PDT, EWS Watchlist
2018-05-11 03:07 PDT, EWS Watchlist
2018-05-11 08:00 PDT, Andy VanWagoner
2018-05-11 10:00 PDT, Andy VanWagoner
2018-05-11 13:29 PDT, Andy VanWagoner
2018-05-14 10:25 PDT, Andy VanWagoner
2018-05-14 11:51 PDT, EWS Watchlist
2018-05-15 10:20 PDT, Andy VanWagoner
2018-05-16 12:58 PDT, Andy VanWagoner