Bug 133314 - CSS JIT: add support for the "not" pseudo class
Summary: CSS JIT: add support for the "not" pseudo class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 133295
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-27 10:30 PDT by Yusuke Suzuki
Modified: 2014-06-02 00:01 PDT (History)
2 users (show)

See Also:


Attachments
Patch (24.71 KB, patch)
2014-05-29 22:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (69.29 KB, patch)
2014-06-01 08:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (69.33 KB, patch)
2014-06-01 08:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (71.04 KB, patch)
2014-06-01 21:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2014-05-27 10:30:34 PDT
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
Comment 1 Yusuke Suzuki 2014-05-29 22:24:47 PDT
Created attachment 232280 [details]
Patch
Comment 2 Yusuke Suzuki 2014-05-29 22:50:27 PDT
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.
Comment 3 Benjamin Poulain 2014-05-30 16:25:14 PDT
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
Comment 4 Yusuke Suzuki 2014-05-30 23:50:00 PDT
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!
Comment 5 Yusuke Suzuki 2014-06-01 08:31:58 PDT
Created attachment 232342 [details]
Patch
Comment 6 Yusuke Suzuki 2014-06-01 08:34:40 PDT
Created attachment 232343 [details]
Patch
Comment 7 Benjamin Poulain 2014-06-01 16:31:31 PDT
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".
Comment 8 Yusuke Suzuki 2014-06-01 21:56:32 PDT
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 ...".
Comment 9 Yusuke Suzuki 2014-06-01 21:57:49 PDT
Created attachment 232367 [details]
Patch
Comment 10 WebKit Commit Bot 2014-06-02 00:01:24 PDT
Comment on attachment 232367 [details]
Patch

Clearing flags on attachment: 232367

Committed r169525: <http://trac.webkit.org/changeset/169525>
Comment 11 WebKit Commit Bot 2014-06-02 00:01:28 PDT
All reviewed patches have been landed.  Closing bug.