Bug 96990 – Make ContentSelectorQuery work when siblings are passed explicitly.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

As the first step of Bug 96988, we would like to refactor ContentSelectorQuery when parent and children are passed explicitly.

[Shinya Kawanaka]

Created attachment 164686 [details]
Patch

[Shinya Kawanaka]

Created attachment 164689 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 164689 [details]
Patch

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?

[Shinya Kawanaka]

Created attachment 164868 [details]
WIP

[Shinya Kawanaka]

Maybe we can remove ReifiedElement from the WIP patch.

[WebKit Review Bot]

Comment on attachment 164868 [details]
WIP

Attachment 164868 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13951166

New failing tests:
css3/selectors3/html/css3-modsel-36.html
css3/css3-modsel-36.html
css3/selectors3/html/css3-modsel-81.html

[Shinya Kawanaka]

Created attachment 164899 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 164899 [details]
Patch

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

[Shinya Kawanaka]

The refactoring will be done in Bug 97298

[Hayato Ito]

Created attachment 165057 [details]
wip. including other patches

[Hayato Ito]

webkit-patch seems to upload the patch to the unintentional bug. Please ignore that.

(In reply to comment #10)
> Created an attachment (id=165057) [details]
> wip. including other patches

[Shinya Kawanaka]

Created attachment 165525 [details]
WIP including other patches

[Shinya Kawanaka]

Created attachment 165554 [details]
WIP including other patches

[Build Bot]

Comment on attachment 165554 [details]
WIP including other patches

Attachment 165554 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13997923

[Shinya Kawanaka]

Created attachment 165561 [details]
WIP including other patches

[Hayato Ito]

Created attachment 167079 [details]
Rebase on dependent patch https://bugs.webkit.org/attachment.cgi?id=167073

[Hayato Ito]

Created attachment 167256 [details]
Rebase

[Dimitri Glazkov (Google)]

Comment on attachment 167256 [details]
Rebase

OMG, this patch is so tremendously ugly, I want to cry. The adding of the base class (!!!) for a simple struct like SelectorCheckingContext, the slopping around of the shadow DOM-specific logic in SelectorChecker, the extra dependencies of headers from html/shadow.. Ugh.

Folks, we _must_ find a better way. We can't land this. I'll help you next week.

[Shinya Kawanaka]

(In reply to comment #18)
> (From update of attachment 167256 [details])
> OMG, this patch is so tremendously ugly, I want to cry. The adding of the base class (!!!) for a simple struct like SelectorCheckingContext, the slopping around of the shadow DOM-specific logic in SelectorChecker, the extra dependencies of headers from html/shadow.. Ugh.
> 
> Folks, we _must_ find a better way. We can't land this. I'll help you next week.

I'm back.

Now that the number of arguments of checkOneSelector is 1, we might be able to have 2 arguments without performance regression. When the number of arguments was 3, I detected 10% performance regression, though. Let me try.

[Shinya Kawanaka]

When we make checkOneSelector a template and we cannot place ShadowDOM related things in SelectorChecker.cpp, we have to move checkOneSelector to header file in the current design. Since we have to instantiate checkOneSelector for ShadowDOM.

Of course we don't want to do this, though...

[Shinya Kawanaka]

Or, we create a new header like dom/ContainerNodeAlgorithm.h

[Shinya Kawanaka]

Created attachment 167745 [details]
WIP

[Gyuyoung Kim]

Comment on attachment 167745 [details]
WIP

Attachment 167745 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14246004

[Dimitri Glazkov (Google)]

Comment on attachment 167745 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=167745&action=review

This patch is marked improvement.

> Source/WebCore/css/SelectorChecker.cpp:1369
> +bool SelectorChecker::checkOneSelectorWith(const SelectorChecker::SelectorCheckingContext&, const ShadowDOMTraversalStrategy&) const;

"With" seems out of place. I understand what you mean here, but try looking at this code out of context of this patch? WithStrategy?

> Source/WebCore/css/SelectorChecker.h:90
> +    bool checkOneSelector(const SelectorCheckingContext&) const;

This seems like an improvement.

> Source/WebCore/css/SelectorCheckerStrategy.h:29
> +#ifndef SelectorCheckerStrategy_h

It's not a strategy for checking selectors, right? So the naming is a bit off. Maybe SiblingTraversalStrategies?

[Shinya Kawanaka]

Created attachment 167976 [details]
WIP

[Shinya Kawanaka]

We cannot test content reprojection only with this patch. We need hayato's patch.

[Gyuyoung Kim]

Comment on attachment 167976 [details]
WIP

Attachment 167976 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14238679

[Build Bot]

Comment on attachment 167976 [details]
WIP

Attachment 167976 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14238698

[WebKit Review Bot]

Comment on attachment 167976 [details]
WIP

Attachment 167976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14255169

New failing tests:
fast/dom/shadow/content-reprojection.html

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> We cannot test content reprojection only with this patch. We need hayato's patch.

That's ok. We can land the test with hayato's patch or even separately.

[Dimitri Glazkov (Google)]

Comment on attachment 167976 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=167976&action=review

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:194
> +bool RuntimeEnabledFeatures::isContentReprojectionEnabled = false;

I don't think we need this flag. We're nearly there and there is no content on the Web that would depend on old behavior.

[Shinya Kawanaka]

Created attachment 168178 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #31)
> (From update of attachment 167976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167976&action=review
> 
> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:194
> > +bool RuntimeEnabledFeatures::isContentReprojectionEnabled = false;
> 
> I don't think we need this flag. We're nearly there and there is no content on the Web that would depend on old behavior.

Removed.

[WebKit Review Bot]

Comment on attachment 168178 [details]
Patch

Clearing flags on attachment: 168178

Committed r131068: <http://trac.webkit.org/changeset/131068>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.