<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604#c21">Comment # 21</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604">bug 147604</a>
from <span class="vcard"><a class="email" href="mailto:sukolsak@gmail.com" title="Sukolsak Sakshuwong <sukolsak@gmail.com>"> <span class="fn">Sukolsak Sakshuwong</span></a>
</span></b>
<pre>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, <a href="https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/StringPrototype.cpp#L801">https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/StringPrototype.cpp#L801</a> Should I file a bug for that?
(In reply to <a href="show_bug.cgi?id=147604#c19">comment #19</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=267366&action=diff" name="attach_267366" title="Patch">attachment 267366</a> <a href="attachment.cgi?id=267366&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=267366&action=review">https://bugs.webkit.org/attachment.cgi?id=267366&action=review</a>
>
> > 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.</span >
Fixed.
<span class="quote">> > 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.</span >
Changed to append(collation). collation can only be ASCII. See <a href="show_bug.cgi?id=147604#c13">Comment 13</a> (<a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="show_bug.cgi?id=147604#c13">https://bugs.webkit.org/show_bug.cgi?id=147604#c13</a>).
<span class="quote">> > 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.</span >
Fixed.
<span class="quote">> > 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.</span >
Fixed.
<span class="quote">> > Source/JavaScriptCore/runtime/IntlCollator.cpp:252
> > + const HashSet<String>& availableLocales = state.callee()->globalObject()->intlCollatorAvailableLocales();
>
> I like auto& here better for the return type.</span >
Fixed.
<span class="quote">> > 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().</span >
Fixed by using auto and WTF_ARRAY_LENGTH.
<span class="quote">> > 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?</span >
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.
<span class="quote">> > 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.</span >
Fixed.
<span class="quote">> 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.</span >
I think this is the only place that uses this code.
<span class="quote">> > 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?</span >
Changed the message to "Failed to compare strings." The ICU spec doesn't say what can cause an error for these functions.
<span class="quote">> > 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.</span >
Fixed.
<span class="quote">> > 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.</span >
Fixed.
<span class="quote">> > 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.</span >
ASCII, according to <a href="http://userguide.icu-project.org/locale">http://userguide.icu-project.org/locale</a>
<span class="quote">> > 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 "-".</span >
Fixed.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>