[Webkit-unassigned] [Bug 147604] [INTL] Implement Collator Compare Functions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147604

--- Comment #21 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks for the review.

Regarding the string object lifetime bug, it looks like there are a few places in StringPrototype.cpp that do that. For example, https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/StringPrototype.cpp#L801 Should I file a bug for that?

(In reply to comment #19)
> Comment on attachment 267366 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267366&action=review
> 
> > Source/JavaScriptCore/runtime/IntlCollator.cpp:104
> > +        keyLocaleData.append(String());
> 
> Alternate syntax for null in this context is { } rather than String(). I
> slightly prefer the former.

Fixed.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:125
> > +                keyLocaleData.append(String(collation));
> 
> This treats the keyword value strings as ASCII or Latin-1, not UTF-8. Is
> that correct? Also, an explicit conversion to String() is not needed; please
> just write append(collation).
> 
> To create the strings as UTF-8, we would use String::fromUTF8(collation) but
> we also have to consider that the string might be invalid UTF-8 which would
> result in a null string.

Changed to append(collation). collation can only be ASCII. See Comment 13 (https://bugs.webkit.org/show_bug.cgi?id=147604#c13).

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:170
> > +    Vector<String> requestedLocales = canonicalizeLocaleList(state, locales);
> 
> I like to use auto for the return value type in a case like this.

Fixed.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:220
> > +    opt.set(ASCIILiteral("localeMatcher"), matcher);
> 
> More efficient to use the add function here rather than the set function.
> 
> > Source/JavaScriptCore/runtime/IntlCollator.cpp:241
> > +        opt.set(ASCIILiteral("kn"), numericString);
> 
> Ditto.
> 
> > Source/JavaScriptCore/runtime/IntlCollator.cpp:247
> > +        opt.set(ASCIILiteral("kf"), caseFirst);
> 
> Ditto.

Fixed.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:252
> > +    const HashSet<String>& availableLocales = state.callee()->globalObject()->intlCollatorAvailableLocales();
> 
> I like auto& here better for the return type.

Fixed.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:253
> > +    HashMap<String, String> result = resolveLocale(availableLocales, requestedLocales, opt, relevantExtensionKeys, sizeof(relevantExtensionKeys) / sizeof(relevantExtensionKeys[0]), localeData);
> 
> I like auto here better for the return type.
> 
> WTF_ARRAY_LENGTH is preferred over that sizeof expression. Or perhaps we can
> use std::array and use relevantExtensionKeys.size().

Fixed by using auto and WTF_ARRAY_LENGTH.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:329
> > +        ASSERT(!state.hadException());
> 
> Why can we assert this? What guarantees it is true. If we can assert here,
> why can’t we assert along the way doing each step inside initializeCollator
> itself?

The spec guarantees that it is true. According to the steps in ECMA-402, 10.1.1, InitializeCollator (collator, locales, options) will not throw if locales and options are undefined.

Another reason is that the spec says that "The Intl.Collator prototype object is itself an Intl.Collator instance ... whose internal slots are set as if it had been constructed by the expression Construct(%Collator%)". If InitializeCollator(collator, undefined, undefined) could throw, then Intl.Collator.prototype could throw when it was created, which doesn't make sense.

We can't assert inside initializeCollator because locales and options are not always undefined.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:332
> > +    if (!m_collator) {
> 
> I suggest putting the code to allocate a new collator into a separate
> function. So the code here would be:
> 
>     if (!m_collator) {
>         createCollator();
>         if (!m_collator)
>             return state.vm().throwException(&state, createError(&state,
> ASCIILiteral("error message here")));
>     }
> 
> The code to call initializeCollator could also possibly go into the
> createCollator function for the same reason.
> 
> I think this would make this function easier to read and possible even more
> efficient.

Fixed.

> Is there any other place we can share this code with? It’s pretty long
> winded and I seem to have seen code like this before.

I think this is the only place that uses this code.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:384
> > +        return state.vm().throwException(&state, createError(&state, ASCIILiteral("ucol_setAttribute failed.")));
> 
> This does not seem like an acceptable string for an error seen by the web.
> When would this exception occur? Why is it useful to give the function name
> ucol_setAttribute to the web developer?

Changed the message to "Failed to compare strings." The ICU spec doesn't say what can cause an error for these functions.

> > Source/JavaScriptCore/runtime/IntlCollator.h:50
> > +    JSValue compareStrings(ExecState&, StringView x, StringView y);
> 
> I don’t think the names “x” and “y” here add anything. Please omit them.
> 
> > Source/JavaScriptCore/runtime/IntlCollator.h:67
> > +    enum class Usage {
> > +        Sort,
> > +        Search
> > +    };
> 
> I would find this more readable as a one-liner.
> 
> > Source/JavaScriptCore/runtime/IntlCollator.h:74
> > +    enum class Sensitivity {
> > +        Base,
> > +        Accent,
> > +        Case,
> > +        Variant
> > +    };
> 
> I would find this more readable as a one-liner.

Fixed.

> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:98
> > +    StringView x = state->argument(0).toString(state)->view(state);
> 
> This won’t work. We are creating a StringView pointing to the result of
> toString, but then the garbage collector is allowed to destroy that object.
> Instead the local variable needs to be of type JSString* and the call to
> view needs to be separate. I suggest calling view in the line that calls
> compareStrings.
> 
> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:104
> > +    StringView y = state->argument(1).toString(state)->view(state);
> 
> Ditto.

Fixed.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:118
> > +    String locale(uloc_getDefault());
> 
> Is the result of uloc_getDefault guaranteed to be ASCII or Latin-1? Or could
> it be UTF-8? If it’s UTF-8 then we need to write this code differently.

ASCII, according to http://userguide.icu-project.org/locale

> > Source/JavaScriptCore/runtime/IntlObject.cpp:202
> >              state.vm().throwException(&state, createRangeError(&state, String(notFound)));
> 
> This explicit conversion to String should not be needed. Please remove it.
> 
> > Source/JavaScriptCore/runtime/IntlObject.cpp:211
> > -    return fallback;
> > +    return String(fallback);
> 
> This explicit conversion to String should not be needed. Please don’t make
> this change.
> 
> > Source/JavaScriptCore/runtime/IntlObject.cpp:759
> > -                        supportedExtensionAddition = "-" + key + '-' + value;
> > +                        supportedExtensionAddition = String("-") + key + '-' + value;
> 
> More efficient idiom for this is:
> 
>     supportedExtensionAddition = makeString('-', key, '-', value);
> 
> Avoids extra work to allocate and destroy a single character string for "-".

Fixed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151215/e9b610e9/attachment-0001.html>