[webkit-reviews] review requested: [Bug 29103] typeAheadFind in select element is case-sensitive for Cyrillic and Greek : [Attachment 41558] The second quick fix for SelectElement::typeAheadFind()

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  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 41558: The second quick fix for SelectElement::typeAheadFind()
https://bugs.webkit.org/attachment.cgi?id=41558&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
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