Use invalidation sets when selector contains adjacent combinators.

Use invalidation sets when selector contains adjacent combinators.

When we found a direct or indirect adjacent combinator, we fell back on
using a subtree recalc for all features in the selector in question. That
is not necessary as we can resume the process of adding separate features
to the invalidation sets once we encounter a non-adjacent combinator going
leftwards.

Example:

  .a .b + .c { }

We have an empty invalidation set for 'c', 'b' currently needs to do a sub-
tree recalc to recalculate the 'c' class element when 'b' is added or
removed. However, once we see the descendant combinator left of '.b' we can
get away with adding 'c' to the invalidation set for 'a' since 'c' will
have to be in 'a's sub-tree.

Eventually, I think we could merge collectFeaturesFromSelector with
updateInvalidationSet to get rid of a pass through the selector.

R=esprehn@chromium.org
BUG=351613, 335247
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170881

[esprehn, 2014-04-03 22:58:05]

lgtm, I do wonder how much this really matters in practice. I think most
developers do .a + .b which is going to cause a subtree recalc on .a :/

https://codereview.chromium.org/224593002/diff/1/LayoutTests/fast/css/invalid...
File
LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html
(right):

https://codereview.chromium.org/224593002/diff/1/LayoutTests/fast/css/invalid...
LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html:5:
<!--
Can you put this comment not inside the <style> ? Editors do weird things with
that.

https://codereview.chromium.org/224593002/diff/1/Source/core/css/RuleFeature.cpp
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/224593002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:188: bool wholeSubtree = current->relation() ==
CSSSelector::DirectAdjacent || current->relation() ==
CSSSelector::IndirectAdjacent;
It would be nicer to merge this check into the addFeaturesToInvalidationSets
loop. If the loop was:

const CSSSelector* current = &selector;
while (current && current->tagHistory()) {
  switch (current->relation()) {
    // checks for wholeSubtree or not.
  }
  if (DescendantInvalidationSet* invalidationSet =
invalidationSetForSelector(*current)) {
    // ...
  }
}

I think that would work.

[rune, 2014-04-04 09:29:36]

https://codereview.chromium.org/224593002/diff/1/LayoutTests/fast/css/invalid...
File
LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html
(right):

https://codereview.chromium.org/224593002/diff/1/LayoutTests/fast/css/invalid...
LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html:5:
<!--
On 2014/04/03 22:58:05, esprehn wrote:
> Can you put this comment not inside the <style> ? Editors do weird things with
> that.

Oh. That was not intentional. Moved it out of <style>.

https://codereview.chromium.org/224593002/diff/1/Source/core/css/RuleFeature.cpp
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/224593002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:188: bool wholeSubtree = current->relation() ==
CSSSelector::DirectAdjacent || current->relation() ==
CSSSelector::IndirectAdjacent;
On 2014/04/03 22:58:05, esprehn wrote:
> It would be nicer to merge this check into the addFeaturesToInvalidationSets
> loop. If the loop was:
> 
> const CSSSelector* current = &selector;
> while (current && current->tagHistory()) {
>   switch (current->relation()) {
>     // checks for wholeSubtree or not.
>   }
>   if (DescendantInvalidationSet* invalidationSet =
> invalidationSetForSelector(*current)) {
>     // ...
>   }
> }
> 
> I think that would work.

The problem with that is that there is no previous selector to pick the
relation() from to set the wholeSubtree value in the case of selectorList(). I
call addFeaturesToInvalidationSets() recursively for selectorList() and the
wholeSubtree input in that case is for the last combinator that is not
SubSelector, not from the previous CSSSelector.

[rune, 2014-04-04 11:32:44]

On 2014/04/03 22:58:05, esprehn wrote:
> lgtm, I do wonder how much this really matters in practice. I think most
> developers do .a + .b which is going to cause a subtree recalc on .a :/

I did a dump of all selectors with a non-adjacent combinator left of an adjacent
combinator for a selection of sites:

http://pastebin.com/cfPtS1Hm

It's quite common, it seems. The question is how often the classes/ids are
actually modified, and for which part of the selector.

[rune, 2014-04-04 11:34:42]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-04 11:34:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/224593002/20001

[commit-bot: I haz the power, 2014-04-04 11:53:49]

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

[commit-bot: I haz the power, 2014-04-04 11:53:49]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-04 13:59:57]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-04 14:00:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/224593002/40001

[commit-bot: I haz the power, 2014-04-04 14:47:11]

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

[commit-bot: I haz the power, 2014-04-04 14:47:11]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-04 14:49:51]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-04 14:49:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/224593002/40001

[commit-bot: I haz the power, 2014-04-04 16:14:41]

Change committed as 170881