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

[bugzilla-daemon at webkit.org]

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

--- Comment #10 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks for the review. I will fix these. I have some questions.

(In reply to comment #9)
> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:96
> > +        return codePointCompare(x, y);
> 
> It is really OK to silently do the wrong thing if ucol_open fails?

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?

> > 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.

Case sensitive. Should I store an enum class instead and convert it to a string in resolvedOptions()?

> > 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.

Is it OK if I address these in a separate patch?

-- 
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/20151103/82a1daf3/attachment.html>