Bug 134835 – CSS JIT: Implement Pseudo Element

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

[Yusuke Suzuki]

Implement Pseudo Element.

[Yusuke Suzuki]

Created attachment 234770 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 234770 [details]
Patch

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

Added comments.

> Source/WebCore/css/ElementRuleCollector.cpp:314
> +        if (!m_pseudoStyleRequest.scrollbar) {

Currently, we don't support scroll / selection's pseudo element.

> Source/WebCore/cssjit/SelectorCompiler.cpp:170
> +    PseudoId dynamicPseudo;

Noe: When mode is `StyleInvalidation`, dynamicPseudo value should be considered as `NOPSEUDO`. (see SelectorChecker.cpp)
So when using this value, we need to consider about `StyleInvalidation` case.

> Source/WebCore/cssjit/SelectorCompiler.cpp:583
> +        // TODO: Implement scrollbar / selection's exceptional case.

We don't support it yet.

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

We don't support it yet. We start with the simple case.

> Source/WebCore/cssjit/SelectorCompiler.cpp:662
> +                return FunctionType::CannotMatchAnything;

When mode is QuerySelector, selector always fails.

> Source/WebCore/cssjit/SelectorCompiler.cpp:672
> +                break;

We only support these cases yet.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1270
> +    }

This is corresponding to SelectorChecker::mark's process.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1516
> +void SelectorCodeGenerator::loadCheckingContext(Assembler::RegisterID checkingContext)

Extracted this.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1525
> +Assembler::Jump SelectorCodeGenerator::branchOnResolvingModeWithCheckingContext(Assembler::RelationalCondition condition, SelectorChecker::Mode mode, Assembler::RegisterID checkingContext)

Extracted this.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2825
> +        Assembler::Jump styleInvalidation = branchOnResolvingModeWithCheckingContext(Assembler::Equal, SelectorChecker::Mode::StyleInvalidation, checkingContext);

