Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(442)

Issue 1683923003: Don't add siblingRules with combinator left of ::content. (Closed)

Created:
4 years, 10 months ago by rune
Modified:
4 years, 10 months ago
Reviewers:
esprehn, hayato, kochi
CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't add siblingRules with combinator left of ::content. RuleFeatureSet::siblingRules are collected because we need to skip style sharing between elements which do not match the same set of sibling selectors. Style sharing only happens if two elements' parent chain of computed styles are common. That means that descendants of their common ancestor also need to share style. Due to this check in SharedStyleFinder::canShareStyleWithElement: if (!sharingCandidateDistributedToSameInsertionPoint(candidate)) return false; Elements with distributed nodes in their ancestor chain need to be distributed to the same insertion points in order to share style. That is, two elements which may share style match the same insertion points for all selectors containing the ::content pseudo. Which also means that any sibling selectors left of any ::content pseudo will match the same sibling combinations for all elements which may share style. Hence, we don't need to reject style sharing based on selectors like: .a + .b ::content .c because the sharing would already be rejected because of insertion point mismatch in the ancestor chain. This CL skips adding rules to RuleFeatureSet::siblingRules if we have seen a ::content combinator before we see any sibling selectors. We currently rejecting style sharing for slots checking isChildOfV1ShadowHost() in Element::supportsStyleSharing(), so we could have skipped adding any of the ::slotted rules to siblingRules. I'm not doing that here as that might change since ::slotted is work in progress. If continue to skip style sharing past different slots, this means we will only need to consider siblingRules from the same TreeScope when for Shadow DOM V1, which means we don't need a global set of siblingRules. R=kochi@chromium.org,hayato@chromium.org,esprehn@chromium.org BUG=401359 Committed: https://crrev.com/87e6a478af25c7976849f5bc8cf88d16cdcd635c Cr-Commit-Position: refs/heads/master@{#375454}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Missing clear #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -17 lines) Patch
M third_party/WebKit/Source/core/css/RuleFeature.h View 1 chunk +5 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeature.cpp View 1 2 chunks +8 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeatureSetTest.cpp View 3 chunks +68 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (4 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683923003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683923003/1
4 years, 10 months ago (2016-02-10 13:57:59 UTC) #2
rune
ptal
4 years, 10 months ago (2016-02-10 13:58:50 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-10 15:22:57 UTC) #5
hayato
On 2016/02/10 15:22:57, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
4 years, 10 months ago (2016-02-15 02:31:02 UTC) #6
kochi
On 2016/02/15 02:31:02, hayato wrote: > On 2016/02/10 15:22:57, commit-bot: I haz the power wrote: ...
4 years, 10 months ago (2016-02-15 03:40:29 UTC) #7
kochi
https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp File third_party/WebKit/Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp#newcode590 third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if (!metadata.foundInsertionPointCrossing && current->isSiblingSelector()) If foundInsertionPointCrossing is only used ...
4 years, 10 months ago (2016-02-15 04:10:17 UTC) #8
rune
https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp File third_party/WebKit/Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp#newcode590 third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if (!metadata.foundInsertionPointCrossing && current->isSiblingSelector()) On 2016/02/15 04:10:17, kochi wrote: ...
4 years, 10 months ago (2016-02-15 07:27:04 UTC) #9
rune
On 2016/02/15 07:27:04, rune wrote: > > If you have some reason to keep it ...
4 years, 10 months ago (2016-02-15 08:04:19 UTC) #10
rune
https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp File third_party/WebKit/Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp#newcode590 third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if (!metadata.foundInsertionPointCrossing && current->isSiblingSelector()) On 2016/02/15 04:10:17, kochi wrote: ...
4 years, 10 months ago (2016-02-15 08:12:07 UTC) #11
kochi
lgtm https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp File third_party/WebKit/Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/core/css/RuleFeature.cpp#newcode590 third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if (!metadata.foundInsertionPointCrossing && current->isSiblingSelector()) On 2016/02/15 07:27:04, rune ...
4 years, 10 months ago (2016-02-15 10:10:57 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683923003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683923003/20001
4 years, 10 months ago (2016-02-15 10:20:46 UTC) #14
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 10 months ago (2016-02-15 13:40:01 UTC) #15
commit-bot: I haz the power
4 years, 10 months ago (2016-02-16 22:49:57 UTC) #17
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/87e6a478af25c7976849f5bc8cf88d16cdcd635c
Cr-Commit-Position: refs/heads/master@{#375454}

Powered by Google App Engine
This is Rietveld 408576698