RESOLVED FIXED 96990
Make ContentSelectorQuery work when siblings are passed explicitly.
https://bugs.webkit.org/show_bug.cgi?id=96990
Summary Make ContentSelectorQuery work when siblings are passed explicitly.
Shinya Kawanaka
Reported 2012-09-18 00:50:54 PDT
As the first step of Bug 96988, we would like to refactor ContentSelectorQuery when parent and children are passed explicitly.
Attachments
Patch (20.12 KB, patch)
2012-09-19 01:15 PDT, Shinya Kawanaka
no flags
Patch (20.05 KB, patch)
2012-09-19 01:22 PDT, Shinya Kawanaka
no flags
WIP (28.55 KB, patch)
2012-09-20 02:32 PDT, Shinya Kawanaka
no flags
Patch (26.38 KB, patch)
2012-09-20 05:48 PDT, Shinya Kawanaka
no flags
wip. including other patches (23.07 KB, patch)
2012-09-20 23:55 PDT, Hayato Ito
no flags
WIP including other patches (34.52 KB, patch)
2012-09-24 22:52 PDT, Shinya Kawanaka
no flags
WIP including other patches (33.99 KB, patch)
2012-09-25 01:29 PDT, Shinya Kawanaka
no flags
WIP including other patches (34.93 KB, patch)
2012-09-25 02:17 PDT, Shinya Kawanaka
no flags
Rebase on dependent patch https://bugs.webkit.org/attachment.cgi?id=167073 (16.86 KB, patch)
2012-10-04 05:22 PDT, Hayato Ito
no flags
Rebase (16.85 KB, patch)
2012-10-04 22:58 PDT, Hayato Ito
no flags
WIP (32.69 KB, patch)
2012-10-09 06:14 PDT, Shinya Kawanaka
no flags
WIP (46.52 KB, patch)
2012-10-10 04:07 PDT, Shinya Kawanaka
no flags
Patch (28.37 KB, patch)
2012-10-11 02:41 PDT, Shinya Kawanaka
no flags
Shinya Kawanaka
Comment 1 2012-09-19 01:15:52 PDT
Shinya Kawanaka
Comment 2 2012-09-19 01:22:31 PDT
Dimitri Glazkov (Google)
Comment 3 2012-09-19 09:25:11 PDT
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
Comment 4 2012-09-20 02:32:07 PDT
Shinya Kawanaka
Comment 5 2012-09-20 02:32:32 PDT
Maybe we can remove ReifiedElement from the WIP patch.
WebKit Review Bot
Comment 6 2012-09-20 04:22:41 PDT
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
Comment 7 2012-09-20 05:48:20 PDT
Dimitri Glazkov (Google)
Comment 8 2012-09-20 10:16:27 PDT
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
Comment 9 2012-09-20 23:44:37 PDT
The refactoring will be done in Bug 97298
Hayato Ito
Comment 10 2012-09-20 23:55:16 PDT
Created attachment 165057 [details] wip. including other patches
Hayato Ito
Comment 11 2012-09-20 23:56:52 PDT
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
Comment 12 2012-09-24 22:52:37 PDT
Created attachment 165525 [details] WIP including other patches
Shinya Kawanaka
Comment 13 2012-09-25 01:29:24 PDT
Created attachment 165554 [details] WIP including other patches
Build Bot
Comment 14 2012-09-25 01:37:38 PDT
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
Comment 15 2012-09-25 02:17:44 PDT
Created attachment 165561 [details] WIP including other patches
Hayato Ito
Comment 16 2012-10-04 05:22:20 PDT
Hayato Ito
Comment 17 2012-10-04 22:58:05 PDT
Dimitri Glazkov (Google)
Comment 18 2012-10-05 09:22:07 PDT
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
Comment 19 2012-10-08 22:04:24 PDT
(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
Comment 20 2012-10-08 23:55:19 PDT
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
Comment 21 2012-10-08 23:57:33 PDT
Or, we create a new header like dom/ContainerNodeAlgorithm.h
Shinya Kawanaka
Comment 22 2012-10-09 06:14:58 PDT
Gyuyoung Kim
Comment 23 2012-10-09 06:22:45 PDT
Dimitri Glazkov (Google)
Comment 24 2012-10-09 14:22:47 PDT
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
Comment 25 2012-10-10 04:07:02 PDT
Shinya Kawanaka
Comment 26 2012-10-10 04:09:50 PDT
We cannot test content reprojection only with this patch. We need hayato's patch.
Gyuyoung Kim
Comment 27 2012-10-10 04:21:54 PDT
Build Bot
Comment 28 2012-10-10 05:33:10 PDT
WebKit Review Bot
Comment 29 2012-10-10 06:04:13 PDT
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)
Comment 30 2012-10-10 10:22:11 PDT
(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 31 2012-10-10 10:54:26 PDT
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
Comment 32 2012-10-11 02:41:22 PDT
Shinya Kawanaka
Comment 33 2012-10-11 03:34:31 PDT
(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 34 2012-10-11 09:10:42 PDT
Comment on attachment 168178 [details] Patch Clearing flags on attachment: 168178 Committed r131068: <http://trac.webkit.org/changeset/131068>
WebKit Review Bot
Comment 35 2012-10-11 09:10:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.