When mode is `StyleInvalidation`, we consider it as `NOPSEUDO`.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2839
> +    if (shouldUseRenderStyleFromCheckingContext(fragment)) {

`context.pseudoId` has effect only when fragment is Rightmost in the root fragments.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2851
> +            failureCases.append(branchOnResolvingModeWithCheckingContext(Assembler::Equal, SelectorChecker::Mode::StyleInvalidation, checkingContext));

Consider `StyleInvalidation` case.

[Benjamin Poulain]

Comment on attachment 234770 [details]
Patch

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

Great work! I really enjoy how well written your patches are.

I have only made a first pass tonight. I have some style comments but I haven't seen any correctness issue so far. I will look into this more tomorrow.

> Source/WebCore/css/ElementRuleCollector.cpp:313
> +        // TODO: Currently a compiled selector doesn't support scrollbar / selection's exceptional case.

WebKit always use FIXME instead of TODO.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:170
>> +    PseudoId dynamicPseudo;
> 
> Noe: When mode is `StyleInvalidation`, dynamicPseudo value should be considered as `NOPSEUDO`. (see SelectorChecker.cpp)
> So when using this value, we need to consider about `StyleInvalidation` case.

I believe it would be better to keep a pointer to the CSSSelector* for the pseudo element here. That way it will be easier to add support for custom pseudo elements.

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

This one should be in the "Helpers" section bellow instead of the "Element properties matchers".

I would name this generateMarkPseudoStyleForPseudoElement() since it is quite a unique kind of marking.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:583
>> +        // TODO: Implement scrollbar / selection's exceptional case.
> 
> We don't support it yet.

TODO->FIXME.

I would extend this comment quite a bit.

> Source/WebCore/cssjit/SelectorCompiler.cpp:587
> +        if (relation == CSSSelector::SubSelector) {
> +            if (fragment.dynamicPseudo != NOPSEUDO)
> +                return FunctionType::CannotCompile;
> +        }

I am quite confused by this, can you explain?

I see SelectorChecker::matchRecursively() has quite a bit of mess in CSSSelector::SubSelector. Some of that code looks suspicious :(

> Source/WebCore/cssjit/SelectorCompiler.cpp:651
> +        case CSSSelector::PseudoElement: {

Probably best to move this block just after CSSSelector::PseudoClass for clarity.

> Source/WebCore/cssjit/SelectorCompiler.cpp:681
>              fragment.onlyMatchesLinksInQuirksMode = false;
> -            return FunctionType::CannotCompile;
> +
> +            if (selector->pseudoElementType() == CSSSelector::PseudoElementCue)
> +                return FunctionType::CannotCompile;
> +
> +            if (selector->isCustomPseudoElement())
> +                return FunctionType::CannotCompile;
> +
> +            // In the QuerySelector context, PseudoElement selectors always fail.
> +            if (selectorContext == SelectorContext::QuerySelector)
> +                return FunctionType::CannotMatchAnything;
> +
> +            PseudoId pseudoId = CSSSelector::pseudoId(selector->pseudoElementType());
> +            fragment.dynamicPseudo = pseudoId;
> +
> +            switch (pseudoId) {
> +            case FIRST_LINE:
> +            case FIRST_LETTER:
> +            case BEFORE:
> +            case AFTER:
> +                break;
> +
> +            // TODO: Support SCROLLBAR, RESIZER, SELECTION etc.
> +            default:
> +                return FunctionType::CannotCompile;
> +            }
> +
> +            functionType = FunctionType::SelectorCheckerWithCheckingContext;
> +            break;
> +        }

I would simplify this whole block.

First, let's start with if (selectorContext == SelectorContext::QuerySelector) to get that out of the way.

Then, switch() on selector->pseudoElementType(), PseudoElementAfter, PseudoElementBefore, PseudoElementFirstLetter, PseudoElementFirstLine succeed, everything else fails.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1268
> +        RELEASE_ASSERT(!m_selectorFragments.isEmpty());

If I am not mistaken, Vector::first() will already assert on overflow. In that case this could be an ASSERT instead of a RELEASE_ASSERT.

[Yusuke Suzuki]

Comment on attachment 234770 [details]
Patch

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

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

>> Source/WebCore/css/ElementRuleCollector.cpp:313
>> +        // TODO: Currently a compiled selector doesn't support scrollbar / selection's exceptional case.
> 
> WebKit always use FIXME instead of TODO.

Thx, changed.

>>> Source/WebCore/cssjit/SelectorCompiler.cpp:170
>>> +    PseudoId dynamicPseudo;
>> 
>> Noe: When mode is `StyleInvalidation`, dynamicPseudo value should be considered as `NOPSEUDO`. (see SelectorChecker.cpp)
>> So when using this value, we need to consider about `StyleInvalidation` case.
> 
> I believe it would be better to keep a pointer to the CSSSelector* for the pseudo element here. That way it will be easier to add support for custom pseudo elements.

OK, I've changed `PseudoId dynamicPseudo` to `const CSSSelector* pseudoElementSelector`.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:260
>> +    void generateMarkPseudoStyle(Assembler::JumpList& failureCases, const SelectorFragment&);
> 
> This one should be in the "Helpers" section bellow instead of the "Element properties matchers".
> 
> I would name this generateMarkPseudoStyleForPseudoElement() since it is quite a unique kind of marking.

Sounds nice. Changed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:587
>> +        }
> 
> I am quite confused by this, can you explain?
> 
> I see SelectorChecker::matchRecursively() has quite a bit of mess in CSSSelector::SubSelector. Some of that code looks suspicious :(

In this case, we should reject subselector after pseudo element case. For example, "p::first-line" works, but "p::first-line.ok" should not work.
This process is defined in SelectorChecker.cpp, http://trac.webkit.org/browser/trunk/Source/WebCore/css/SelectorChecker.cpp?rev=171059#L272

But to complete implementation, we need to cover
+ scrollbar / selection exceptional cases
+ StyleInvalidation (dynamicPseudo == NOPSEUDO) case

So in this implementation, simply fallback this case.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:651
>> +        case CSSSelector::PseudoElement: {
> 
> Probably best to move this block just after CSSSelector::PseudoClass for clarity.

Right. Moved.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:681
>> +        }
> 
> I would simplify this whole block.
> 
> First, let's start with if (selectorContext == SelectorContext::QuerySelector) to get that out of the way.
> 
> Then, switch() on selector->pseudoElementType(), PseudoElementAfter, PseudoElementBefore, PseudoElementFirstLetter, PseudoElementFirstLine succeed, everything else fails.

Nice! Changed to that!

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1268
>> +        RELEASE_ASSERT(!m_selectorFragments.isEmpty());
> 
> If I am not mistaken, Vector::first() will already assert on overflow. In that case this could be an ASSERT instead of a RELEASE_ASSERT.

Right. Changed it.

[Yusuke Suzuki]

Created attachment 234846 [details]
Patch

[Build Bot]

Comment on attachment 234846 [details]
Patch

Attachment 234846 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5642612279083008

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html

[Build Bot]

Created attachment 234848 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Benjamin Poulain]

Comment on attachment 234846 [details]
Patch

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

Some comments. I'll come back to this to finish the review.

> Source/WebCore/css/ElementRuleCollector.cpp:297
> +            if (m_pseudoStyleRequest.pseudoId != NOPSEUDO)
> +                return false;

Now with http://trac.webkit.org/changeset/171059, I think we can change this into an assertion for m_pseudoStyleRequest.pseudoId != NOPSEUDO.

I think ASSERT_WITH_MESSAGE would be useful here. Maybe something like:
    ASSERT_WITH_MESSAGE(m_pseudoStyleRequest.pseudoId != NOPSEUDO, "When matching pseudo elements, we should never compile a selector checker without context. ElementRuleCollector::collectMatchingRulesForList() should filter out useless rules for pseudo elements.")

> Source/WebCore/cssjit/SelectorCompiler.cpp:272
>      void addFlagsToElementStyleFromContext(Assembler::RegisterID checkingContext, int64_t);
>      Assembler::JumpList jumpIfNoPreviousAdjacentElement();
>      Assembler::JumpList jumpIfNoNextAdjacentElement();
> +    Assembler::Jump branchOnResolvingModeWithCheckingContext(Assembler::RelationalCondition, SelectorChecker::Mode, Assembler::RegisterID checkingContext);
> +    Assembler::Jump branchOnResolvingMode(Assembler::RelationalCondition, SelectorChecker::Mode, Assembler::RegisterID scratchForCheckingContext);
>      Assembler::Jump jumpIfNotResolvingStyle(Assembler::RegisterID checkingContextRegister);
>      void generateSpecialFailureInQuirksModeForActiveAndHoverIfNeeded(Assembler::JumpList& failureCases, const SelectorFragment&);
>      Assembler::Jump modulo(JSC::MacroAssembler::ResultCondition, Assembler::RegisterID inputDividend, int divisor);
>      void moduloIsZero(Assembler::JumpList& failureCases, Assembler::RegisterID inputDividend, int divisor);
> +    void loadCheckingContext(Assembler::RegisterID checkingContext);
> +    void generateMarkPseudoStyleForPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment&);

