Skip child ShadowRoots when invalidation does not have boundary crossing rules

Skip child ShadowRoots when invalidation does not have boundary crossing rules

When the only things that changed are not tree boundary crossing rules we
can skip invalidation in descendant ShadowRoots. This means that
if you have lots of nested components with duplicate class names then using
a descendant selector in an enclosing component won't invalidate style in
nested ShadowRoots.

I also packed the booleans on DescendantInvalidationSet since we keep growing
it with new flags.

In the future we can make this tree boundary crossing logic even smarter so that
if you have both tree boundary and not tree boundary crossing invalidation
sets on the same subtree we can skip running the non-tree boundary crossing
rules when recursing into a ShadowRoot.

I also took this as an opportunity to clean up the addFeaturesToInvalidationSets
logic and merged the new treeBoundaryCrossing flag and the wholeSubtreeInvalid
flag into the InvalidationSetFeatures.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175171

[chrishtr, 2014-05-29 20:54:07]

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:273: features.wholeSubtree = false;
Why this change?

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:278: features.wholeSubtree = false;
I don't get why it's ok to set wholeSubtree to false here.

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:76: return;
Move tree boundary crossing before this early return conditional.

[esprehn, 2014-05-29 21:07:30]

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:273: features.wholeSubtree = false;
On 2014/05/29 20:54:07, chrishtr wrote:
> Why this change?

Because I can't handle all 4 types of relations the same way anymore.

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:278: features.wholeSubtree = false;
On 2014/05/29 20:54:07, chrishtr wrote:
> I don't get why it's ok to set wholeSubtree to false here.

We always did that, see the old code. I just split the two cases apart so that
ShadowPseudo/ShadowDeep set both wholeSubtree and treeBoundaryCrossing, and
DirectAdjacent/IndirectAdjacent only set wholeSubtree.

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:76: return;
On 2014/05/29 20:54:07, chrishtr wrote:
> Move tree boundary crossing before this early return conditional.

That would make us traverse too far down the tree when wholeSubtreeInvalid is
true, but I should probably fix that in a separate patch. I'll move this.

[chrishtr, 2014-05-29 21:43:43]

lgtm

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:273: features.wholeSubtree = false;
On 2014/05/29 21:07:30, esprehn wrote:
> On 2014/05/29 20:54:07, chrishtr wrote:
> > Why this change?
> 
> Because I can't handle all 4 types of relations the same way anymore.

Ah right, fallthrough.

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:278: features.wholeSubtree = false;
On 2014/05/29 21:07:30, esprehn wrote:
> On 2014/05/29 20:54:07, chrishtr wrote:
> > I don't get why it's ok to set wholeSubtree to false here.
> 
> We always did that, see the old code. I just split the two cases apart so that
> ShadowPseudo/ShadowDeep set both wholeSubtree and treeBoundaryCrossing, and
> DirectAdjacent/IndirectAdjacent only set wholeSubtree.

Oh right.

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:278: features.wholeSubtree = false;
On 2014/05/29 21:07:30, esprehn wrote:
> On 2014/05/29 20:54:07, chrishtr wrote:
> > I don't get why it's ok to set wholeSubtree to false here.
> 
> We always did that, see the old code. I just split the two cases apart so that
> ShadowPseudo/ShadowDeep set both wholeSubtree and treeBoundaryCrossing, and
> DirectAdjacent/IndirectAdjacent only set wholeSubtree.

Ok.

https://codereview.chromium.org/286903024/diff/70001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/70001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:229: if (lastSelector->relation() ==
CSSSelector::ShadowPseudo || lastSelector->relation() ==
CSSSelector::ShadowDeep)
Changing these to a switch statement would be a little cleaner.

[esprehn, 2014-05-30 20:49:03]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:51:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/esprehn@chromium.org/286903024/140001

[rune, 2014-05-30 21:02:44]

lgtm

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:278: features.wholeSubtree = false;
On 2014/05/29 21:07:30, esprehn wrote:
> On 2014/05/29 20:54:07, chrishtr wrote:
> > I don't get why it's ok to set wholeSubtree to false here.
> 
> We always did that, see the old code. I just split the two cases apart so that
> ShadowPseudo/ShadowDeep set both wholeSubtree and treeBoundaryCrossing, and
> DirectAdjacent/IndirectAdjacent only set wholeSubtree.

FTR, it's because these shadow combinators are like descendant combinators, and
wholeSubtree in this context is about adjacent combinators requiring
wholeSubtree because the features of the rightmost compound selector are not of
a descendant. That is however canceled out as encountering a descendant-like
combinator will make the rightmost compound selector start matching a descendant
again.

.a /deep/ .b + .c // .c not a descendant of .b, but .c a descendant of .a

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:217: for (const CSSSelector* lastSelector =
&selector;; lastSelector; lastSelector = lastSelector->tagHistory()) {
The double ;; is what gave you compile problems, I suppose.

Oh. Fixed in next patch set.

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:162: bool wholeSubtree;

Me likes the way this made the .cpp code more readable.

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/invalid...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/286903024/diff/110001/Source/core/css/invalid...
Source/core/css/invalidation/StyleInvalidator.cpp:121: continue;

Just an idea for further improvements:

If treeBoundaryCrossing is false, and something in the shadow tree needs
invalidation, we can continue with an empty set of invalidation sets here. For
instance by introducing a start index in addition to invalidationSetsSize in
RecursionCheckpoint. I see now you mentioned something along those lines in the
description.

[commit-bot: I haz the power, 2014-05-30 23:20:20]

Change committed as 175171