Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496123002:
Matching part for >>> (shadow-piercing descendant combinator). (Closed)
DescriptionMatching part for >>> (shadow-piercing descendant combinator).
Parsing part is done in the previous dependent CL.
https://codereview.chromium.org/2500813003/
This CL implements matching '>>>' shadow-piercing descendant
combinator in static profile.
Note that the combinator only pierces through open shadow roots
and not V0 or closed shadow roots.
BUG=633007
Committed: https://crrev.com/04a4852c265670de8100ceee3e20215f4a56091f
Cr-Commit-Position: refs/heads/master@{#433457}
Patch Set 1 : wip #Patch Set 2 : wip2 #Patch Set 3 : rebase #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : address rune's comments. #
Total comments: 4
Patch Set 6 : Update comment as Rune suggested. #
Total comments: 2
Messages
Total messages: 36 (27 generated)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Matching part for >>> (deep descendant combinator). Matching '>>>' deep descendant combinator in static profile. Note that the combinator only pierces through V1 shadow roots and not V0 shadow roots. BUG=633007 ========== to ========== Matching part for >>> (deep descendant combinator). Matching '>>>' deep descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ==========
kochi@chromium.org changed reviewers: + hayato@chromium.org, rune@opera.com
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Matching part for >>> (deep descendant combinator). Matching '>>>' deep descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ========== to ========== Matching part for >>> (deep descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ Matching '>>>' deep descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Matching part for >>> (deep descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ Matching '>>>' deep descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ========== to ========== Matching part for >>> (shadow-piercing descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ This CL implements matching '>>>' shadow-piercing descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right): https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:291: return nullptr; So this can happen if you start querying inside a closed tree and try to escape during matching? <div class="a"></div> <div id="host"> <:shadow-root closed> <div id="scoping"> <div class="b"></div> </div> </:shadow-root> </div> </div> scoping.querySelector(".a >>> .b"); If you do: document.querySelector(".a >>> .b"); you could avoid traversing down the closed shadow tree and the code above would not be hit? https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:452: if (root->type() == ShadowRootType::UserAgent) Will you be able to end up here at all? Doesn't that mean the dom traversal in querySelector enter UA shadow trees? Does it? Should it?
PTAL (Rune, thanks for your early review!) https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right): https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:291: return nullptr; On 2016/11/17 10:05:25, rune wrote: > So this can happen if you start querying inside a closed tree and try to escape > during matching? > > <div class="a"></div> > <div id="host"> > <:shadow-root closed> > <div id="scoping"> > <div class="b"></div> > </div> > </:shadow-root> > </div> > </div> > > scoping.querySelector(".a >>> .b"); > > If you do: > > document.querySelector(".a >>> .b"); > > you could avoid traversing down the closed shadow tree and the code above would > not be hit? We filter out closed and user-agent shadow roots in SelectorQuery.cpp, but V0 shadow roots are included, so we have to exclude V0 shadow roots here. https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:452: if (root->type() == ShadowRootType::UserAgent) On 2016/11/17 10:05:25, rune wrote: > Will you be able to end up here at all? Doesn't that mean the dom traversal in > querySelector enter UA shadow trees? Does it? Should it? Good catch, removed. (the wip version naively copied most of the parts from /deep/ matching logic.)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right): https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:291: return nullptr; On 2016/11/18 07:31:21, kochi wrote: > On 2016/11/17 10:05:25, rune wrote: > > So this can happen if you start querying inside a closed tree and try to > escape > > during matching? > > > > <div class="a"></div> > > <div id="host"> > > <:shadow-root closed> > > <div id="scoping"> > > <div class="b"></div> > > </div> > > </:shadow-root> > > </div> > > </div> > > > > scoping.querySelector(".a >>> .b"); > > > > If you do: > > > > document.querySelector(".a >>> .b"); > > > > you could avoid traversing down the closed shadow tree and the code above > would > > not be hit? > > We filter out closed and user-agent shadow roots in SelectorQuery.cpp, > but V0 shadow roots are included, so we have to exclude V0 shadow > roots here. Acknowledged. https://codereview.chromium.org/2496123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/SelectorChecker.cpp:452: if (root->type() == ShadowRootType::UserAgent) On 2016/11/18 07:31:21, kochi wrote: > On 2016/11/17 10:05:25, rune wrote: > > Will you be able to end up here at all? Doesn't that mean the dom traversal in > > querySelector enter UA shadow trees? Does it? Should it? > > Good catch, removed. > (the wip version naively copied most of the parts from /deep/ matching > logic.) Perhaps you should DCHECK that the shadow root type is not UserAgent in parentOrOpenShadowHostElement? https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/query-selector.html (right): https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/query-selector.html:6: <div id="x"><span>test2</span></div> You didn't introduce this, but using the same id multiple times should be avoided. Use class instead? https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html (right): https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html:64: }, 'left side of >>> should be in the same node tree as context object.'); "left side of >>> should be in the same" -> "leftmost compound should match an element in the same" ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review, Rune! Hayato-san, could you review? https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/query-selector.html (right): https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/query-selector.html:6: <div id="x"><span>test2</span></div> On 2016/11/18 09:16:02, rune wrote: > You didn't introduce this, but using the same id multiple times should be > avoided. Use class instead? > Looks weird to me as well, but the test comment ('querySelectorAll for multiple #Ids in a shadow tree') sounds this is intentional. Let's keep this for now. https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html (right): https://codereview.chromium.org/2496123002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html:64: }, 'left side of >>> should be in the same node tree as context object.'); On 2016/11/18 09:16:02, rune wrote: > "left side of >>> should be in the same" -> "leftmost compound should match an > element in the same" ? Done. Thanks for the suggestion.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2496123002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html (right): https://codereview.chromium.org/2496123002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html:71: Could we have one (or a few) tests for a selector which has more than one '>>>'?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kochi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com Link to the patchset: https://codereview.chromium.org/2496123002/#ps100001 (title: "Update comment as Rune suggested.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479656815721470,
"parent_rev": ["6113299a6a090b976eeca6f36b9683bf21046da7", null], "commit_rev":
["1c6beebf1507a14dd5efcd09c9068ea8eeac28bc", null]}
Message was sent while issue was closed.
Description was changed from ========== Matching part for >>> (shadow-piercing descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ This CL implements matching '>>>' shadow-piercing descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ========== to ========== Matching part for >>> (shadow-piercing descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ This CL implements matching '>>>' shadow-piercing descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Matching part for >>> (shadow-piercing descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ This CL implements matching '>>>' shadow-piercing descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 ========== to ========== Matching part for >>> (shadow-piercing descendant combinator). Parsing part is done in the previous dependent CL. https://codereview.chromium.org/2500813003/ This CL implements matching '>>>' shadow-piercing descendant combinator in static profile. Note that the combinator only pierces through open shadow roots and not V0 or closed shadow roots. BUG=633007 Committed: https://crrev.com/04a4852c265670de8100ceee3e20215f4a56091f Cr-Commit-Position: refs/heads/master@{#433457} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/04a4852c265670de8100ceee3e20215f4a56091f Cr-Commit-Position: refs/heads/master@{#433457}
Message was sent while issue was closed.
Oops, I failed to publish my comment. The follow-up CL is https://codereview.chromium.org/2523673002 https://codereview.chromium.org/2496123002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html (right): https://codereview.chromium.org/2496123002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shadow-dom/shadow-piercing-descendant-combinator.html:71: On 2016/11/18 09:50:34, hayato wrote: > Could we have one (or a few) tests for a selector which has more than one '>>>'? SGTM. I'll add another test in a separate CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