You can keep those sorted alphabetically by ASCII comparison. That's easier than deciding what is the best place for the APIs :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:583
> +        // FIXME: Implement scrollbar / selection's exceptional case.

I would prefer a complete explanation like in SelectorChecker. This is not obvious stuff.

> Source/WebCore/cssjit/SelectorCompiler.cpp:587
> +        if (relation == CSSSelector::SubSelector) {
> +            if (fragment.pseudoElementSelector)
> +                return FunctionType::CannotCompile;
> +        }

Since we do not handle the scrollbar, nor the selection pseudo elements, shouldn't this return CannotMatchAnything?

> Source/WebCore/cssjit/SelectorCompiler.cpp:622
> +        case CSSSelector::PseudoElement: {

Can you please add a test for pseudo element matching inside the  functional pseudo classes :not() and :-webkit-any()?

I am concerned about the FIRST_LETTER that has special marking yet does not match the rules. I doubt we have good coverage for that.
I doubt the code of SelectorChecker is right in that case, but that's another issue :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:2822
> +        Assembler::Jump styleInvalidation = branchOnResolvingModeWithCheckingContext(Assembler::Equal, SelectorChecker::Mode::StyleInvalidation, checkingContext);
> +
> +        FunctionCall functionCall(m_assembler, m_registerAllocator, m_stackAllocator, m_functionCalls);
> +        functionCall.setFunctionAddress(setUsesFirstLetterRules);
> +        functionCall.setOneArgument(elementAddressRegister);
> +        functionCall.call();
> +
> +        // When mode is style invalidation, we can skip this check.
> +        styleInvalidation.link(&m_assembler);

I think we may be able to remove this entirely by changing the SelectorChecker and RuleFeatureSet first in a separate patch.

RuleFeatureSet::collectFeaturesFromSelector() already collect the selector features to setup the "fast path" flags. I don't know why FirstLetter is treated differently, it may just be an historical accident.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2826
> +void SelectorCodeGenerator::generateElementHasRequestedPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment& fragment)

I wonder if this check could be moved above all other testing in the cases where the pseudo element is not inside a :-webkit-any(). Maybe even before we save the checking context on the stack.

In SelectorChecker, we have no idea what dynamicPseudo is gonna be, so we have to execute the pseudo element code before exiting.
In the CSS JIT, we know exactly what we will test for, so we could return early if the input pseudo ID does not correspond to what this selector can match.

