[webkit-reviews] review denied: [Bug 133314] CSS JIT: add support for the "not" pseudo class : [Attachment 232280] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 133314: CSS JIT: add support for the "not" pseudo class
https://bugs.webkit.org/show_bug.cgi?id=133314

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232280&action=review


The patch is great. Some little comments below.
I think the test coverage could go deeper.

> Source/WebCore/cssjit/SelectorCompiler.cpp:159
> +    Vector<SelectorFragment, 0> notFilters;

You don't need to specify an initial capacity of zero, that is the default.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:430
>> +		    return FunctionType::CannotCompile;
> 
> Is this check necessary? I took the conservative.

Long term, we want to remove FunctionType::CannotCompile and be able to compile
everything as a fast selector.
I think you could return CannotMatchAnything here.

You could also add a regular assertion ASSERT(notFragments.size() != 1) before
the if() to catch any bug in the parser during development.

> Source/WebCore/cssjit/SelectorCompiler.cpp:437
> +	       // Always pass the filters. Don't append the fragment to
notFilters.
> +	       if
(subFragment.pseudoClasses.contains(CSSSelector::PseudoClassVisited))
> +		   return FunctionType::SimpleSelectorChecker;

You don't need to do this yet. We will need to revisit PseudoClassVisited, I am
suspicious the implementation in the slow SelectorChecker may have bugs.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:439
>> +		// FIXME(Yusuke Suzuki): Currently we don't support
visitedMatchType.
> 
> Currently, this implementation doesn't support visitedMatchType. When link
pseudo class is found in the :not, fallback to the non-JIT implementation.
> After this issue, I'm planning to implement :visited, :any.

This is another case where I have doubts about the old SelectorChecker.
Returning CannotCompile is reasonable until it is investigated.
You should remove your name from the FIXME, the bugs are open to everyone.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:471
>> +static FunctionType constructFragments(const CSSSelector *rootSelector,
SelectorContext selectorContext, SelectorFragmentList& selectorFragments)
> 
> Made it a static function.

WebKit style: the star is on the wrong side on "const CSSSelector
*rootSelector".

>> Source/WebCore/cssjit/SelectorCompiler.cpp:540
>> +		functionType = mostRestrictiveFunctionType(functionType,
relationFunctionType);
> 
> Used `mostRestrictiveFunctionType` instead of using `std::max` directly.

Good idea.

> Source/WebCore/cssjit/SelectorCompiler.cpp:567
>      // Element + BacktrackingRegister + ElementData + a pointer to values +
an index on that pointer + the value we expect;
>      unsigned minimum = 6;

This number and comment is spread in 3 places now. Let's put it in a constant
outside the function, and have the comment there.

> Source/WebCore/cssjit/SelectorCompiler.cpp:572
> +	   // Element + ElementData + scratchRegister + attributeArrayPointer +
expectedLocalName + (qualifiedNameImpl && expectedValue).

You can remove this comment.

> LayoutTests/ChangeLog:13
> +	   * fast/selectors/pseudo-class-not-expected.txt: Added.
> +	   * fast/selectors/pseudo-class-not.html: Added.

It would be good to add the following tests:
    -:not() with backtracking.
    -:not() with the maximum register allocation.
    -:not() with :active and/or :hover to test the new inFunctionalPseudoClass