<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#c10">Comment # 10</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. I will fix these. I have some questions.
(In reply to <a href="show_bug.cgi?id=147604#c9">comment #9</a>)
<span class="quote">> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:96
> > + return codePointCompare(x, y);
>
> It is really OK to silently do the wrong thing if ucol_open fails?</span >
I am not sure. I was wondering about that too. Then I checked StringImpl.cpp and found a few methods that do the wrong thing (returning "*this") when an ICU function fails (e.g. u_strToUpper.) So I did a similar thing. Should we do RELEASE_ASSERT(U_SUCCESS(status)) instead? If we should use codePointCompare, is it OK if I implement codePointCompare for StringViews?
<span class="quote">> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:100
> > + const String& sensitivity = collator->sensitivity();
>
> Is this attribute string itself supposed to be case folding or case
> sensitive? This is not a good pattern, doing string comparisons to convert
> the collator’s sensitivity setting into arguments for the ICU collator every
> time we compare a string.</span >
Case sensitive. Should I store an enum class instead and convert it to a string in resolvedOptions()?
<span class="quote">> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:94
> > + UCollator* icuCollator = ucol_open(collator->locale().utf8().data(), &status);
>
> For acceptable performance, I think it’s important that we not use ucol_open
> and ucol_close every time. I tried that in code in WebCore and found that
> everything was much too slow. I think we probably need to put the UCollator
> in the IntlCollator, not create a new one every time.
>
> Doing all that work to initialize the collator attributes every time we
> compare a pair of strings is going to result in very poor performance.
>
> We also need to come up with a fast path so we aren’t converting simple
> strings with all ASCII characters to 16 bit just to call ICU with them.
> Again, we’ve run into this in the past. We need performance tests for this.</span >
Is it OK if I address these in a separate patch?</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>