Bug 209775 – [ECMA-402] Implement Intl.ListFormat

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Shane Carr]

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

[Radar WebKit Bug Importer]

<rdar://problem/61079787>

[Yusuke Suzuki]

Grepped ICU code with listPatterns and maybe we should use ulistformatter.h

[Yusuke Suzuki]

https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ulistformatter_8h.html

[Yusuke Suzuki]

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]

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]

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]

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]

(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]

Talked with Michael offline, and we agree that we will implement it once ICU ulistfmt_openForType gets stable.

[Yusuke Suzuki]

Created attachment 412234 [details]
Patch

[Yusuke Suzuki]

Created attachment 412238 [details]
Patch

[Yusuke Suzuki]

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]

Created attachment 412239 [details]
Patch

[Ross Kirsling]

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 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]

Committed r268956: <https://trac.webkit.org/changeset/268956>