[webkit-reviews] review denied: [Bug 96990] Make ContentSelectorQuery work when parent and children are passed explicitly. : [Attachment 164899] 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 164899: Patch
https://bugs.webkit.org/attachment.cgi?id=164899&action=review

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


This looks pretty cool, except:

1) can you split up the SelectorChecker patch out and let's work on landing
this first
2) measure SelectorChecker patch for possible performance regression. This is a
sensitive part of WebKit machinery?

Also:

> Source/WebCore/css/SelectorChecker.cpp:33
> +#include "ContentSelectorQuery.h"

This seems like a backward dependency. Why is this needed?

> Source/WebCore/css/SelectorChecker.cpp:1426
> +template
> +bool
SelectorChecker::checkOneSelectorWithStrategy<ContentSelectorCheckingStrategy>(
const ContentSelectorCheckingStrategy&, const SelectorCheckingContext&,
PseudoId&, bool&) const;

Why does this need to live here? WebCore/css stuff knowing about
WebCore/html/shadow seems wrong.

> Source/WebCore/css/SelectorChecker.h:154
> +class DefaultSelectorCheckingStrategy {

Can this be SelectorChecker::DOMTraversalStrategy?

> Source/WebCore/html/shadow/ContentSelectorQuery.h:64
> +class ContentSelectorCheckingStrategy {

... and this now becomes ShadowDOMTraversalStrategy