Sibling invalidation sets

Sibling invalidation sets

Design doc: https://goo.gl/z0Z9gn

See also Rune's document https://goo.gl/KzlKHN

BUG=378135
Committed: https://crrev.com/6f611157e71e74e53568b7a96f73000f0f8187f8
Cr-Commit-Position: refs/heads/master@{#356121}

[Eric Willigers, 2015-08-26 12:28:16]

Patchset #5 (id:80001) has been deleted

[Eric Willigers, 2015-08-26 12:28:36]

Patchset #2 (id:20001) has been deleted

[Eric Willigers, 2015-08-26 12:28:43]

Patchset #2 (id:40001) has been deleted

[Eric Willigers, 2015-08-28 03:41:53]

Patchset #5 (id:140001) has been deleted

[Eric Willigers, 2015-08-28 03:42:41]

Patchset #3 (id:100001) has been deleted

[Eric Willigers, 2015-08-28 08:21:34]

ericwilligers@chromium.org changed reviewers:
+ dstockwell@chromium.org, esprehn@chromium.org, rune@opera.com,
timloh@chromium.org

[Eric Willigers, 2015-08-31 06:40:43]

https://codereview.chromium.org/1317533002/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1317533002/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2123: styleChanged =
documentElement->childNeedsStyleInvalidation();
WebViewTest.ShowUnhandledTapUIIfNeededWithMutateStyle was failing with mouseup
and mousemove not causing style updates.

This EventHandler change is one way to avoid introducing a test failure, I'm not
sure if it is correct.

[Eric Willigers, 2015-09-01 09:17:20]

Patchset #6 (id:200001) has been deleted

[Eric Willigers, 2015-09-01 09:46:59]

Patchset #7 (id:240001) has been deleted

[rune, 2015-09-01 15:56:46]

https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
File Source/core/css/invalidation/DescendantInvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
Source/core/css/invalidation/DescendantInvalidationSet.h:119:
RefPtrWillBeMember<DescendantInvalidationSet> m_descendantInvalidationSet;
Does this mean that if this is the sibling invalidation set for .a, and you have
the following selectors:

.a + .b .c {}
.a + span + .d .e {}

you will have one descendant invalidation set for .b containing .c, one
descendant invalidation set for .d containing .e, and a third descendant
invalidation set pointed to here which combines .b and .d into a set which
contains both .c and .e?

[Eric Willigers, 2015-09-01 22:09:38]

On 2015/09/01 15:56:46, rune wrote:
>
https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
> File Source/core/css/invalidation/DescendantInvalidationSet.h (right):
> 
>
https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
> Source/core/css/invalidation/DescendantInvalidationSet.h:119:
> RefPtrWillBeMember<DescendantInvalidationSet> m_descendantInvalidationSet;
> Does this mean that if this is the sibling invalidation set for .a, and you
have
> the following selectors:
> 
> .a + .b .c {}
> .a + span + .d .e {}
> 
> you will have one descendant invalidation set for .b containing .c, one
> descendant invalidation set for .d containing .e, and a third descendant
> invalidation set pointed to here which combines .b and .d into a set which
> contains both .c and .e?

I would refer to the third invalidation set as a sibling invalidation set. True,
it is temporarily implemented as a DescendantInvalidationSet, but we can either
rename that to InvalidationSet (my preference) or introduce distinct classes.

Your example produces:-
descendant invalidation set for .b containing .c
descendant invalidation set for .d containing .e
sibling invalidation set for .a containing .b and .d, with the sibling
invalidation set itself owning a descendant invalidation set containing .c and
.e
sibling invalidation set for <span> containing .d, with the sibling invalidation
set itself owning a descendant invalidation set containing .e

[rune, 2015-09-02 07:03:44]

On 2015/09/01 22:09:38, Eric Willigers wrote:
> On 2015/09/01 15:56:46, rune wrote:
> >
>
https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
> > File Source/core/css/invalidation/DescendantInvalidationSet.h (right):
> > 
> >
>
https://codereview.chromium.org/1317533002/diff/260001/Source/core/css/invali...
> > Source/core/css/invalidation/DescendantInvalidationSet.h:119:
> > RefPtrWillBeMember<DescendantInvalidationSet> m_descendantInvalidationSet;
> > Does this mean that if this is the sibling invalidation set for .a, and you
> have
> > the following selectors:
> > 
> > .a + .b .c {}
> > .a + span + .d .e {}
> > 
> > you will have one descendant invalidation set for .b containing .c, one
> > descendant invalidation set for .d containing .e, and a third descendant
> > invalidation set pointed to here which combines .b and .d into a set which
> > contains both .c and .e?
> 
> I would refer to the third invalidation set as a sibling invalidation set.
True,
> it is temporarily implemented as a DescendantInvalidationSet, but we can
either
> rename that to InvalidationSet (my preference) or introduce distinct classes.
> 
> Your example produces:-
> descendant invalidation set for .b containing .c
> descendant invalidation set for .d containing .e
> sibling invalidation set for .a containing .b and .d, with the sibling
> invalidation set itself owning a descendant invalidation set containing .c and
> .e
> sibling invalidation set for <span> containing .d, with the sibling
invalidation
> set itself owning a descendant invalidation set containing .e

Ah, so the difference between my original idea is that I wanted to re-use the
descendant invalidation sets for the sibling features instead of having a
separate descendant invalidation set for siblings. Having a separate one is
probably better. My gut feeling is that having descendant/child combinators to
the right of adjacent combinators is not so common in the wild. With my proposal
given selectors:

.a + .b {}
.b span {}

We would have invalidated spans in sub-tree of .b when changing .a. While with
your proposal, we would only invalidate the element with .b.

[Eric Willigers, 2015-09-07 06:45:29]

Patchset #10 (id:320001) has been deleted

[Timothy Loh, 2015-09-10 06:09:40]

Looks pretty good

https://codereview.chromium.org/1317533002/diff/340001/Source/core/input/Even...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1317533002/diff/340001/Source/core/input/Even...
Source/core/input/EventHandler.cpp:2127: if (!styleChanged) {
Why did this have to change?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:387:
invalidationSet.setMaxDirectAdjacentSelectors(features.maxDirectAdjacentSelectors);
updateMax..?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:420:
addFeaturesToInvalidationSet(*invalidationSet, siblingFeatures ?
*siblingFeatures : descendantFeatures);
This would probably be clearer if we did this separately for the
sibling/descendant cases

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:462: if (result.first)
this might always be non-null?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:84: void
collectInvalidationSetsForClass(InvalidationSetVector&, InvalidationSetVector&,
Element&, const AtomicString& className) const;
argument names are probably a good idea here

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.cpp:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Nothing in this file, don't need it?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:12: class CORE_EXPORT
InvalidationListData final {
could do with a better name

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:71: InvalidationSet&
ensureInvalidationSet();
awkward name

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:54: break;
this could possibly still be an early return, as well as the >= Subtree check
above

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:153:
m_invalidationEntries.append(Entry { &invalidationSet, invalidationLimit });
this is banned https://chromium-cpp.appspot.com/ :-(

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:164:
m_invalidationEntries[index] =
m_invalidationEntries[m_invalidationEntries.size() - 1];
.last() I think?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp (left):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:64: if (relation
!= CSSSelector::Descendant && relation != CSSSelector::Child && relation !=
CSSSelector::SubSelector)
This doesn't seem related?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
Source/core/dom/StyleEngine.cpp:635: InvalidationSetVector descendant, sibling;
Needs a class or struct or something instead of passing two values around
everywhere

[esprehn, 2015-09-10 08:54:53]

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:241: InvalidationSet*
RuleFeatureSet::invalidationSetForSelector(const CSSSelector& selector,
InvalidateType type)
InvalidateType is a weird name, I think you want InvalidationType?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeatureSetTest.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:1: 
missing copyright

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:127: expectClassInvalidation("p",
descendant);
I think you want more than one test, having one megatest is super hard to debug.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:207: expectClassesInvalidation("w", "x",
descendant);
please write small tests

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleSe...
Source/core/css/RuleSet.h:72: class CORE_EXPORT RuleData {
ditto

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/StyleR...
File Source/core/css/StyleRule.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/StyleR...
Source/core/css/StyleRule.h:104: class CORE_EXPORT StyleRule : public
StyleRuleBase {
Why do you need to export StyleRule from core? this doesn't belong outside core

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationData.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationData.cpp:13: m_invalidationSet[type] =
InvalidationSet::create();
Why do you need dynamic dispatch for the type? Using an array like this is
pretty hard to debug later.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:17: InvalidationSetVector&
descendants() { return m_invalidationList[InvalidateDescendants]; }
This doesn't need an array and an enum like this.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:23: InvalidationSetVector
m_invalidationList[InvalidateTypeCount];
Please don't use an array of Vectors, this only contains two of them, just add
two InvalidationSetVector's here.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:49: // recalculated.
comment needs updating?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:122: unsigned m_appliesDirectly :
1;
I'm not a fan of special cases like this where it can be yourself or your kids,
why do you need this?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
Source/core/dom/StyleEngine.cpp:635: InvalidationSetVector descendant, sibling;
One variable per line

https://codereview.chromium.org/1317533002/diff/380001/Source/core/input/Even...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/input/Even...
Source/core/input/EventHandler.cpp:2129: styleChanged =
documentElement->childNeedsStyleInvalidation();
This is not the right layer for this... is this a separate bug you need to fix?

[rune, 2015-09-10 13:54:50]

I think this CL needs to be split up to make it easier to review the actual
sibling invalidation set implementation.

At least, the renaming of InvalidationSet, and the appliesDirectly optimization
should be split out.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/StyleR...
File Source/core/css/StyleRule.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/StyleR...
Source/core/css/StyleRule.h:104: class CORE_EXPORT StyleRule : public
StyleRuleBase {
On 2015/09/10 08:54:53, esprehn wrote:
> Why do you need to export StyleRule from core? this doesn't belong outside
core

Because RuleFeatureSetTest.cpp needs access to StyleRule and unit tests are in a
different component than the rest of Blink, I think. Which makes the CORE_EXPORT
thing pretty broken as you in an ideal world would unit test all your classes,
effectively making all your classes CORE_EXPORT.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.cpp (left):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.cpp:12: }
I prefer --no-find-copies for git cl upload to see unrelated diffs like this.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:50: class CORE_EXPORT
InvalidationSet final : public RefCountedWillBeGarbageCollected<InvalidationSet>
{
Could we do the DescendantInvalidationSet -> InvalidationSet renaming in a
separate CL?

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:122: unsigned m_appliesDirectly :
1;
On 2015/09/10 08:54:53, esprehn wrote:
> I'm not a fan of special cases like this where it can be yourself or your
kids,
> why do you need this?

This is an optimization for the case where a feature occurs in non-leftmore
compounds only. For instance given the selector:

.a .b { }

We currently create an invalidation set for .a containing .b, but we currently
don't know if there was also a selector like ".a {}" which requires a
LocalStyleChange on the element which gets .a set. So currently, we always do a
LocalStyleChange for the element which gets an invalidation set scheduled.

I don't know if this optimization is more important for sibling invalidation
sets, but I would like to see that optimization as a separate CL.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp (left):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:64: if (relation
!= CSSSelector::Descendant && relation != CSSSelector::Child && relation !=
CSSSelector::SubSelector)
On 2015/09/10 06:09:40, Timothy Loh wrote:
> This doesn't seem related?

This might actually work with the current code given that
maxDirectAdjacentSelectors is up-to-date with the new stylesheet when the
SubtreeStyleChange set in StyleSheetInvalidationAnalysis::invalidateStyle
triggers sibling recalcs in
ContainerNode::checkForChildrenAdjacentRuleChanges().

But, it should be a different CL in either case.

[Eric Willigers, 2015-09-14 07:20:24]

The following comments have not yet been addressed:
RuleFeatureSetTest.cpp should have small tests.
InvalidationData.cpp - justify dynamic dispatch
InvalidationSet.h - comment needs updating, measure impact of not having
appliesDirectly
EventHandler.cpp - not the right layer

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:241: InvalidationSet*
RuleFeatureSet::invalidationSetForSelector(const CSSSelector& selector,
InvalidateType type)
On 2015/09/10 08:54:53, esprehn wrote:
> InvalidateType is a weird name, I think you want InvalidationType?

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:387:
invalidationSet.setMaxDirectAdjacentSelectors(features.maxDirectAdjacentSelectors);
On 2015/09/10 06:09:40, Timothy Loh wrote:
> updateMax..?

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:420:
addFeaturesToInvalidationSet(*invalidationSet, siblingFeatures ?
*siblingFeatures : descendantFeatures);
On 2015/09/10 06:09:40, Timothy Loh wrote:
> This would probably be clearer if we did this separately for the
> sibling/descendant cases

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:462: if (result.first)
On 2015/09/10 06:09:40, Timothy Loh wrote:
> this might always be non-null?

Acknowledged.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:84: void
collectInvalidationSetsForClass(InvalidationSetVector&, InvalidationSetVector&,
Element&, const AtomicString& className) const;
On 2015/09/10 06:09:40, Timothy Loh wrote:
> argument names are probably a good idea here

Combined the two vector references into one reference to a class.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeatureSetTest.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:1: 
On 2015/09/10 08:54:53, esprehn wrote:
> missing copyright

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.cpp (left):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.cpp:12: }
On 2015/09/10 13:54:50, rune wrote:
> I prefer --no-find-copies for git cl upload to see unrelated diffs like this.
> 

Removed this file.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationListData.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:12: class CORE_EXPORT
InvalidationListData final {
On 2015/09/10 06:09:40, Timothy Loh wrote:
> could do with a better name

Acknowledged. Renamed to InvalidationLists.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:17: InvalidationSetVector&
descendants() { return m_invalidationList[InvalidateDescendants]; }
On 2015/09/10 08:54:53, esprehn wrote:
> This doesn't need an array and an enum like this.

Fixed.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationListData.h:23: InvalidationSetVector
m_invalidationList[InvalidateTypeCount];
On 2015/09/10 08:54:53, esprehn wrote:
> Please don't use an array of Vectors, this only contains two of them, just add
> two InvalidationSetVector's here.

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:50: class CORE_EXPORT
InvalidationSet final : public RefCountedWillBeGarbageCollected<InvalidationSet>
{
On 2015/09/10 13:54:50, rune wrote:
> Could we do the DescendantInvalidationSet -> InvalidationSet renaming in a
> separate CL?

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:71: InvalidationSet&
ensureInvalidationSet();
On 2015/09/10 06:09:40, Timothy Loh wrote:
> awkward name

Renamed to ensureDescendantInvalidationSet

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:54: break;
On 2015/09/10 06:09:40, Timothy Loh wrote:
> this could possibly still be an early return, as well as the >= Subtree check
> above

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:153:
m_invalidationEntries.append(Entry { &invalidationSet, invalidationLimit });
On 2015/09/10 06:09:40, Timothy Loh wrote:
> this is banned https://chromium-cpp.appspot.com/ :-(

Fixed.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:164:
m_invalidationEntries[index] =
m_invalidationEntries[m_invalidationEntries.size() - 1];
On 2015/09/10 06:09:40, Timothy Loh wrote:
> .last() I think?

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp (left):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:64: if (relation
!= CSSSelector::Descendant && relation != CSSSelector::Child && relation !=
CSSSelector::SubSelector)
On 2015/09/10 13:54:50, rune wrote:
> On 2015/09/10 06:09:40, Timothy Loh wrote:
> > This doesn't seem related?
> 
> This might actually work with the current code given that
> maxDirectAdjacentSelectors is up-to-date with the new stylesheet when the
> SubtreeStyleChange set in StyleSheetInvalidationAnalysis::invalidateStyle
> triggers sibling recalcs in
> ContainerNode::checkForChildrenAdjacentRuleChanges().
> 
> But, it should be a different CL in either case.

Reverted, it isn't needed.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
Source/core/dom/StyleEngine.cpp:635: InvalidationSetVector descendant, sibling;
On 2015/09/10 06:09:40, Timothy Loh wrote:
> Needs a class or struct or something instead of passing two values around
> everywhere

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/dom/StyleE...
Source/core/dom/StyleEngine.cpp:635: InvalidationSetVector descendant, sibling;
On 2015/09/10 08:54:53, esprehn wrote:
> One variable per line

Acknowledged.

[Eric Willigers, 2015-09-14 07:23:23]

Patchset #13 (id:400001) has been deleted

[rune, 2015-09-14 23:06:51]

I've dived into all the details yet. Can we split out appliesDirectly first?

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationData.h (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationData.h:13: InvalidateDescendants = 0,
Enums start from 0 if you don't specify. Just drop all enum value assignments.

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:122: unsigned m_appliesDirectly :
1;
I'd still like to see this split out as a separate CL. A lot of the expected
test result changes are for the descendant case with !appliesDirectly, right?

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:180: // may propagate the
SubtreeStyleChange to our own siblings' subtrees.
I think that we ultimately want to end up with removing
checkForChildrenAdjacentRuleChanges() and let SubtreeStyleChange only cause a
subtree recalc. In theory, all setNeedsStyleRecalc(SubtreeStyleChange) around
the code that depends on recalculating sibling trees are there to account for
adjacent combinators.

https://codereview.chromium.org/1317533002/diff/420001/Source/core/input/Even...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/input/Even...
Source/core/input/EventHandler.cpp:2129: styleChanged =
documentElement->childNeedsStyleInvalidation();
The test which started failing has a sibling selector where a change used to
cause a setNeedsStyleRecalc(SubtreeStyleChange) instead of a
setNeedsStyleInvalidation(). There's a difference between marking ancestor
chains for setNeedsStyleRecalc and setNeedsStyleInvalidation where the
invalidation case would not increase the style version. Instead of this fix
here, I think incStyleVersion for style invalidations would be the correct fix.

See https://codereview.chromium.org/1333193004

[Eric Willigers, 2015-09-15 05:39:18]

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
File Source/core/css/RuleFeatureSetTest.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:127: expectClassInvalidation("p",
descendant);
On 2015/09/10 08:54:53, esprehn wrote:
> I think you want more than one test, having one megatest is super hard to
debug.

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/RuleFe...
Source/core/css/RuleFeatureSetTest.cpp:207: expectClassesInvalidation("w", "x",
descendant);
On 2015/09/10 08:54:53, esprehn wrote:
> please write small tests

Done.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationData.cpp (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationData.cpp:13: m_invalidationSet[type] =
InvalidationSet::create();
On 2015/09/10 08:54:53, esprehn wrote:
> Why do you need dynamic dispatch for the type? Using an array like this is
> pretty hard to debug later.

I dispatch on the type to avoid duplicating the RuleFeatureSet methods
invalidationSetForSelector ensureClassInvalidationSet
ensureAttributeInvalidationSet ensureIdInvalidationSet
ensurePseudoInvalidationSet.

The class members don't need to be arrays.

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/380001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:49: // recalculated.
On 2015/09/10 08:54:53, esprehn wrote:
> comment needs updating?

Done.

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationData.h (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationData.h:13: InvalidateDescendants = 0,
On 2015/09/14 23:06:51, rune wrote:
> Enums start from 0 if you don't specify. Just drop all enum value assignments.

Done.

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:122: unsigned m_appliesDirectly :
1;
On 2015/09/14 23:06:51, rune wrote:
> I'd still like to see this split out as a separate CL. A lot of the expected
> test result changes are for the descendant case with !appliesDirectly, right?

Removed for now. Yes, may of the expected test result changes don't involve
sibling selectors.

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
Source/core/css/invalidation/StyleInvalidator.cpp:180: // may propagate the
SubtreeStyleChange to our own siblings' subtrees.
On 2015/09/14 23:06:51, rune wrote:
> I think that we ultimately want to end up with removing
> checkForChildrenAdjacentRuleChanges() and let SubtreeStyleChange only cause a
> subtree recalc. In theory, all setNeedsStyleRecalc(SubtreeStyleChange) around
> the code that depends on recalculating sibling trees are there to account for
> adjacent combinators.

Yes, the complication is DOM changes, e.g. removing or inserting a node between
siblings so + + applies or no longer applies. An extra bit on Element for
SiblingStyleChanges would be one option.

[esprehn, 2015-09-15 05:45:13]

On 2015/09/15 at 05:39:18, ericwilligers wrote:
> ...
> 
>
https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
> File Source/core/css/invalidation/StyleInvalidator.cpp (right):
> 
>
https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
> Source/core/css/invalidation/StyleInvalidator.cpp:180: // may propagate the
SubtreeStyleChange to our own siblings' subtrees.
> On 2015/09/14 23:06:51, rune wrote:
> > I think that we ultimately want to end up with removing
> > checkForChildrenAdjacentRuleChanges() and let SubtreeStyleChange only cause
a
> > subtree recalc. In theory, all setNeedsStyleRecalc(SubtreeStyleChange)
around
> > the code that depends on recalculating sibling trees are there to account
for
> > adjacent combinators.
> 
> Yes, the complication is DOM changes, e.g. removing or inserting a node
between siblings so + + applies or no longer applies. An extra bit on Element
for SiblingStyleChanges would be one option.

We shouldn't need any extra bits, the style invalidation sets should handle any
sibling cases. The "worst case" should just be scheduling an invalidation set
that marks SubtreeStyleChange on every sibling.

[Eric Willigers, 2015-09-17 07:38:59]

On 2015/09/15 05:45:13, esprehn wrote:
> On 2015/09/15 at 05:39:18, ericwilligers wrote:
> > ...
> > 
> >
>
https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
> > File Source/core/css/invalidation/StyleInvalidator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1317533002/diff/420001/Source/core/css/invali...
> > Source/core/css/invalidation/StyleInvalidator.cpp:180: // may propagate the
> SubtreeStyleChange to our own siblings' subtrees.
> > On 2015/09/14 23:06:51, rune wrote:
> > > I think that we ultimately want to end up with removing
> > > checkForChildrenAdjacentRuleChanges() and let SubtreeStyleChange only
cause
> a
> > > subtree recalc. In theory, all setNeedsStyleRecalc(SubtreeStyleChange)
> around
> > > the code that depends on recalculating sibling trees are there to account
> for
> > > adjacent combinators.
> > 
> > Yes, the complication is DOM changes, e.g. removing or inserting a node
> between siblings so + + applies or no longer applies. An extra bit on Element
> for SiblingStyleChanges would be one option.
> 
> We shouldn't need any extra bits, the style invalidation sets should handle
any
> sibling cases. The "worst case" should just be scheduling an invalidation set
> that marks SubtreeStyleChange on every sibling.

I have now documented the idea of of using an invalidation set when nodes are
inserted or removed,  https://goo.gl/z0Z9gn#heading=h.pw857ccdu0c9

[Eric Willigers, 2015-09-22 03:15:22]

Rebased to use invalidatesSelf.

[rune, 2015-10-02 13:17:59]

I've reviewed the process of building the invalidation sets now. With comments
below.

I have yet to review the invalidation process itself.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:408: // selector is the selector immediately to
the left of the rightmost selector.
Immediately to the left of the rightmost _combinator_?

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:410: // descendantFeatures has the features of
the rightmost selector.
"rightmost compound selector"

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:447: selector = current->tagHistory();
This took a while for me to understand. It's used to record the start of the
previous compound selector, right? Instead of re-using the in-parameter, could
you make it a local variable naming it previousCompoundSelector,
lastCompoundSelectorInAdjacentChain, or something similar?

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:459: } else {
Could you try to get rid of some if-ery nesting here? You could use continue as
you did for the block above.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:173: void addFeaturesToInvalidationSets(const
CSSSelector*, InvalidationSetFeatures*, InvalidationSetFeatures&);
You should name the two InvalidationSetFeatures parameters. The types are not
self-explanatory here.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:187: InvalidationData&
RuleFeatureSet::ensureInvalidationData(Map& map, const typename Map::KeyType&
key)
This method does not need to be a member of RuleFeatureSet, and if you move the
implementations of ensureClass* etc. to the cpp file, you can move this template
method there.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:131: unsigned
m_maxDirectAdjacentSelectors;
Can we use sub-classing instead?

SiblingInvalidationSet sub-class which adds these members.

In particular, I think that the code in addFeaturesToInvalidationSets would be
easier to read and understand if you had

class InvalidationSet { ... }

class DescendantInvalidationSet : public InvalidationSet {
    /* nothing added, just to have a separate type to make things clearer */
}

class SiblingInvalidationSet : public InvalidationSet
{
    maxDirectAdjacentSelectors;
    DescendantInvalidationSet m_descendantInvalidationSet
}

[rune, 2015-10-02 13:57:56]

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:461: auto result =
extractInvalidationSetFeatures(*selector, localFeatures, false);
Note: extractInvalidationSetFeatures with requiresSubtreeInvalidation were
implemented under the assumption that it was done on the rightmost compound, and
this breaks that assumption. I don't think it currently matters for all
practical purposes, but it's something to be aware of.

Currently, it matters for ::first-line, ::first-letter, and :host-context. The
two former can only appear in the rightmost compound anyway, and the latter will
never match anything if preceeded by an adjacent combinator.

[Eric Willigers, 2015-10-14 00:06:50]

Patchset #1 (id:1) has been deleted

[Eric Willigers, 2015-10-14 00:07:16]

Patchset #3 (id:160001) has been deleted

[Eric Willigers, 2015-10-14 00:07:41]

Patchset #13 (id:460001) has been deleted

[Eric Willigers, 2015-10-14 00:08:05]

Patchset #13 (id:480001) has been deleted

[Eric Willigers, 2015-10-14 00:08:20]

Patchset #3 (id:180001) has been deleted

[Eric Willigers, 2015-10-14 00:25:40]

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:408: // selector is the selector immediately to
the left of the rightmost selector.
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> Immediately to the left of the rightmost _combinator_?

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:410: // descendantFeatures has the features of
the rightmost selector.
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> "rightmost compound selector"

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:447: selector = current->tagHistory();
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> This took a while for me to understand. It's used to record the start of the
> previous compound selector, right? Instead of re-using the in-parameter, could
> you make it a local variable naming it previousCompoundSelector,
> lastCompoundSelectorInAdjacentChain, or something similar?

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:447: selector = current->tagHistory();
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> This took a while for me to understand. It's used to record the start of the
> previous compound selector, right? Instead of re-using the in-parameter, could
> you make it a local variable naming it previousCompoundSelector,
> lastCompoundSelectorInAdjacentChain, or something similar?

Yes, for example when processing ".p.q + .r.s + .t.v > .w.x + .y.z"
from right to left, we need to remember .v so that when we reach ".s +" we know
the compound selector to the right is ".t.v".

Renamed to lastCompoundSelectorInAdjacentChain.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:459: } else {
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> Could you try to get rid of some if-ery nesting here? You could use continue
as
> you did for the block above.

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.cpp:461: auto result =
extractInvalidationSetFeatures(*selector, localFeatures, false);
On 2015/10/02 13:57:56, rune (OOO - slower) wrote:
> Note: extractInvalidationSetFeatures with requiresSubtreeInvalidation were
> implemented under the assumption that it was done on the rightmost compound,
and
> this breaks that assumption. I don't think it currently matters for all
> practical purposes, but it's something to be aware of.
> 
> Currently, it matters for ::first-line, ::first-letter, and :host-context. The
> two former can only appear in the rightmost compound anyway, and the latter
will
> never match anything if preceeded by an adjacent combinator.

Acknowledged.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:173: void addFeaturesToInvalidationSets(const
CSSSelector*, InvalidationSetFeatures*, InvalidationSetFeatures&);
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> You should name the two InvalidationSetFeatures parameters. The types are not
> self-explanatory here.

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/RuleFe...
Source/core/css/RuleFeature.h:187: InvalidationData&
RuleFeatureSet::ensureInvalidationData(Map& map, const typename Map::KeyType&
key)
On 2015/10/02 13:17:58, rune (OOO - slower) wrote:
> This method does not need to be a member of RuleFeatureSet, and if you move
the
> implementations of ensureClass* etc. to the cpp file, you can move this
template
> method there.

Done.

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/invali...
File Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/1317533002/diff/500001/Source/core/css/invali...
Source/core/css/invalidation/InvalidationSet.h:131: unsigned
m_maxDirectAdjacentSelectors;
On 2015/10/02 13:17:59, rune (OOO - slower) wrote:
> Can we use sub-classing instead?
> 
> SiblingInvalidationSet sub-class which adds these members.
> 
> In particular, I think that the code in addFeaturesToInvalidationSets would be
> easier to read and understand if you had
> 
> class InvalidationSet { ... }
> 
> class DescendantInvalidationSet : public InvalidationSet {
>     /* nothing added, just to have a separate type to make things clearer */
> }
> 
> class SiblingInvalidationSet : public InvalidationSet
> {
>     maxDirectAdjacentSelectors;
>     DescendantInvalidationSet m_descendantInvalidationSet
> }

Done.

[Eric Willigers, 2015-10-20 03:23:28]

Patchset #5 (id:280001) has been deleted

[Eric Willigers, 2015-10-20 03:24:04]

Patchset #3 (id:220001) has been deleted

[Eric Willigers, 2015-10-20 03:46:12]

Patchset #15 (id:590001) has been deleted

[Eric Willigers, 2015-10-20 04:38:01]

Patchset #15 (id:610001) has been deleted

[rune, 2015-10-20 18:39:24]

Great work!

LGTM with a couple of notes:

See oilpan note (I see you're already doing oilpan trybot'ing)

Have you done any performance testing?

If you haven't already, I recommend running PerformanceTests/CSS locally to
check if there are any regressions.

Also, did you confirm that this actually fixes the html spec slowness
(crbug.com/376847)?

https://codereview.chromium.org/1317533002/diff/650001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.h (right):

https://codereview.chromium.org/1317533002/diff/650001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.h:116: using
PendingInvalidationMap = HashMap<Element*, RefPtr<PendingInvalidations>>;
Does it work to take this off the oilpan heap and not trace Element?

[Eric Willigers, 2015-10-21 00:49:26]

Patchset #15 (id:630001) has been deleted

[Eric Willigers, 2015-10-21 00:49:39]

Patchset #16 (id:670001) has been deleted

[Eric Willigers, 2015-10-26 06:43:07]

> Also, did you confirm that this actually fixes the html spec slowness
> (crbug.com/376847)?

I haven't experienced the slowness except for the first few seconds after page
load. I've tried adding back a couple of the style rules mentioned in the
discussion, but still don't see slow response to hover changes.

[Eric Willigers, 2015-10-26 06:43:23]

https://codereview.chromium.org/1317533002/diff/650001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.h (right):

https://codereview.chromium.org/1317533002/diff/650001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.h:116: using
PendingInvalidationMap = HashMap<Element*, RefPtr<PendingInvalidations>>;
On 2015/10/20 18:39:24, rune wrote:
> Does it work to take this off the oilpan heap and not trace Element?

Fixed. Back on Oilpan heap.

[rune, 2015-10-26 10:27:06]

Still lgtm

[Eric Willigers, 2015-10-26 19:59:12]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2015-10-26 20:00:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317533002/690001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317533002/690001

[commit-bot: I haz the power, 2015-10-26 21:06:44]

Committed patchset #16 (id:690001)

[commit-bot: I haz the power, 2015-10-26 21:08:25]

Patchset 16 (id:??) landed as
https://crrev.com/6f611157e71e74e53568b7a96f73000f0f8187f8
Cr-Commit-Position: refs/heads/master@{#356121}

[esprehn, 2015-10-26 21:20:14]

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators-expected.txt
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators-expected.txt:9:
PASS internals.updateStyleAndReturnAffectedElementCount() is 2
why did this one go up?

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html:59:
shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "2");
why did it go up?

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:496:
siblingFeatures->maxDirectAdjacentSelectors =
std::numeric_limits<unsigned>::max();
I'd just use UINT_MAX for all of these, this std::numeric_limits thing is so
verbose...

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:604: if
(element.parentElement())
Why are you checking parentElement()? For what reason? This won't work for the
html element or the direct kids of a ShadowRoot, I'm not sure what you were
trying to do here.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:619: if
(element.parentElement())
ditto, this parentElement() check doesn't seem right.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:634: if
(element.parentElement())
ditto

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:649: if
(element.parentElement())
ditto

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:140: void
StyleInvalidator::SiblingData::pushInvalidationSet(const SiblingInvalidationSet&
invalidationSet)
Lets unnest this class, SiblingInvaliationData and put it in another file.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:159:
m_invalidationEntries.removeLast();
What is this doing? Can you add a comment explaining why you're removing entries
inside the match function, that doesn't seem right... match should be const.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:168: //
Avoid directly setting SubtreeStyleChange on element, or
ContainerNode::checkForChildrenAdjacentRuleChanges()
Why does checkForChildrenAdjacentRuleChanges still exist? That was supposed to
be replaced by the sibling invalidator.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:171: for
(Element* child = ElementTraversal::firstChild(element); child; child =
ElementTraversal::nextSibling(*child)) {
We should delete the checkForChildrenAdjacentRuleChanges function and just set
SubtreeStyleChange on the parent, making this be O(n) is unfortunate. We should
be able to early out here.

[Eric Willigers, 2015-10-27 07:24:55]

On 2015/10/26 21:20:14, esprehn wrote:
> ...

I'll address the various comments individually.

The invalidation-set-with-adjacent-combinators.html
change is fixed in
https://codereview.chromium.org/1418133013/

[rune, 2015-10-27 09:50:03]

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:168: //
Avoid directly setting SubtreeStyleChange on element, or
ContainerNode::checkForChildrenAdjacentRuleChanges()
On 2015/10/26 21:20:14, esprehn wrote:
> Why does checkForChildrenAdjacentRuleChanges still exist? That was supposed to
> be replaced by the sibling invalidator.

Yes. I do think there are a few optimizations around which expects
SubtreeStyleChange to recalculate sibling subtrees through
checkForChildrenAdjacentRuleChanges. For instance:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

For sibling invalidations, we still need to schedule an invalidation even though
SubtreeStyleChange is set.

[Eric Willigers, 2015-10-28 23:09:08]

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators-expected.txt
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators-expected.txt:9:
PASS internals.updateStyleAndReturnAffectedElementCount() is 2
On 2015/10/26 21:20:14, esprehn wrote:
> why did this one go up?

Addressed in https://codereview.chromium.org/1418133013/
Done.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/invalidation/invalidation-set-with-adjacent-combinators.html:59:
shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "2");
On 2015/10/26 21:20:14, esprehn wrote:
> why did it go up?

Addressed in https://codereview.chromium.org/1418133013/
Done.

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RuleFeature.cpp:496:
siblingFeatures->maxDirectAdjacentSelectors =
std::numeric_limits<unsigned>::max();
On 2015/10/26 21:20:14, esprehn wrote:
> I'd just use UINT_MAX for all of these, this std::numeric_limits thing is so
> verbose...

Done. https://codereview.chromium.org/1424783003/

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp
(right):

https://codereview.chromium.org/1317533002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp:159:
m_invalidationEntries.removeLast();
On 2015/10/26 21:20:14, esprehn wrote:
> What is this doing? Can you add a comment explaining why you're removing
entries
> inside the match function, that doesn't seem right... match should be const.

A rule like
.a + *
only affects the next sibling, and is then retired from SiblingData.

I've moved this retirement out to advance(), and added a comment, in
https://codereview.chromium.org/1418333004/