[webkit-reviews] review denied: [Bug 135242] compile scrollbar pseudoclass css selectors : [Attachment 235717] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 135242: compile scrollbar pseudoclass css selectors
https://bugs.webkit.org/show_bug.cgi?id=135242

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235717&action=review


The JIT part looks great.

Can you please also add ref-tests for Resizer+window inactive?

We should talk about mixing those pseudo elements with other elements when you
have 5 minutes. We may want to improve the RuleCollector's partitioning.

> Source/WebCore/css/ElementRuleCollector.cpp:-296
> -	       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.");

Ok, I know how to fix this:

We could assert than if the selector checker match, we must have that
m_pseudoStyleRequest.pseudoId == NOPSEUDO.

> Source/WebCore/css/SelectorChecker.cpp:481
>	   } else if (context.hasScrollbarPseudo) {

This can be executed for RESIZER. In that case, context.scrollbar could be
null, and you would break PseudoClassWindowInactive.
Can you please also add a test for that?

> Source/WebCore/css/SelectorCheckerTestFunctions.h:167
> +template <class ContextType>

For genericity, we generally use "typename" instead of "class" for templates.

We could remove "element" and "scope" from the context so that it works the
same way in both implementations.

> Source/WebCore/cssjit/SelectorCompiler.cpp:687
> +	   if (relation == CSSSelector::SubSelector
> +	       && fragment.pseudoElementSelector
> +	       &&
!isScrollbarPseudoElement(fragment.pseudoElementSelector->pseudoElementType()))
{
> +	       return FunctionType::CannotMatchAnything;
> +	   }

This is straightforward and it looks correct.

What we have in SelectorChecker is a mess and it is hard to tell if it is
correct (I actually somewhat doubt it is). Can you please rewrite
SelectorChecker's SubSelector handling to have the exact same logic? (you can
make SelectorChecker as inefficient as you want to make it equivalent, at this
point it really does not matter).