Bug 142932 – CSS Selectors: fix attribute case-insensitive matching of Contain and List

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

CSS Selectors: fix attribute case-insensitive matching of Contain and List

[Benjamin Poulain]

Created attachment 249159 [details]
Patch

[Benjamin Poulain]

Created attachment 249203 [details]
Patch

[Darin Adler]

Comment on attachment 249203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249203&action=review

> Source/WTF/wtf/text/StringCommon.h:421
> +template <typename SearchCharacterType, typename MatchCharacterType>
> +size_t findIgnoringASCIICase(const SearchCharacterType* searchCharacters, const MatchCharacterType* matchCharacters, unsigned startOffset, unsigned searchLength, unsigned matchLength)

I think the searchCharacters pointer should be before adding startOffset and this function should add startOffset and we should remove all the "+ startOffset" from the callers.

> Source/WTF/wtf/text/StringCommon.h:424
> +    // delta is the number of additional times to test; delta == 0 means test only once.
> +    unsigned delta = searchLength - matchLength;

Seems we need to assert searchLength >= matchLength.

> Source/WTF/wtf/text/StringCommon.h:433
> +    unsigned i = 0;
> +    // Keep looping until we match.
> +    while (!equalIgnoringASCIICase(searchCharacters + i, matchCharacters, matchLength)) {
> +        if (i == delta)
> +            return notFound;
> +        ++i;
> +    }
> +    return startOffset + i;

I think this should be a for loop:

    unsigned i;
    for (i = 0; !equalIgnoringASCIICase(searchCharacters + i, matchCharacters, matchLength); ++i) {
        if (i == delta)
           return notFound;
    }
    return startOffset + i;

But I think we should write it in a more traditional fashion:

    for (unsigned i = 0; i <= delta; ++i) {
        if (equalIgnoringASCIICase(searchCharacters + i, matchCharacters, matchLength))
            return startOffset + i;
    }
    return notFound;

> Source/WTF/wtf/text/StringView.h:108
> +    WTF_EXPORT_STRING_API size_t findIgnoringASCIICase(const StringView&, unsigned startOffset) const;

If you have a StringView and want to apply a startOffset it’s just view.substring(startOffset) so it’s not really helpful to have this overload.

> Source/WTF/wtf/text/StringView.h:112
> +    WTF_EXPORT_STRING_API bool containsIgnoringASCIICase(const StringView&, unsigned startOffset) const;

If you have a StringView and want to apply a startOffset it’s just view.substring(startOffset) so it’s not really helpful to have this overload.

> Source/WebCore/css/SelectorChecker.cpp:460
> +        bool valueContainsSelectorValue;
> +        if (caseSensitive)
> +            valueContainsSelectorValue = value.contains(selectorValue);
> +        else
> +            valueContainsSelectorValue = value.containsIgnoringASCIICase(selectorValue);
> +
> +        if (!valueContainsSelectorValue || selectorValue.isEmpty())
>              return false;

This is strange code. Why call contains when selectorValue is empty? Why not just return first? Then we avoid the whole local variable thing:

    if (selectorValue.isEmpty())
        return false;
    if (caseSensitive) {
        if (value.contains(selectorValue))
            return false;
    } else {
        if (value.containsIgnoringASCIICase(selectorValue))
            return false;
    }

And in fact, that’s exactly the coding style used below for Begin and End.

[Benjamin Poulain]

(In reply to comment #3)
> > Source/WebCore/css/SelectorChecker.cpp:460
> > +        bool valueContainsSelectorValue;
> > +        if (caseSensitive)
> > +            valueContainsSelectorValue = value.contains(selectorValue);
> > +        else
> > +            valueContainsSelectorValue = value.containsIgnoringASCIICase(selectorValue);
> > +
> > +        if (!valueContainsSelectorValue || selectorValue.isEmpty())
> >              return false;
> 
> This is strange code. Why call contains when selectorValue is empty? Why not
> just return first? Then we avoid the whole local variable thing:

The reason was that selectorValue.isEmpty() never happens in practice.

But you are right, better make this code clean. The CSS JIT does a much better job at making this fast.

[Benjamin Poulain]

Created attachment 249218 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249218 [details]
Patch

Clearing flags on attachment: 249218

Committed r181845: <http://trac.webkit.org/changeset/181845>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.