This becomes tricky with -wekbit-any(). :(

> Source/WebCore/cssjit/SelectorCompiler.cpp:2835
> +        if (!fragment.pseudoElementSelector)
> +            failureCases.append(m_assembler.branch8(Assembler::NotEqual, Assembler::Address(checkingContext, OBJECT_OFFSETOF(CheckingContext, pseudoId)), Assembler::TrustedImm32(NOPSEUDO)));

This case in particular would be awesome if it could become the first instruction of the compiled selector.

[Yusuke Suzuki]

Comment on attachment 234846 [details]
Patch

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

Thank you for your detailed review!
This is my quick response, I'm not responding to the all comments yet. I'll see all comments and respond them soon :)

>> Source/WebCore/css/ElementRuleCollector.cpp:297
>> +                return false;
> 
> Now with http://trac.webkit.org/changeset/171059, I think we can change this into an assertion for m_pseudoStyleRequest.pseudoId != NOPSEUDO.
> 
> I think ASSERT_WITH_MESSAGE would be useful here. Maybe something like:
>     ASSERT_WITH_MESSAGE(m_pseudoStyleRequest.pseudoId != NOPSEUDO, "When matching pseudo elements, we should never compile a selector checker without context. ElementRuleCollector::collectMatchingRulesForList() should filter out useless rules for pseudo elements.")

http://trac.webkit.org/changeset/171059 Looks very cool! selector not matching pseudo element + pseudoId != NOPSEUDO combination is filtered out in collectMatchingRulesForList() phase.
So I've inserted an assertion for m_pseudoStyleRequest.pseudoId == NOPSEUDO. Maybe `!=` is typo, correct?

>> Source/WebCore/cssjit/SelectorCompiler.cpp:272
>> +    void generateMarkPseudoStyleForPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment&);
> 
> You can keep those sorted alphabetically by ASCII comparison. That's easier than deciding what is the best place for the APIs :)

Make sense. Changed them :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:583
>> +        // FIXME: Implement scrollbar / selection's exceptional case.
> 
> I would prefer a complete explanation like in SelectorChecker. This is not obvious stuff.

Sounds nice. I've moved comments on SelectorChecker here. And added more detailed FIXME comment.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:587
>> +        }
> 
> Since we do not handle the scrollbar, nor the selection pseudo elements, shouldn't this return CannotMatchAnything?

When mode is `StyleInvalidation`, dynamicPseudo in SelectorChecker becomes NOPSEUDO, and as the result, this[1] guard becomes false and it doesn't become CannotMatchAnything.

[1]: http://trac.webkit.org/browser/trunk/Source/WebCore/css/SelectorChecker.cpp?rev=171107#L278

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2822
>> +        styleInvalidation.link(&m_assembler);
> 
> I think we may be able to remove this entirely by changing the SelectorChecker and RuleFeatureSet first in a separate patch.
> 
> RuleFeatureSet::collectFeaturesFromSelector() already collect the selector features to setup the "fast path" flags. I don't know why FirstLetter is treated differently, it may just be an historical accident.

That's reasonable. I'll open an issue for that first!
I've created an issue[2]. Later, I'll attach a patch.

[2]: https://bugs.webkit.org/show_bug.cgi?id=134936

[Yusuke Suzuki]

Comment on attachment 234846 [details]
Patch

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

Thank you for your detailed review!
This is my quick response, I'm not responding to the all comments yet. I'll see all comments and respond them soon :)

>> Source/WebCore/css/ElementRuleCollector.cpp:297
>> +                return false;
> 
> Now with http://trac.webkit.org/changeset/171059, I think we can change this into an assertion for m_pseudoStyleRequest.pseudoId != NOPSEUDO.
> 
> I think ASSERT_WITH_MESSAGE would be useful here. Maybe something like:
>     ASSERT_WITH_MESSAGE(m_pseudoStyleRequest.pseudoId != NOPSEUDO, "When matching pseudo elements, we should never compile a selector checker without context. ElementRuleCollector::collectMatchingRulesForList() should filter out useless rules for pseudo elements.")

http://trac.webkit.org/changeset/171059 Looks very cool! selector not matching pseudo element + pseudoId != NOPSEUDO combination is filtered out in collectMatchingRulesForList() phase.
So I've inserted an assertion for m_pseudoStyleRequest.pseudoId == NOPSEUDO. Maybe `!=` is typo, correct?

>> Source/WebCore/cssjit/SelectorCompiler.cpp:272
>> +    void generateMarkPseudoStyleForPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment&);
> 
> You can keep those sorted alphabetically by ASCII comparison. That's easier than deciding what is the best place for the APIs :)

Make sense. Changed them :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:583
>> +        // FIXME: Implement scrollbar / selection's exceptional case.
> 
> I would prefer a complete explanation like in SelectorChecker. This is not obvious stuff.

