[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41084|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2009-10-13 14:14:08 PDT ---
(From update of attachment 41084)
Thanks for keeping at it!

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

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

> +        // 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().

review- because of the style mistakes.

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