Bug 129580 – Add a fallback path for compiling the remaining attribute checkers

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

[Benjamin Poulain]

Add a fallback path for compiling the remaining attribute checkers

[Benjamin Poulain]

Created attachment 225607 [details]
Patch

[Darin Adler]

Comment on attachment 225607 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.cpp:282
>      switch (match) {

Could we replace all the "break;" here with return true? Then we could remove the default case and move the ASSERT_NOT_REACHED out of the switch and get a warning if someone adds a new case we don’t handle here.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1241
> +    bool valueStartsWithExpectedString;
> +    if (caseSensitivity == CaseSensitive)
> +        valueStartsWithExpectedString = valueImpl.startsWith(expectedString);
> +    else
> +        valueStartsWithExpectedString = valueImpl.startsWith(expectedString, false);
> +
> +    if (!valueStartsWithExpectedString)
> +        return false;

Single line version of these 7 lines of code:

    if (!valueImpl.startsWith(expectedString, caseSensitivity == CaseSensitive))

> Source/WebCore/cssjit/SelectorCompiler.cpp:1261
> +        if (!foundPos || value[foundPos - 1] == ' ') {
> +            unsigned endStr = foundPos + expectedString->length();

foundPos and endStr? Not only are these abbreviated, but they are using different abbreviations even though their purposes are similar. Please use names made out of words instead of these abbreviations.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1299
> +    default:
> +        ASSERT_NOT_REACHED();

Can we use return instead of break and remove this default case?

[Benjamin Poulain]

Thanks for the review!

(In reply to comment #2)
> (From update of attachment 225607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225607&action=review
> 
> > Source/WebCore/css/SelectorChecker.cpp:282
> >      switch (match) {
> 
> Could we replace all the "break;" here with return true? Then we could remove the default case and move the ASSERT_NOT_REACHED out of the switch and get a warning if someone adds a new case we don’t handle here.

Unfortunately, match can be any selector type, not just the attribute types.

> > Source/WebCore/cssjit/SelectorCompiler.cpp:1241
> > +    bool valueStartsWithExpectedString;
> > +    if (caseSensitivity == CaseSensitive)
> > +        valueStartsWithExpectedString = valueImpl.startsWith(expectedString);
> > +    else
> > +        valueStartsWithExpectedString = valueImpl.startsWith(expectedString, false);
> > +
> > +    if (!valueStartsWithExpectedString)
> > +        return false;
> 
> Single line version of these 7 lines of code:
> 
>     if (!valueImpl.startsWith(expectedString, caseSensitivity == CaseSensitive))

Unfortunately, the version taking a boolean is quite a bit less efficient.

[Benjamin Poulain]

Committed r164961: <http://trac.webkit.org/changeset/164961>

[Alexey Proskuryakov]

This change caused assertion failures all over the place, and Ben is not around, rolling out:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r164964%20(3038)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129596

[Benjamin Poulain]

(In reply to comment #5)
> This change caused assertion failures all over the place, and Ben is not around, rolling out:
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r164964%20(3038)/results.html

Damn, of course. Sorry about that, the assertion does not apply to one of the type.

I have been unable to test in debug because JavaScriptCore was crashing all over the place for me.
I'll fix the assertion.

[Benjamin Poulain]

Committed r164972: <http://trac.webkit.org/changeset/164972>