[webkit-reviews] review granted: [Bug 209779] [ECMA-402] Implement Intl.DisplayNames : [Attachment 403170] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 209779: [ECMA-402] Implement Intl.DisplayNames
https://bugs.webkit.org/show_bug.cgi?id=209779

Attachment 403170: Patch

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




--- Comment #16 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 403170
  --> https://bugs.webkit.org/attachment.cgi?id=403170
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.)