Bug 209775

Summary: [ECMA-402] Implement Intl.ListFormat
Product: WebKit Reporter: Shane Carr <sffc>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: andy, annulen, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, mmaxfield, msaboff, ross.kirsling, ryuan.choi, saam, sergio, 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
ews-feeder: commit-queue-
Patch ross.kirsling: review+

Shane Carr
Reported 2020-03-30 14:43:21 PDT
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
Attachments
Patch (50.11 KB, patch)
2020-10-23 23:20 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (73.88 KB, patch)
2020-10-24 00:30 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (75.88 KB, patch)
2020-10-24 00:47 PDT, Yusuke Suzuki 		ross.kirsling: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-03-30 16:39:24 PDT
<rdar://problem/61079787>
Yusuke Suzuki
Comment 2 2020-06-29 20:34:46 PDT
Grepped ICU code with listPatterns and maybe we should use ulistformatter.h
Yusuke Suzuki
Comment 4 2020-06-29 20:38:16 PDT
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~.
Yusuke Suzuki
Comment 5 2020-06-29 22:24:32 PDT
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.
Shane Carr
Comment 6 2020-06-30 10:52:56 PDT
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.
Yusuke Suzuki
Comment 7 2020-06-30 11:03:51 PDT
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?
Ross Kirsling
Comment 8 2020-06-30 12:57:37 PDT
(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.
Yusuke Suzuki
Comment 9 2020-06-30 13:51:05 PDT
Talked with Michael offline, and we agree that we will implement it once ICU ulistfmt_openForType gets stable.
Yusuke Suzuki
Comment 10 2020-10-23 23:20:14 PDT
Created attachment 412234 [details] Patch
Yusuke Suzuki
Comment 11 2020-10-24 00:30:11 PDT
Created attachment 412238 [details] Patch
Yusuke Suzuki
Comment 12 2020-10-24 00:31:12 PDT
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).
Yusuke Suzuki
Comment 13 2020-10-24 00:47:36 PDT
Created attachment 412239 [details] Patch
Ross Kirsling
Comment 14 2020-10-24 12:41:44 PDT
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.
Yusuke Suzuki
Comment 15 2020-10-24 23:13:24 PDT
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.
Yusuke Suzuki
Comment 16 2020-10-24 23:20:56 PDT
Committed r268956: <https://trac.webkit.org/changeset/268956>
Note You need to log in before you can comment on or make changes to this bug.