[webkit-reviews] review granted: [Bug 129580] Add a fallback path for compiling the remaining attribute checkers : [Attachment 225607] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 129580: Add a fallback path for compiling the remaining attribute checkers
https://bugs.webkit.org/show_bug.cgi?id=129580

Attachment 225607: Patch
https://bugs.webkit.org/attachment.cgi?id=225607&action=review

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