Bug 96990

Created attachment 164686 [details] Patch

Created attachment 164689 [details] Patch

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?

Created attachment 164868 [details] WIP

Maybe we can remove ReifiedElement from the WIP patch.

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

Created attachment 164899 [details] Patch

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

The refactoring will be done in Bug 97298

Created attachment 165057 [details] wip. including other patches

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

Created attachment 165525 [details] WIP including other patches

Created attachment 165554 [details] WIP including other patches

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

Created attachment 165561 [details] WIP including other patches

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

Created attachment 167256 [details] Rebase

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.

(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.

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...

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

Created attachment 167745 [details] WIP

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

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?

Created attachment 167976 [details] WIP

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

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

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

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

(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.

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.

Created attachment 168178 [details] Patch

(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.

Comment on attachment 168178 [details] Patch Clearing flags on attachment: 168178 Committed r131068: <http://trac.webkit.org/changeset/131068>

All reviewed patches have been landed. Closing bug.