Bug 209779 – [ECMA-402] Implement Intl.DisplayNames

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Shane Carr]

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

[Radar WebKit Bug Importer]

<rdar://problem/61079776>

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 403154 [details]
WIP, but it is now working

[Yusuke Suzuki]

Created attachment 403155 [details]
WIP, but it is now working

[Yusuke Suzuki]

Created attachment 403157 [details]
WIP, but it is now working

[Ross Kirsling]

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`?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 403165 [details]
Patch

[Ross Kirsling]

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

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

Created attachment 403170 [details]
Patch

[Ross Kirsling]

(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).

[Yusuke Suzuki]

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.

[Ross Kirsling]

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.)

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 403174 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403221 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403240 [details]
Patch

[Yusuke Suzuki]

Created attachment 403243 [details]
Patch

[Yusuke Suzuki]

Created attachment 403261 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 403274 [details]
Patch for landing

[Ross Kirsling]

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.

[Yusuke Suzuki]

Created attachment 404623 [details]
Rebaselined

[Yusuke Suzuki]

Created attachment 404640 [details]
Rebaselined

[Yusuke Suzuki]

Since this feature is behind the flag, I think I should just land it.

[Yusuke Suzuki]

Committed r264639: <https://trac.webkit.org/changeset/264639>

[Yusuke Suzuki]

Committed r264640: <https://trac.webkit.org/changeset/264640>