Bug 209779 - [ECMA-402] Implement Intl.DisplayNames
Summary: [ECMA-402] Implement Intl.DisplayNames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 213425
  Show dependency treegraph
 
Reported: 2020-03-30 14:52 PDT by Shane Carr
Modified: 2020-08-10 11:12 PDT (History)
21 users (show)

See Also:


Attachments
WIP, but it is now working (74.89 KB, patch)
2020-06-29 18:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP, but it is now working (74.93 KB, patch)
2020-06-29 18:37 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP, but it is now working (76.83 KB, patch)
2020-06-29 18:42 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (85.56 KB, patch)
2020-06-29 20:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (86.11 KB, patch)
2020-06-29 21:14 PDT, Yusuke Suzuki
ross.kirsling: review+
Details | Formatted Diff | Diff
Patch for landing (86.37 KB, patch)
2020-06-29 22:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (87.08 KB, patch)
2020-06-30 12:11 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (87.10 KB, patch)
2020-06-30 13:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (88.58 KB, patch)
2020-06-30 14:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (88.68 KB, patch)
2020-06-30 21:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (89.08 KB, patch)
2020-06-30 23:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Rebaselined (89.31 KB, patch)
2020-07-17 17:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Rebaselined (89.23 KB, patch)
2020-07-18 04:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shane Carr 2020-03-30 14:52:00 PDT
Intl.DisplayNames is at Stage 3 in TC39; it is on track to reach Stage 4 in 2020.  V8 has had it ready to ship for some time, and SpiderMonkey plans to implement it soon.

