[webkit-reviews] review denied: [Bug 96990] Make ContentSelectorQuery work when parent and children are passed explicitly. : [Attachment 164689] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 96990: Make ContentSelectorQuery work when parent and children are passed
explicitly.
https://bugs.webkit.org/show_bug.cgi?id=96990

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164689&action=review


This is a nice WIP patch, but there are severe issues that make it a pretty sad
patch for review. You can do much better! :)

> Source/WebCore/ChangeLog:18
> +	   * css/SelectorChecker.cpp:
> +	   (WebCore::SelectorChecker::checkOneSelector):
> +	   (WebCore::SelectorChecker::checkAttributeSelector): Extracted to
share the code with ContentSelectorChecker.

This is a separate patch.

> Source/WebCore/css/SelectorChecker.cpp:1274
> +bool SelectorChecker::checkAttributeSelector(CSSSelector* selector, Element*
element, bool& matched) const

Can we improve the signature a bit? The method takes a bool by-ref param, then
returns a bool value. It's pretty hard to understand what's going on.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:140
> +bool ContentSelectorChecker::checkContentSelector(CSSSelector* selector,
ReifiedElement* virtualElement) const

I suspect the awkwardness in naming here is primarily due to the somewhat
forced "is-a" relationship with SelectorChecker.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:168
> +    ASSERT(mode() == SelectorChecker::CollectingRules);

When will this ever ASSERT?

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:247
> +	       // Even though WinIE allows checked and indeterminate to
co-exist, the CSS selector spec says that
> +	       // you can't be both checked and indeterminate. We will behave
like WinIE behind the scenes and just
> +	       // obey the CSS spec here in the test for matching the pseudo.

I am very unhappy with copy-pasting code from SelectorChecker. I suspected as
much, and this comment confirmed my suspicions. We can't land this -- need to
keep looking for better solutions.

> Source/WebCore/html/shadow/ContentSelectorQuery.h:48
> +    ReifiedNode(Node* parent, const Vector<RefPtr<Node> >& children, int
nthChild);

Why do we need ReifiedNode? Our selectors will only ever match elements. We
might as well drop pretending we can match nodes.

> Source/WebCore/html/shadow/ContentSelectorQuery.h:79
> +class ContentSelectorChecker : public SelectorChecker {

Creating a sub-class relationship like this with a piece of machinery as
sensitive as SelectorChecker is extremely brittle. Can we have composition
here, instead?