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+

Description Shane Carr 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
Comment 1 Radar WebKit Bug Importer 2020-03-30 16:39:24 PDT
<rdar://problem/61079787>
Comment 2 Yusuke Suzuki 2020-06-29 20:34:46 PDT
Grepped ICU code with listPatterns and maybe we should use ulistformatter.h
Comment 4 Yusuke Suzuki 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~.
Comment 5 Yusuke Suzuki 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.
Comment 6 Shane Carr 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.
Comment 7 Yusuke Suzuki 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?
Comment 8 Ross Kirsling 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.
Comment 9 Yusuke Suzuki 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.
Comment 10 Yusuke Suzuki 2020-10-23 23:20:14 PDT
Created attachment 412234 [details]
Patch
Comment 11 Yusuke Suzuki 2020-10-24 00:30:11 PDT
Created attachment 412238 [details]
Patch
Comment 12 Yusuke Suzuki 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).
Comment 13 Yusuke Suzuki 2020-10-24 00:47:36 PDT
Created attachment 412239 [details]
Patch
Comment 14 Ross Kirsling 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.
Comment 15 Yusuke Suzuki 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.
Comment 16 Yusuke Suzuki 2020-10-24 23:20:56 PDT
Committed r268956: <https://trac.webkit.org/changeset/268956>