[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 bugzilla-daemon at webkit.org
Wed Oct 21 02:16:51 PDT 2009

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 29103: typeAheadFind in select element is case-sensitive for Cyrillic and

Attachment 41558: The second quick fix for SelectElement::typeAheadFind()

------- 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
> a bug in startsWith.
> On the other hand, it seems safer to work around String::startsWith by
> 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
> Unfortunately, the comment makes it sound like this String::startsWith
> is intentional and a good thing, and I do not think that is true. Really
> 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
> not String::lower().

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


Hironori Bono

More information about the webkit-reviews mailing list