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

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