Add a fallback path for compiling the remaining attribute checkers
Created attachment 225607 [details] Patch
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?
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.
Committed r164961: <http://trac.webkit.org/changeset/164961>
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
Re-opened since this is blocked by bug 129596
(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.
Committed r164972: <http://trac.webkit.org/changeset/164972>