[webkit-reviews] review denied: [Bug 29103] typeAheadFind in select element is case-sensitive for Cyrillic and Greek : [Attachment 41084] a quick fix for SelectElement::typeAheadFind()

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Hironori Bono <hbono at chromium.org>'s
request for review:
Bug 29103: typeAheadFind in select element is case-sensitive for Cyrillic and
Greek
https://bugs.webkit.org/show_bug.cgi?id=29103

Attachment 41084: a quick fix for SelectElement::typeAheadFind()
https://bugs.webkit.org/attachment.cgi?id=41084&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.