[webkit-reviews] review granted: [Bug 209774] [ECMA-402] Implement unified Intl.NumberFormat : [Attachment 402953] 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 209774: [ECMA-402] Implement unified Intl.NumberFormat
https://bugs.webkit.org/show_bug.cgi?id=209774

Attachment 402953: Patch

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




--- Comment #17 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 402953
  --> https://bugs.webkit.org/attachment.cgi?id=402953
Patch

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

r=me, this is awesome.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:223
> +    if (iterator != std::end(simpleUnits)) {
> +	   if (!WTF::codePointCompare(iterator->subType().characters(),
iterator->subType().length(), unitIdentifier.characters8(),
unitIdentifier.length()))

&& perhaps?

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:317
> -	   throwTypeError(globalObject, scope, "failed to initialize
NumberFormat due to invalid locale"_s);
> +	   throwTypeError(globalObject, scope, "Failed to initialize
NumberFormat due to invalid locale"_s);

Hmm, it seems like most Intl error messages start with a lowercase letter --
wonder if we should address them en masse in a separate patch?

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:397
> +    } else {
> +	   if (m_style == Style::Unit) {

Seems like we should just make it `else if`.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:640
> +	   case CurrencyDisplay::NarrowSymbol:
> +	       throwTypeError(globalObject, scope, "Failed to initialize
NumberFormat"_s);
> +	       return;

Hmm. I don't object to these messages because "not supported in the ICU version
you're using" isn't appropriate to show to the user, but I wonder if we should
differentiate them from the U_FAILURE cases somehow.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:726
> +    status = U_ZERO_ERROR;

I think you can delete this line?
(U_FAILURE is a `> U_ZERO_ERROR` check.)

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:761
> +    status = U_ZERO_ERROR;

Ditto.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:1042
> +    status = U_ZERO_ERROR;

Ditto x3 in this section.

> Source/JavaScriptCore/runtime/IntlNumberFormatInlines.h:90
> +class IntlFieldIterator {

Fancy!

> Source/WTF/wtf/unicode/icu/ICUHelpers.h:112
> +struct ICUDeleter {

What a good idea. :D

> JSTests/stress/intl-numberformat-unified.js:38
> +    shouldBe((-100).toLocaleString("bn", {

Do you think we should have some more tests for non-English locales (at least,
ones which have more predictable behavior)?

> JSTests/test262/expectations.yaml:1707
> +test/intl402/NumberFormat/prototype/format/engineering-scientific-de-DE.js:

These all require ICU >64, I take it? :(