WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(20.05 KB, patch)
2012-09-19 01:22 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(28.55 KB, patch)
2012-09-20 02:32 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(26.38 KB, patch)
2012-09-20 05:48 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
wip. including other patches
(23.07 KB, patch)
2012-09-20 23:55 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
WIP including other patches
(34.52 KB, patch)
2012-09-24 22:52 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP including other patches
(33.99 KB, patch)
2012-09-25 01:29 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP including other patches
(34.93 KB, patch)
2012-09-25 02:17 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Rebase
(16.85 KB, patch)
2012-10-04 22:58 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
WIP
(32.69 KB, patch)
2012-10-09 06:14 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(46.52 KB, patch)
2012-10-10 04:07 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(28.37 KB, patch)
2012-10-11 02:41 PDT
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1
2012-09-19 01:15:52 PDT
Created
attachment 164686
[details]
Patch
Shinya Kawanaka
Comment 2
2012-09-19 01:22:31 PDT
Created
attachment 164689
[details]
Patch
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
Created
attachment 164868
[details]
WIP
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
Created
attachment 164899
[details]
Patch
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
Created
attachment 167079
[details]
Rebase on dependent patch
https://bugs.webkit.org/attachment.cgi?id=167073
Hayato Ito
Comment 17
2012-10-04 22:58:05 PDT
Created
attachment 167256
[details]
Rebase
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
Created
attachment 167745
[details]
WIP
Gyuyoung Kim
Comment 23
2012-10-09 06:22:45 PDT
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 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
Created
attachment 167976
[details]
WIP
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
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 28
2012-10-10 05:33:10 PDT
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 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
Created
attachment 168178
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug