Don't add siblingRules with combinator left of ::content.

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}

[rune, 2016-02-10 13:57:26]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-10 13:57:59]

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

[rune, 2016-02-10 13:58:50]

ptal

[commit-bot: I haz the power, 2016-02-10 15:22:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-10 15:22:57]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-02-15 02:31:02]

On 2016/02/10 15:22:57, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

kochi@, could you review this CL?

I think you (or Rune?) will consider the similar thing for "::slotted".

[kochi, 2016-02-15 03:40:29]

On 2016/02/15 02:31:02, hayato wrote:
> On 2016/02/10 15:22:57, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> kochi@, could you review this CL?
> 
> I think you (or Rune?) will consider the similar thing for "::slotted".

Sorry for the delay.  I'm starting review now.

[kochi, 2016-02-15 04:10:17]

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if
(!metadata.foundInsertionPointCrossing && current->isSiblingSelector())
If foundInsertionPointCrossing is only used for the flag that indicate
::content is found in the loop, could it be a local boolean flag instead
of a FeatureMetadata member?  I don't see any other usage for this flag
in this CL.

If you have some reason to keep it in FeatureMetadata, probably you need to
set it false in FeatureMetadata::clear() below.

[rune, 2016-02-15 07:27:04]

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if
(!metadata.foundInsertionPointCrossing && current->isSiblingSelector())
On 2016/02/15 04:10:17, kochi wrote:
> If foundInsertionPointCrossing is only used for the flag that indicate
> ::content is found in the loop, could it be a local boolean flag instead
> of a FeatureMetadata member?  I don't see any other usage for this flag
> in this CL.

It's passed recursively down to selectorList below. Note that
isSiblingSelector() also covers :nth-*, :only-*, etc. So for instance for:

:not(:nth-child(even)) ::content :not(:only-child) {}

the flag needs to be passed to collectFeaturesFromSelector() for the inner
selectors of :not and be different in those cases. But, I could use a bool
parameter to collectFeaturesFromSelector instead?

> If you have some reason to keep it in FeatureMetadata, probably you need to
> set it false in FeatureMetadata::clear() below.

Good catch, but it probably makes more sense to add a bool parameter to
collectFeaturesFromSelector instead as the flag is not used after the
collection. wdyt?

[rune, 2016-02-15 08:04:19]

On 2016/02/15 07:27:04, rune wrote:

> > If you have some reason to keep it in FeatureMetadata, probably you need to
> > set it false in FeatureMetadata::clear() below.
> 
> Good catch, but it probably makes more sense to add a bool parameter to
> collectFeaturesFromSelector instead as the flag is not used after the
> collection. wdyt?

Actually, we need to return the value out to collectFeaturesFromRuleData as
well, so I'm leaning towards keeping it with the other results in
FeatureMetaData.

[rune, 2016-02-15 08:12:07]

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if
(!metadata.foundInsertionPointCrossing && current->isSiblingSelector())
On 2016/02/15 04:10:17, kochi wrote:

> If you have some reason to keep it in FeatureMetadata, probably you need to
> set it false in FeatureMetadata::clear() below.

Done.

[kochi, 2016-02-15 10:10:57]

lgtm

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1683923003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.cpp:590: if
(!metadata.foundInsertionPointCrossing && current->isSiblingSelector())
On 2016/02/15 07:27:04, rune wrote:
> On 2016/02/15 04:10:17, kochi wrote:
> > If foundInsertionPointCrossing is only used for the flag that indicate
> > ::content is found in the loop, could it be a local boolean flag instead
> > of a FeatureMetadata member?  I don't see any other usage for this flag
> > in this CL.
> 
> It's passed recursively down to selectorList below. Note that
> isSiblingSelector() also covers :nth-*, :only-*, etc. So for instance for:
> 
> :not(:nth-child(even)) ::content :not(:only-child) {}
> 
> the flag needs to be passed to collectFeaturesFromSelector() for the inner
> selectors of :not and be different in those cases. But, I could use a bool
> parameter to collectFeaturesFromSelector instead?
> 
> > If you have some reason to keep it in FeatureMetadata, probably you need to
> > set it false in FeatureMetadata::clear() below.
> 
> Good catch, but it probably makes more sense to add a bool parameter to
> collectFeaturesFromSelector instead as the flag is not used after the
> collection. wdyt?

Thanks for explanation.  I overlooked that it is passed down recursively.

As you already resolved, it looks like keeping the flag in FeatureMetadata makes
sense.

[rune, 2016-02-15 10:20:19]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-02-15 10:20:46]

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

[commit-bot: I haz the power, 2016-02-15 13:40:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-16 22:49:57]

Description was changed from

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

to

==========
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}
==========

[commit-bot: I haz the power, 2016-02-16 22:49:57]

Patchset 2 (id:??) landed as
https://crrev.com/87e6a478af25c7976849f5bc8cf88d16cdcd635c
Cr-Commit-Position: refs/heads/master@{#375454}