[webkit-reviews] review denied: [Bug 134835] CSS JIT: Implement Pseudo Element : [Attachment 235063] 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 134835: CSS JIT: Implement Pseudo Element
https://bugs.webkit.org/show_bug.cgi?id=134835

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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::generateRequestedPseudoElementEqualsToSelectorPseudoElem
ent(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 :(