[webkit-reviews] review granted: [Bug 209772] [ECMA-402] Implement Intl.Locale : [Attachment 398303] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 209772: [ECMA-402] Implement Intl.Locale
https://bugs.webkit.org/show_bug.cgi?id=209772

Attachment 398303: Patch

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 398303
  --> https://bugs.webkit.org/attachment.cgi?id=398303
Patch

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

Looks generally great. I have some class design thoughts.

> Source/JavaScriptCore/runtime/IntlLocale.cpp:85
> +    UErrorCode status = U_ZERO_ERROR;
> +    Vector<char, 32> buffer(32);
> +    int32_t parsedLength;
> +    auto bufferLength = uloc_forLanguageTag(input.data(), buffer.data(),
buffer.size(), &parsedLength, &status);
> +    if (needsToGrowToProduceBuffer(status)) {
> +	   buffer.resize(bufferLength);
> +	   status = U_ZERO_ERROR;
> +	   uloc_forLanguageTag(input.data(), buffer.data(), bufferLength,
&parsedLength, &status);
> +    }
> +    if (U_FAILURE(status) || parsedLength !=
static_cast<int32_t>(input.length()))
> +	   return false;

I am not OK with us continuing to write these out individually. I am going to
come up with a template/pattern for these so we have a better chance of doing
it right.

> Source/JavaScriptCore/runtime/IntlLocale.cpp:475
> +	   m_numeric = triState(keyword("colnumeric"_s).startsWith("true"_s));

I looked at the specification and did not know how to find the string
"colnumeric" or the rule about "true" as a prefix. Surprised that a "starts
with" check is correct here.

Checking that something starts with an ASCII literal (case sensitive) is not
well optimized, perhaps because it’s not very common. Surprised that it is the
right thing here.

>From a micro-optimization point of view, annoying that this creates/destroys a
WTF::String containing the characters "true"; a malloc/free pair. Neither ==
nor startsWithLettersIgnoringASCIICase does that. We just don’t have a
startsWith overload that takes a literal. If we don’t want to add new overloads
for WTF::String::startsWith, to avoid allocating memory we could use
StringView:

    StringView { keyword("colnumeric"_s) }.startsWith("true")

That would be more efficient. Also, ironically, the free function startsWith
from StringCommon.h is better optimized, although it doesn’t work with
literals:

    startsWith(keyword("colnumeric"_s), StringView { "true "})

Both of the above would be more efficient than String::startsWith. Further, the
StringCommon.h functions could be fixed to work with ASCII literals by adding
member functions named length(), is8Bit(), characters8(), and characters16() to
ASCIILiteral or by some other kinds of refactoring.

> Source/JavaScriptCore/runtime/IntlLocale.h:76
> +    void setKeyword(ASCIILiteral, const String&);

I think this should be called setKeywordValue. I think this should take a
StringView. I know that currently String has a function to produce UTF-8
characters and StringView does not, but we should fix that.

> Source/JavaScriptCore/runtime/IntlLocale.h:77
> +    String keyword(ASCIILiteral) const;

I think this should be called keywordValue.

> Source/JavaScriptCore/runtime/IntlLocale.h:80
> +    CString m_localeID;

CString seems fine/great for the locale ID after it’s computed. While it’s
being computed I don’t think CString is optimal. Vector<char> would probably be
better, probably even with inline capacity.

I think it’s a mistake that IntlLocale object is both the "locale ID builder"
and the thing to use after it’s built. I suggest a separate object for use as
the locale ID builder, that gets created and destroyed within the
initializeLocaleFunction. Many of the private member functions would be part of
that, not part of IntlLocale.