[Webkit-unassigned] [Bug 29103] typeAheadFind in select element is case-sensitive for Cyrillic and Greek

[bugzilla-daemon at webkit.org]

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


Hironori Bono <hbono at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41558|                            |review?
               Flag|                            |
  Attachment #41084|0                           |1
        is obsolete|                            |




--- Comment #9 from Hironori Bono <hbono at chromium.org>  2009-10-21 02:16:51 PDT ---
Created an attachment (id=41558)
 --> (https://bugs.webkit.org/attachment.cgi?id=41558)
The second quick fix for SelectElement::typeAheadFind()

Thank you for your review and comments.

> > +    WebCore::String prefix_lower(prefix.lower());
>
> There's no need for an explicit WebCore prefix here -- this code is already
> inside the WebCore namespace. We don't use underscores in variable names in
> WebKit code.

Thank you for noticing this. I have removed the "String" scope.

> > +        // String::startsWith() doesn't convert non-ASCII characters to lowercase since converting a non-ASCII
> > +        // string to lowercase is slow.
>
> I think I steered you wrong here. It's strange that String::startsWith is not
> the same as other functions in StringImpl in this respect and arguable it *is*
> a bug in startsWith.
>
> On the other hand, it seems safer to work around String::startsWith by calling
> lower() here than to change String::startsWith, so I think we can live with
> this as a fix to this bug. We can always revisit later if we fix startsWith.
> And this fix avoids the need for you to stdy all the call sites of startsWith.
>
> Unfortunately, the comment makes it sound like this String::startsWith behavior
> is intentional and a good thing, and I do not think that is true. Really we're
> working around a bug in String::startsWith here, and someone should fix that
> bug later. Or alternatively we should change the design of the other String
> member functions. For many we need ASCII-only versions, not full-Unicode
> versions.

Thank you for noticing this.
I totally misunderstood the comment #4.
I have updated this comment to "to be removed when startWith() supports
non-ascii characters".

> > +        // So, we convert each option string (which may contain non-ASCII characters) to lowercase before calling
> > +        // String::startsWith().
> >          String text = optionElement->textIndentedToRespectGroupLabel();
> > -        if (stripLeadingWhiteSpace(text).startsWith(prefix, false)) {
> > +        if (stripLeadingWhiteSpace(text).lower().startsWith(prefix_lower)) {
>
> The correct function to use in both cases above is actually String::foldCase(),
> not String::lower().

Thank you. I forgot String::foldCase() and agree it is better than
String::lower().
I changed String::lower() with String::foldCase().

Regards,

Hironori Bono

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.