As usage of Intl.DisplayNames 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.DisplayNames 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.DisplayNames.  Implementing Intl.DisplayNames 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-displaynames
Comment 1 Radar WebKit Bug Importer 2020-03-30 16:39:18 PDT
<rdar://problem/61079776>
Comment 2 Yusuke Suzuki 2020-06-29 13:02:11 PDT
Ugh, DisplayNames tests in test262 have many bugs...
While the draft says that we should throw TypeError if option.type is not defined, test262 is not following to that, as a result, almost all tests are failing with TypeError :(
Comment 3 Yusuke Suzuki 2020-06-29 13:04:45 PDT
It seems that the current draft lacks the description about Dialect names v.s. Standard names.
As a result, SpiderMonkey and V8 prototypes do not agree the results for language types.

https://github.com/tc39/proposal-intl-displaynames/issues/20
Comment 4 Yusuke Suzuki 2020-06-29 14:01:13 PDT
(In reply to Yusuke Suzuki from comment #2)
> Ugh, DisplayNames tests in test262 have many bugs...
> While the draft says that we should throw TypeError if option.type is not
> defined, test262 is not following to that, as a result, almost all tests are
> failing with TypeError :(

Filed https://github.com/tc39/test262/issues/2682.
I guess we should have "language" fallback for "type" option.
Comment 5 Yusuke Suzuki 2020-06-29 18:35:26 PDT
Created attachment 403154 [details]
WIP, but it is now working
Comment 6 Yusuke Suzuki 2020-06-29 18:37:30 PDT
Created attachment 403155 [details]
WIP, but it is now working
Comment 7 Yusuke Suzuki 2020-06-29 18:42:54 PDT
Created attachment 403157 [details]
WIP, but it is now working
Comment 8 Ross Kirsling 2020-06-29 19:44:05 PDT
Comment on attachment 403157 [details]
WIP, but it is now working

View in context: https://bugs.webkit.org/attachment.cgi?id=403157&action=review

Just a few early comments.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:67
> +Vector<String> IntlDisplayNames::localeData(const String&, size_t)
> +{
> +    return { };
> +}

I suppose you could write this inline? Unless you feel it's clearer this way.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:117
> +    if (typeString.isNull())
> +        throwTypeError(globalObject, scope, "type must be a string"_s);

I think this is strictly "must not be undefined", right?
(We'd've already thrown if it's not string-coercible.)

> Source/JavaScriptCore/runtime/IntlLocale.cpp:216
> +static bool isUnicodeVariantSubtag(StringView string)

Any reason for leaving this one `static`?
Comment 9 Yusuke Suzuki 2020-06-29 20:05:14 PDT
Comment on attachment 403157 [details]
WIP, but it is now working

View in context: https://bugs.webkit.org/attachment.cgi?id=403157&action=review

Thanks!

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:67
>> +}
> 
> I suppose you could write this inline? Unless you feel it's clearer this way.

Sounds good. maybe, we should just pass lambda.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:117
>> +        throwTypeError(globalObject, scope, "type must be a string"_s);
> 
> I think this is strictly "must not be undefined", right?
> (We'd've already thrown if it's not string-coercible.)

Right. Changed

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:216
>> +static bool isUnicodeVariantSubtag(StringView string)
> 
> Any reason for leaving this one `static`?

Since it is not used outside of IntlLocale, but exposing it would be nice. Fixed.
Comment 10 Yusuke Suzuki 2020-06-29 20:10:08 PDT
Created attachment 403165 [details]
Patch
Comment 11 Ross Kirsling 2020-06-29 20:19:11 PDT
(In reply to Yusuke Suzuki from comment #9)
> >> Source/JavaScriptCore/runtime/IntlLocale.cpp:216
> >> +static bool isUnicodeVariantSubtag(StringView string)
> > 
> > Any reason for leaving this one `static`?
> 
> Since it is not used outside of IntlLocale, but exposing it would be nice.
> Fixed.

Oh oops, I hadn't noticed that these were in IntlLocale and not IntlObject! I had them as static here because they weren't used elsewhere, but I figured we'd move them to IntlObject if they became needed, alongside isUnicodeLocaleIdentifierType.
Comment 12 Yusuke Suzuki 2020-06-29 20:20:12 PDT
Comment on attachment 403165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403165&action=review

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:148
> +        UDISPCTX_CAPITALIZATION_NONE,

It is possible that we should use UDISPCTX_CAPITALIZATION_FOR_STANDALONE instead of UDISPCTX_CAPITALIZATION_NONE.
ICU test cases show that UDISPCTX_CAPITALIZATION_NONE is effective if the display-name is date value (like, "Juillet 2008" v.s. "juillet 2008") while currently we are not supporting date values in DisplayNames.
What do you think of?
Comment 13 Yusuke Suzuki 2020-06-29 21:14:19 PDT
Created attachment 403170 [details]
Patch
Comment 14 Ross Kirsling 2020-06-29 21:22:49 PDT
(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 403165 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403165&action=review
> 
> > Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:148
> > +        UDISPCTX_CAPITALIZATION_NONE,
> 
> It is possible that we should use UDISPCTX_CAPITALIZATION_FOR_STANDALONE
> instead of UDISPCTX_CAPITALIZATION_NONE.
> ICU test cases show that UDISPCTX_CAPITALIZATION_NONE is effective if the
> display-name is date value (like, "Juillet 2008" v.s. "juillet 2008") while
> currently we are not supporting date values in DisplayNames.
> What do you think of?

RelativeTimeFormat uses UDISPCTX_CAPITALIZATION_FOR_STANDALONE, but there too, the difference was unclear (and I think V8 and SM didn't agree either).
Comment 15 Yusuke Suzuki 2020-06-29 21:24:17 PDT
Comment on attachment 403165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403165&action=review

>>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:148
>>> +        UDISPCTX_CAPITALIZATION_NONE,
>> 
>> It is possible that we should use UDISPCTX_CAPITALIZATION_FOR_STANDALONE instead of UDISPCTX_CAPITALIZATION_NONE.
>> ICU test cases show that UDISPCTX_CAPITALIZATION_NONE is effective if the display-name is date value (like, "Juillet 2008" v.s. "juillet 2008") while currently we are not supporting date values in DisplayNames.
>> What do you think of?
> 
> RelativeTimeFormat uses UDISPCTX_CAPITALIZATION_FOR_STANDALONE, but there too, the difference was unclear (and I think V8 and SM didn't agree either).

I think, given that,

1. UDISPCTX_CAPITALIZATION_FOR_STANDALONE makes date format better ("Juillet 2008" case)
2. Intl.DisplayFormat will support date related types in the future (dayPeriod etc.)

maybe we should use UDISPCTX_CAPITALIZATION_FOR_STANDALONE here.
Comment 16 Ross Kirsling 2020-06-29 21:28:56 PDT
Comment on attachment 403170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403170&action=review

r=me with a bunch of nitpicks.

I feel sad about not being able to flip the test262 flag, since it usually reveals some easily overlooked edge cases -- were you able to confirm behavior with the cases that aren't in need of fixing? Either way, it's still at Stage 3, so there's time for us to help polish the details. :)

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:117
> +        throwTypeError(globalObject, scope, "type must not be an undefined"_s);

nit: s/an //

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:153
> +        // Always disable ICU SUBSTITUTE since it does not match against what the spec defines. ICU has some special substitute rules, for exapmle, language "en-AA"

spelling: "exapmle"

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:185
> +        // 5. If ! IsWellFormedCurrencyCode(code) is false, throw a RangeError exceptipon.

Comically, this spec section consistently uses the misspelling "exceptipon". :P

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:216
> +            return throwTypeError(globalObject, scope, "Failed to query a display names."_s);

nit: s/names/name/

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:222
> +        // https://tc39.es/proposal-intl-displaynames/#sec-Intl.DisplayNames.prototype.of

I think this particular link should be at the top of the function.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:268
> +        ASSERT(code.isAllASCII());

Doesn't seem like you need to assert this outside canonicalizeCodeForDisplayNames since you're asserting it inside, immediately before use.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:281
> +        ASSERT(code.isAllASCII());

Ditto.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:293
> +        ASSERT(code.isAllASCII());

Ditto.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:305
> +        // https://tc39.es/proposal-intl-displaynames/#sec-Intl.DisplayNames.prototype.of

I think this particular link should be at the top of the function.

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:308
> +        return throwTypeError(globalObject, scope, "Failed to query a display names."_s);

nit: s/names/name/

> Source/JavaScriptCore/runtime/IntlDisplayNames.h:76
> +    String m_locale;
> +    CString m_localeCString;

Is it really best to store both?
(Maybe it is; I'm just surprised.)

> Source/JavaScriptCore/runtime/IntlLocale.cpp:227
> +bool isUnicodeLanguageId(StringView string)

We should probably check about "root" and "_" separator support, since V8 and SM are in (possibly intentional) conflict with UTS35 here.
(Not necessarily a blocker for this patch.)
Comment 17 Yusuke Suzuki 2020-06-29 21:42:08 PDT
Comment on attachment 403170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403170&action=review

Thanks!

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:117
>> +        throwTypeError(globalObject, scope, "type must not be an undefined"_s);
> 
> nit: s/an //

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:153
>> +        // Always disable ICU SUBSTITUTE since it does not match against what the spec defines. ICU has some special substitute rules, for exapmle, language "en-AA"
> 
> spelling: "exapmle"

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:185
>> +        // 5. If ! IsWellFormedCurrencyCode(code) is false, throw a RangeError exceptipon.
> 
> Comically, this spec section consistently uses the misspelling "exceptipon". :P

Oh, interesting!

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:216
>> +            return throwTypeError(globalObject, scope, "Failed to query a display names."_s);
> 
> nit: s/names/name/

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:222
>> +        // https://tc39.es/proposal-intl-displaynames/#sec-Intl.DisplayNames.prototype.of
> 
> I think this particular link should be at the top of the function.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:268
>> +        ASSERT(code.isAllASCII());
> 
> Doesn't seem like you need to assert this outside canonicalizeCodeForDisplayNames since you're asserting it inside, immediately before use.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:281
>> +        ASSERT(code.isAllASCII());
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:293
>> +        ASSERT(code.isAllASCII());
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:305
>> +        // https://tc39.es/proposal-intl-displaynames/#sec-Intl.DisplayNames.prototype.of
> 
> I think this particular link should be at the top of the function.

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:308
>> +        return throwTypeError(globalObject, scope, "Failed to query a display names."_s);
> 
> nit: s/names/name/

Fixed.

>> Source/JavaScriptCore/runtime/IntlDisplayNames.h:76
>> +    CString m_localeCString;
> 
> Is it really best to store both?
> (Maybe it is; I'm just surprised.)

Currently I'm having this since ucurr_getName requires it every time. It would be possible that we can remove storing this if the type is not currency. For now, I've put FIXME for the further optimizations.

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:227
>> +bool isUnicodeLanguageId(StringView string)
> 
> We should probably check about "root" and "_" separator support, since V8 and SM are in (possibly intentional) conflict with UTS35 here.
> (Not necessarily a blocker for this patch.)

Sounds nice!
Comment 18 Yusuke Suzuki 2020-06-29 22:07:34 PDT
Created attachment 403174 [details]
Patch for landing
Comment 19 Yusuke Suzuki 2020-06-30 12:11:29 PDT
Created attachment 403221 [details]
Patch for landing
Comment 20 Yusuke Suzuki 2020-06-30 13:48:57 PDT
Created attachment 403240 [details]
Patch
Comment 21 Yusuke Suzuki 2020-06-30 14:41:00 PDT
Created attachment 403243 [details]
Patch
Comment 22 Yusuke Suzuki 2020-06-30 21:43:22 PDT
Created attachment 403261 [details]
Patch for landing
Comment 23 Yusuke Suzuki 2020-06-30 23:40:41 PDT
Created attachment 403274 [details]
Patch for landing
Comment 24 Ross Kirsling 2020-07-01 18:47:28 PDT
Comment on attachment 403274 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=403274&action=review

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:138
> +#if HAVE(ICU_U_LOCALE_DISPLAY_NAMES)

Do we really need an #if here?

> Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:180
> +#if HAVE(ICU_U_LOCALE_DISPLAY_NAMES)

This one seems more reasonable, though it still feels a bit odd to have the #elsif be a TypeError path if it's unreachable code.

> Source/JavaScriptCore/runtime/IntlObject.cpp:169
> +    UNUSED_PARAM(createDisplayNamesConstructor);

Probably better to #if the function itself? Otherwise there's UNUSED_VARIABLE too.
Comment 25 Yusuke Suzuki 2020-07-17 17:19:23 PDT
Created attachment 404623 [details]
Rebaselined
Comment 26 Yusuke Suzuki 2020-07-18 04:14:29 PDT
Created attachment 404640 [details]
Rebaselined
Comment 27 Yusuke Suzuki 2020-07-20 15:48:19 PDT
Since this feature is behind the flag, I think I should just land it.
Comment 28 Yusuke Suzuki 2020-07-20 17:04:16 PDT
Committed r264639: <https://trac.webkit.org/changeset/264639>
Comment 29 Yusuke Suzuki 2020-07-20 17:11:21 PDT
Committed r264640: <https://trac.webkit.org/changeset/264640>