Intl.ListFormat is at Stage 3 in TC39; it 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 Intl.ListFormat increases throughout the ecosystem, WebKit users will be left behind with legacy polyfills, leading to poorer performance relative to other browsers. At Google, we are currently weighing our options for calling Intl.ListFormat 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 Intl.ListFormat. Implementing Intl.ListFormat 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-list-format
<rdar://problem/61079787>
Grepped ICU code with listPatterns and maybe we should use ulistformatter.h
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ulistformatter_8h.html
Note, 1. ulistformatter.h has almost 1:1 APIs for the Intl.ListFormat, so maybe just piping. 2. formatToParts may be implemented via UFormattedValue, so we should require ICU 64~.
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ulistformatter_8h.html#a7635c9427da25367b659a9369c2a237d > This API may be changed in the future versions and was introduced in ICU 67 Hmm, it seems that this APIs are not stable state right now.
ulistfmt_open is stable since ICU 55, but it only supports wide conjunction ("and"). The C++ API had supported other types for some time, but it was not added to the C API until ICU 67.
We are not using C++ APIs due to strong ABI compatibility requirement to use the system ICU (This is my understanding). We can implement a part of Intl.ListFormat (type "conjunction" case), but it is a bit worrying about whether this breaks feature detections. For example, if the web uses, if (Intl.ListFormat) { ... } and, if the code uses disjunction style, we will have a bad time. I think making ulistfmt_openForType stable API in ICU side is blocking us for now. @Ross what do you think of?
(In reply to Yusuke Suzuki from comment #7) > @Ross what do you think of? Yeah, I agree with you. I suppose we could implement conjunction-only behind a flag but it would feel wrong to ship without disjunction.
Talked with Michael offline, and we agree that we will implement it once ICU ulistfmt_openForType gets stable.
Created attachment 412234 [details] Patch
Created attachment 412238 [details] Patch
Comment on attachment 412238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412238&action=review > Source/JavaScriptCore/runtime/IntlListFormat.h:45 > +struct UListFormatterDeleter { > + JS_EXPORT_PRIVATE void operator()(UListFormatter*); > +}; Do not use ICUDeleter since it requires ulistfmt_close, and we do not want to include <unicode/ulistformatter.h> in headers (since we enable draft APIs).
Created attachment 412239 [details] Patch
Comment on attachment 412239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412239&action=review r=me overall, but I wish we didn't have to scatter the HAVEs quite so much. > Source/JavaScriptCore/runtime/IntlListFormat.cpp:54 > +void UListFormatterDeleter::operator()(UListFormatter* formatter) I suppose you should add your explanation about this as a comment. > Source/JavaScriptCore/runtime/IntlListFormat.cpp:124 > +#if HAVE(ICU_U_LIST_FORMATTER) I asked about this for DisplayNames too (bug 209779 comment 24), but it feels sad to have to #if all of these functions if the ListFormat constructor won't even be exposed in this situation... > Source/JavaScriptCore/runtime/IntlListFormat.cpp:156 > + throwTypeError(globalObject, scope, "Failed to initialize Intl.ListFormat since used feature is not supported in the linked ICU version"_s); nit: s/used/this/ probably reads better > JSTests/stress/intl-listformat.js:22 > +if (typeof Intl.ListFormat !== 'undefined') { Seems like you could just return at the very top, in lieu of a //@ line.
Comment on attachment 412239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412239&action=review Thanks! >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:54 >> +void UListFormatterDeleter::operator()(UListFormatter* formatter) > > I suppose you should add your explanation about this as a comment. Added >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:124 >> +#if HAVE(ICU_U_LIST_FORMATTER) > > I asked about this for DisplayNames too (bug 209779 comment 24), but it feels sad to have to #if all of these functions if the ListFormat constructor won't even be exposed in this situation... I think this way minifies the if-def changes. For example, if we do not expose ListFormat at all, we need to have this ifdef in, VM.cpp IntlListFormatConstructor.h / IntlListFormatConstructor.cpp IntlListFormatPrototype.h / IntlListFormatPrototype.cpp IntlListFormat.h / IntlListFormat.cpp IntlObject.cpp So, we scatters this ifdef in many files. On the other hand, in this way, we need ifdef only in IntlListFormat.cpp and IntlObject.cpp. >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:156 >> + throwTypeError(globalObject, scope, "Failed to initialize Intl.ListFormat since used feature is not supported in the linked ICU version"_s); > > nit: s/used/this/ probably reads better Fixed. >> JSTests/stress/intl-listformat.js:22 >> +if (typeof Intl.ListFormat !== 'undefined') { > > Seems like you could just return at the very top, in lieu of a //@ line. Unfortunately, JS cannot `return` in top-level... I'll extract this to `function test`, and call it when Intl.ListFormat is defined.
Committed r268956: <https://trac.webkit.org/changeset/268956>