Sounds nice. I've moved comments on SelectorChecker here. And added more detailed FIXME comment.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:587
>> +        }
> 
> Since we do not handle the scrollbar, nor the selection pseudo elements, shouldn't this return CannotMatchAnything?

When mode is `StyleInvalidation`, dynamicPseudo in SelectorChecker becomes NOPSEUDO, and as the result, this[1] guard becomes false and it doesn't become CannotMatchAnything.

[1]: http://trac.webkit.org/browser/trunk/Source/WebCore/css/SelectorChecker.cpp?rev=171107#L278

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2822
>> +        styleInvalidation.link(&m_assembler);
> 
> I think we may be able to remove this entirely by changing the SelectorChecker and RuleFeatureSet first in a separate patch.
> 
> RuleFeatureSet::collectFeaturesFromSelector() already collect the selector features to setup the "fast path" flags. I don't know why FirstLetter is treated differently, it may just be an historical accident.

That's reasonable. I'll open an issue for that first!
I've created an issue[2]. Later, I'll attach a patch.

[2]: https://bugs.webkit.org/show_bug.cgi?id=134936

[Yusuke Suzuki]

Comment on attachment 234846 [details]
Patch

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

>> Source/WebCore/cssjit/SelectorCompiler.cpp:622
>> +        case CSSSelector::PseudoElement: {
> 
> Can you please add a test for pseudo element matching inside the  functional pseudo classes :not() and :-webkit-any()?
> 
> I am concerned about the FIRST_LETTER that has special marking yet does not match the rules. I doubt we have good coverage for that.
> I doubt the code of SelectorChecker is right in that case, but that's another issue :)

Right. I'll add test for pseudo element matching inside the :not and :-webkit-any.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2826
>> +void SelectorCodeGenerator::generateElementHasRequestedPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment& fragment)
> 
> I wonder if this check could be moved above all other testing in the cases where the pseudo element is not inside a :-webkit-any(). Maybe even before we save the checking context on the stack.
> 
> In SelectorChecker, we have no idea what dynamicPseudo is gonna be, so we have to execute the pseudo element code before exiting.
> In the CSS JIT, we know exactly what we will test for, so we could return early if the input pseudo ID does not correspond to what this selector can match.
> 
> This becomes tricky with -wekbit-any(). :(

Sounds nice.
However, since discarding stack process is placed before `ret`, we need to push the context to the stack before jumping to the failure path.

In the meantime, I've moved this process to the place after pushing contents to the stack.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2835
>> +            failureCases.append(m_assembler.branch8(Assembler::NotEqual, Assembler::Address(checkingContext, OBJECT_OFFSETOF(CheckingContext, pseudoId)), Assembler::TrustedImm32(NOPSEUDO)));
> 
> This case in particular would be awesome if it could become the first instruction of the compiled selector.

Right!

[Yusuke Suzuki]

Created attachment 235062 [details]
Patch

[Yusuke Suzuki]

Created attachment 235063 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 235063 [details]
Patch

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

Added comments

> Source/WebCore/cssjit/SelectorCompiler.cpp:649
> +        }

This block is simplified.

> Source/WebCore/cssjit/SelectorCompiler.cpp:699
> +        fragment.fragmentLevel = fragmentLevel;

Remember fragmentLevel in fragment.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1231
> +    }

Check and go to failure path earlily. In this phase, selector do nothing except for prologue. So in early failure path, we only do epilogue.
Especially in x86_64 environment, compiler do nothing before this.
And we use checkingContextRegister directly!

> Source/WebCore/cssjit/SelectorCompiler.cpp:1282
> +    }

When selector matching is succeeded, marking pseudo style.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1294
> +            ASSERT_WITH_MESSAGE(earlyFailureCases.empty(), "Early failure is used for pseudo element. When early failure is used, function type is SelectorCheckerWithCheckingContext.");

Since earlyFailureCases is used when mode is SelectorCheckerWithCheckingContext, so here (mode is SimpleSelectorChecker), earlyFailureCases should be empty.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1330
> +    }

