Bug 133314 – CSS JIT: add support for the "not" pseudo class

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yusuke Suzuki]

Add support for the "not" pseudo class.
After "active" and "hover" support is done, I'll start to implement this.

NOTE:
We need to consider about this change[1].

[1]: https://bugs.webkit.org/show_bug.cgi?id=133063

[Yusuke Suzuki]

Created attachment 232280 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 232280 [details]
Patch

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

Added comments to review it easily.

> Source/WebCore/cssjit/SelectorCompiler.cpp:430
> +                return FunctionType::CannotCompile;

Is this check necessary? I took the conservative.

> Source/WebCore/cssjit/SelectorCompiler.cpp:433
> +            subFragment.inFunctionalPseudoClass = true;

Set `inFunctionalPseudoClass` as true.

> 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.

> Source/WebCore/cssjit/SelectorCompiler.cpp:468
> +        computeBacktrackingInformation(m_selectorFragments, m_needsAdjacentBacktrackingStart);

Splitted these sequences to `constructFragments` and `computeBacktrackingInformation` since :not doesn't need `computeBacktrackingInformation`.

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

Made it a static function.

> Source/WebCore/cssjit/SelectorCompiler.cpp:540
> +            functionType = mostRestrictiveFunctionType(functionType, relationFunctionType);

Used `mostRestrictiveFunctionType` instead of using `std::max` directly.

> Source/WebCore/cssjit/SelectorCompiler.cpp:563
> +static inline unsigned minimumRegisterRequirements(const SelectorFragment& selectorFragment)

Splitted it to 2 functions, one for Fragment, and one for FragmentList.

> Source/WebCore/cssjit/SelectorCompiler.cpp:590
> +        unsigned notFilterMinimum = minimumRegisterRequirements(subFragment);

Calculating register requirements for notFilters.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1637
> +    if (!fragment.notFilters.isEmpty())

Implemented notFilters check.

[Benjamin Poulain]

Comment on attachment 232280 [details]
Patch

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

[Yusuke Suzuki]

Comment on attachment 232280 [details]
Patch

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

Thank you for your review, Benjamin.

>> 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.

Removed initial capacity.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:437
>> +                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.

Ah, right. I don't know why adjacent :visited causes security risk. We need to investigate it.
So removed this.

>>> 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.

Right. Removed my name from the FIXME.

>>> 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".

Thank you! Fixed it.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:567
>>      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.

That's very nice. Extracted it.

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

This `6` and `minimum`'s `6` have different meaning. So extracted these constants to `numberOfMinimumRequiredRegisters` and `numberOfMinimumRequiredRegistersForAttributeFilter`.

>> LayoutTests/ChangeLog:13
>> +        * 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

Right. I'll add more LayoutTests!

[Yusuke Suzuki]

Created attachment 232342 [details]
Patch

[Yusuke Suzuki]

Created attachment 232343 [details]
Patch

[Benjamin Poulain]

Comment on attachment 232343 [details]
Patch

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

Great patch

> Source/WebCore/cssjit/SelectorCompiler.cpp:158
>      Vector<std::pair<int, int>> nthChildfilters;

Ooops,I forgot to capitalize Filters here :|

> Source/WebCore/cssjit/SelectorCompiler.cpp:233
> +    void generateElementIsNotMatchedToNegation(Assembler::JumpList& failureCases, const SelectorFragment&);

I realize the name "generateElementIsNotMatchedToNegation" is confusing. What do you think of generateElementMatchesNotPseudoClass?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1659
> -    generateElementAttributesMatching(failureCases, elementDataAddress, fragment);
> +        generateElementAttributesMatching(failureCases, elementDataAddress, fragment);

Oops.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2560
> +    Assembler::JumpList localFailureCases;

This could be moved in the for() loop.

> LayoutTests/ChangeLog:23
> +        * fast/selectors/not-active-hover-quirks-expected.txt: Added.
> +        * fast/selectors/not-active-hover-quirks.html: Added.
> +        * fast/selectors/not-active-hover-strict-expected.txt: Added.
> +        * fast/selectors/not-active-hover-strict.html: Added.
> +        * fast/selectors/pseudo-class-not-expected.txt: Added.
> +        * fast/selectors/pseudo-class-not.html: Added.
> +        * fast/selectors/resources/not-hover-active-quirks-utility.js: Added.
> +        (testQuerySelector):
> +        (test):
> +        * fast/selectors/resources/not-hover-active-strict-utility.js: Added.
> +        (testQuerySelector):
> +        (test):

Wow, that's a lot of tests :)

> LayoutTests/fast/selectors/not-active-hover-quirks.html:15
> +description('Test the :not(:active) and :not(:hover) selector when the document is in quirks mode. To test manually, ensure that not move the cursor over the green rectangle and press the gray START div until the test is finished.');

Typo here: "ensure that not move".

[Yusuke Suzuki]

Comment on attachment 232343 [details]
Patch

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

Thank you for your review! I'll upload the revised patch soon.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:158
>>      Vector<std::pair<int, int>> nthChildfilters;
> 
> Ooops,I forgot to capitalize Filters here :|

Oh! I've renamed it :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:233
>> +    void generateElementIsNotMatchedToNegation(Assembler::JumpList& failureCases, const SelectorFragment&);
> 
> I realize the name "generateElementIsNotMatchedToNegation" is confusing. What do you think of generateElementMatchesNotPseudoClass?

`generateElementMatchesNotPseudoClass` looks nice to me. `NotPseudoClass` seems very clear to code readers.
I've renamed it!

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2560
>> +    Assembler::JumpList localFailureCases;
> 
> This could be moved in the for() loop.

Right, done :)

>> LayoutTests/fast/selectors/not-active-hover-quirks.html:15
>> +description('Test the :not(:active) and :not(:hover) selector when the document is in quirks mode. To test manually, ensure that not move the cursor over the green rectangle and press the gray START div until the test is finished.');
> 
> Typo here: "ensure that not move".

Changed to "make sure not to move ... over ...".

[Yusuke Suzuki]

Created attachment 232367 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 232367 [details]
Patch

Clearing flags on attachment: 232367

Committed r169525: <http://trac.webkit.org/changeset/169525>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.