Generate early failure cases. Only generating epilogue is needed :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:1864
> +        if (fragment.fragmentLevel == FragmentsLevel::InFunctionalPseudoType) {

This is tricky. By using fragmentLevel stored in fragment, we handle the pseudo element inside :-webkit-any case.
Pseudo element in root fragments are handled earlily.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2867
> +        return;

This path is already handled in generateRequestedPseudoElementEqualsToSelectorPseudoElement, so JIT compiler can statically remove this!

> Source/WebCore/cssjit/SelectorCompiler.cpp:2877
> +    //  This branch is already checked in generateRequestedPseudoElementEqualsToSelectorPseudoElement.

Same as above!

[Benjamin Poulain]

Comment on attachment 235063 [details]
Patch

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

A few comments, I like the direction this is going. I think the early return is gonna be fantastic for performance.

I now realize WebKit is in a pretty bad state for :webkit-any(). It is not something you should address with this patch, but I probably made some incorrect comments in previous reviews.

Sorry to add such delay on reviews, I'll try to do better.

> Source/WebCore/cssjit/SelectorCompiler.cpp:270
> +    Assembler::Jump branchOnResolvingMode(Assembler::RelationalCondition, SelectorChecker::Mode, Assembler::RegisterID scratchForCheckingContext);

Consistency: this one use the name "scratchForCheckingContext", the other helpers use "checkingContext".

> Source/WebCore/cssjit/SelectorCompiler.cpp:585
> +        // a selector is invalid if something follows a pseudo-element

The first letter should be uppercase and the sentence should end with a period.

> Source/WebCore/cssjit/SelectorCompiler.cpp:591
> +        if (relation == CSSSelector::SubSelector) {
> +            if (fragment.pseudoElementSelector)

Let's do "if (relation == CSSSelector::SubSelector && fragment.pseudoElementSelector) -> early return".

> Source/WebCore/cssjit/SelectorCompiler.cpp:1225
> +    Assembler::JumpList earlyFailureCases;

Maybe we could make this name clearer. What about: "failuresBeforeUsingTheStack", "failureOnFunctionEntry" or something like that?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1229
> +        ASSERT_WITH_MESSAGE(shouldUseRenderStyleFromCheckingContext(m_selectorFragments.first()), "Code for checking pseudo element status is always generated here.");

I like that you used ASSERT_WITH_MESSAGE here but I believe the message could be clearer.

Maybe: "Matching pseudo elements only make sense for the rightmost fragment"?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1230
> +        generateRequestedPseudoElementEqualsToSelectorPseudoElement(earlyFailureCases, m_selectorFragments.first(), checkingContextRegister);

Could you look into generating this even before the prologue? We could evaluate the condition without even touching the stack.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1329
> +    if (!earlyFailureCases.empty()) {
> +        earlyFailureCases.link(&m_assembler);
> +        m_assembler.move(Assembler::TrustedImm32(0), returnRegister);
> +        if (needsEpilogue)
> +            generateEpilogue();
> +        m_assembler.ret();

The early failure is brilliant.

If you can generate the early failure before the prologue, we would not even need to generate the prologue and return twice. You could just directly to the ret above.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1537
> +Assembler::Jump SelectorCodeGenerator::branchOnResolvingMode(Assembler::RelationalCondition condition, SelectorChecker::Mode mode, Assembler::RegisterID scratchForCheckingContext)

Consistency: this one use the name "scratchForCheckingContext", the other helpers use "checkingContext".

> Source/WebCore/cssjit/SelectorCompiler.cpp:1545
> +    // If we not resolving style, skip the whole marking.

You can remove that comment, it had been moved out of its context, it is more misleading than helping now.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1864
>> +        if (fragment.fragmentLevel == FragmentsLevel::InFunctionalPseudoType) {
> 
> This is tricky. By using fragmentLevel stored in fragment, we handle the pseudo element inside :-webkit-any case.
> Pseudo element in root fragments are handled earlily.

Looking at the old SelectorChecker, I see pseudo elements are completely ignored inside :-webkit-any().

The state for Selector Level 4 is undefined (see issue 7).

I completely misled you with my previous comments, sorry about that. I thought we fully support pseudo elements in :-webkit-any.
For now we should match SelectorChecker.
    Do you think we can just use CannotMatchAnything if we encounter any pseudo element inside a functional pseudo class (and update SelectorChecker accordingly)?
    Alternatively, we can just ignore pseudo elements completely inside functional pseudo types.
We should also find out what Mozilla does for -moz-any().

Long term, we should start talking with Tab to clarify/improve the spec of Level 4!

> Source/WebCore/cssjit/SelectorCompiler.cpp:2821
> +void SelectorCodeGenerator::generateRequestedPseudoElementEqualsToSelectorPseudoElement(Assembler::JumpList& failureCases, const SelectorFragment& fragment, Assembler::RegisterID checkingContext)

Gosh, I love this. Most failed cases will evaluate instantly.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2826
> +        //  Generate:
> +        //     if (context.pseudoId != NOPSEUDO && context.pseudoId != dynamicPseudo)
> +        //         return SelectorFailsCompletely;

This comment is a bit misleading. 
What you generate here is huge, it means we take a big shortcut in the evaluation.

You could probably do a little explanation of this optimization instead of the pseudo code.

> LayoutTests/fast/selectors/pseudo-element-inside-any-expected.html:15
> +            <p class="target" id="target2" style="background-color: lime">Background color becomes lime.</p>

Isn't this showing :-webkit-any(::first-letter) is completely broken? The pseudo element is completely ignored :(

[Yusuke Suzuki]

Created attachment 235412 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 235063 [details]
Patch

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

Thank you for your detailed comments!

>> Source/WebCore/cssjit/SelectorCompiler.cpp:270
>> +    Assembler::Jump branchOnResolvingMode(Assembler::RelationalCondition, SelectorChecker::Mode, Assembler::RegisterID scratchForCheckingContext);
> 
> Consistency: this one use the name "scratchForCheckingContext", the other helpers use "checkingContext".

I used `scratchForCheckingContext` because loadCheckingContext(scratchForCheckingContext) is called inside this function.
But maybe this is easy to understand without using this name. So changed to `checkingContext`.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:585
>> +        // a selector is invalid if something follows a pseudo-element
> 
> The first letter should be uppercase and the sentence should end with a period.

Right, fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:591
>> +            if (fragment.pseudoElementSelector)
> 
> Let's do "if (relation == CSSSelector::SubSelector && fragment.pseudoElementSelector) -> early return".

Yeah, changed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1225
>> +    Assembler::JumpList earlyFailureCases;
> 
> Maybe we could make this name clearer. What about: "failuresBeforeUsingTheStack", "failureOnFunctionEntry" or something like that?

I'll change it to `failureOnFunctionEntry`, and jump before function prologue.
Seeing the code, we can check and jump before prologue.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1229
>> +        ASSERT_WITH_MESSAGE(shouldUseRenderStyleFromCheckingContext(m_selectorFragments.first()), "Code for checking pseudo element status is always generated here.");
> 
> I like that you used ASSERT_WITH_MESSAGE here but I believe the message could be clearer.
> 
> Maybe: "Matching pseudo elements only make sense for the rightmost fragment"?

That's correct. Changing your suggestion is better I think too.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1230
>> +        generateRequestedPseudoElementEqualsToSelectorPseudoElement(earlyFailureCases, m_selectorFragments.first(), checkingContextRegister);
> 
> Could you look into generating this even before the prologue? We could evaluate the condition without even touching the stack.

We can move this to the position before the prologue, moved :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1537
>> +Assembler::Jump SelectorCodeGenerator::branchOnResolvingMode(Assembler::RelationalCondition condition, SelectorChecker::Mode mode, Assembler::RegisterID scratchForCheckingContext)
> 
> Consistency: this one use the name "scratchForCheckingContext", the other helpers use "checkingContext".

Done.

>>> Source/WebCore/cssjit/SelectorCompiler.cpp:1864
>>> +        if (fragment.fragmentLevel == FragmentsLevel::InFunctionalPseudoType) {
>> 
>> This is tricky. By using fragmentLevel stored in fragment, we handle the pseudo element inside :-webkit-any case.
>> Pseudo element in root fragments are handled earlily.
> 
> Looking at the old SelectorChecker, I see pseudo elements are completely ignored inside :-webkit-any().
> 
> The state for Selector Level 4 is undefined (see issue 7).
> 
> I completely misled you with my previous comments, sorry about that. I thought we fully support pseudo elements in :-webkit-any.
> For now we should match SelectorChecker.
>     Do you think we can just use CannotMatchAnything if we encounter any pseudo element inside a functional pseudo class (and update SelectorChecker accordingly)?
>     Alternatively, we can just ignore pseudo elements completely inside functional pseudo types.
> We should also find out what Mozilla does for -moz-any().
> 
> Long term, we should start talking with Tab to clarify/improve the spec of Level 4!

> Do you think we can just use CannotMatchAnything if we encounter any pseudo element inside a functional pseudo class (and update SelectorChecker accordingly)?

This looks nice to me.
Currently since there's no standard behavior, so taking conservative behavior (making it CannotMatchAnything) is nice I think.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2826
>> +        //         return SelectorFailsCompletely;
> 
> This comment is a bit misleading. 
> What you generate here is huge, it means we take a big shortcut in the evaluation.
> 
> You could probably do a little explanation of this optimization instead of the pseudo code.

Yeah! I've written the explanation about this function.

>> LayoutTests/fast/selectors/pseudo-element-inside-any-expected.html:15
>> +            <p class="target" id="target2" style="background-color: lime">Background color becomes lime.</p>
> 
> Isn't this showing :-webkit-any(::first-letter) is completely broken? The pseudo element is completely ignored :(

Now, I've changed to make pseudo elements inside functional pseudo classes CannotMatchAnything, it becomes unmatched.

[Yusuke Suzuki]

Created attachment 235413 [details]
Patch

[Yusuke Suzuki]

Created attachment 235414 [details]
Patch

[Yusuke Suzuki]

Created attachment 235415 [details]
Patch

[Yusuke Suzuki]

Oops, I've found an issue. Fix it and append the test case.

[Yusuke Suzuki]

Created attachment 235419 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 235419 [details]
Patch

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

Added comments

> LayoutTests/fast/selectors/pseudo-element-inside-any-expected.html:21
> +            <p class="target" id="target3" style="background-color:lime">pseudo element inside :any has no effect and the other selectors are matched.</p>

This is not the same behavior to Firefox.
In Firefox, when there's pseudo-element inside functional pseudo class (:-moz-any), it becomes global failure.

But I think since we use :-webkit-*any*, if there's any selector that matches to this element, the selector should be matched. Do you think about this?

[Yusuke Suzuki]

Comment on attachment 235419 [details]
Patch

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

>> LayoutTests/fast/selectors/pseudo-element-inside-any-expected.html:21
>> +            <p class="target" id="target3" style="background-color:lime">pseudo element inside :any has no effect and the other selectors are matched.</p>
> 
> This is not the same behavior to Firefox.
> In Firefox, when there's pseudo-element inside functional pseudo class (:-moz-any), it becomes global failure.
> 
> But I think since we use :-webkit-*any*, if there's any selector that matches to this element, the selector should be matched. Do you think about this?

Seeing the gecko code, it is treated as a selector parsing error.

[Benjamin Poulain]

(In reply to comment #24)
> (From update of attachment 235419 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235419&action=review
> 
> >> LayoutTests/fast/selectors/pseudo-element-inside-any-expected.html:21
> >> +            <p class="target" id="target3" style="background-color:lime">pseudo element inside :any has no effect and the other selectors are matched.</p>
> > 
> > This is not the same behavior to Firefox.
> > In Firefox, when there's pseudo-element inside functional pseudo class (:-moz-any), it becomes global failure.
> > 
> > But I think since we use :-webkit-*any*, if there's any selector that matches to this element, the selector should be matched. Do you think about this?
> 
> Seeing the gecko code, it is treated as a selector parsing error.

I agree with you, a local failure would be more useful than a parsing error. It is also more consistent with other kind of failures for pseudo elements.

[Benjamin Poulain]

Comment on attachment 235419 [details]
Patch

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

Brilliant change. I love how good this is compared to SelectorChecker, it is both simpler and more efficient.

> Source/WebCore/css/ElementRuleCollector.cpp:305
> +        // FIXME: Currently a compiled selector doesn't support scrollbar / selection's exceptional case.

Good news! Alex is looking into that! :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:518
> +
> +                // In Selectors Level 4, a pseudo element inside a functional pseudo class is undefined (issue 7).
> +                // Make it as matching failure until the spec clarifies this case.
> +                // So don't insert it to anyFragments.
> +                if (subFragment.pseudoElementSelector)
> +                    continue;
> +

I believe it would make sense to do this check this in constructFragments() to have :not() and :-webkit-any() behave the same way.

> LayoutTests/ChangeLog:13
> +        * fast/selectors/pseudo-element-inside-any-expected.html: Added.
> +        * fast/selectors/pseudo-element-inside-any.html: Added.

It would be good to explain the new behavior of :-webkit-any() here.

[Yusuke Suzuki]

Comment on attachment 235419 [details]
Patch

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

Thank you!

>> Source/WebCore/css/ElementRuleCollector.cpp:305
>> +        // FIXME: Currently a compiled selector doesn't support scrollbar / selection's exceptional case.
> 
> Good news! Alex is looking into that! :)

Great!

>> Source/WebCore/cssjit/SelectorCompiler.cpp:518
>> +
> 
> I believe it would make sense to do this check this in constructFragments() to have :not() and :-webkit-any() behave the same way.

OK, I'll move it.

>> LayoutTests/ChangeLog:13
>> +        * fast/selectors/pseudo-element-inside-any.html: Added.
> 
> It would be good to explain the new behavior of :-webkit-any() here.

Agreed, I'll explain here.

[Yusuke Suzuki]

Committed r171588: <http://trac.webkit.org/changeset/171588>