Factor the style invalidation tree walk out of RuleFeatureSet, and make it generic.

Factor the style invalidation tree walk out of RuleFeatureSet, and make it generic.

This is in anticipation of adding support for attributes.

BUG=352300, 335247
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169663

[esprehn, 2014-03-19 22:01:59]

Tree walk is not a good name, this is lots of algorithms and tree walking is
just an implementation detail.

[esprehn, 2014-03-19 22:09:04]

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:41: bool
StyleInvalidationTreeWalk::RecursionData::matchesCurrentInvalidationSets(Element*
element)
This should be a method on the InvalidationSet itself I think like in my patch.

Lets also avoid all these nested classes.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:55: bool
StyleInvalidationTreeWalk::checkInvalidationSetsAgainstElement(Element* element)
invalidateOwnStyle.

These should match recalcStyle.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:59: if
(RuleFeatureSet::InvalidationList* invalidationList =
m_featureSet->m_pendingInvalidationMap.get(element)) {
You should expose getters, don't reach into the member variables of
RuleFeatureSet.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:80: bool
StyleInvalidationTreeWalk::invalidateStyleForChildren(Element* element)
invalidateChildrenStyle

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:109: }
extra braces

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.h (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:14: static void
computeStyleInvalidation(Document&, RuleFeatureSet*);
No static, this should work like the ChildInsertionNotifier or the
SharedStyleFinder.

StyleInvalidator(document).invalidate();

There's no reason to pass in a RuleFeatureSet, you can get it from the document.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:22: bool
checkInvalidationSetsAgainstElement(Element*);
These should all be references.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:35: friend class
RecursionCheckpoint;
No friends please.

[chrishtr, 2014-03-19 22:49:39]

Going to rename the class now to StyleInvalidator...you make a good point about
tree walk being an implementation detail.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:41: bool
StyleInvalidationTreeWalk::RecursionData::matchesCurrentInvalidationSets(Element*
element)
On 2014/03/19 22:09:04, esprehn wrote:
> This should be a method on the InvalidationSet itself I think like in my
patch.
> 
> Lets also avoid all these nested classes.

The problem with that is that this code operates on an aggregated view of a
bunch of DescendantInvalidationSets. This is not performant enough to represent
as a DescendantInvalidationSet itself because of the need to push and pop
recursive state. Vectors are good for that.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:55: bool
StyleInvalidationTreeWalk::checkInvalidationSetsAgainstElement(Element* element)
On 2014/03/19 22:09:04, esprehn wrote:
> invalidateOwnStyle.
> 
> These should match recalcStyle.

This method does not do a full invalidation. We don't know the full invalidation
to do until after recursion.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:59: if
(RuleFeatureSet::InvalidationList* invalidationList =
m_featureSet->m_pendingInvalidationMap.get(element)) {
On 2014/03/19 22:09:04, esprehn wrote:
> You should expose getters, don't reach into the member variables of
> RuleFeatureSet.

Done.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:80: bool
StyleInvalidationTreeWalk::invalidateStyleForChildren(Element* element)
On 2014/03/19 22:09:04, esprehn wrote:
> invalidateChildrenStyle

Done.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.cpp:109: }
On 2014/03/19 22:09:04, esprehn wrote:
> extra braces

Done.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.h (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:14: static void
computeStyleInvalidation(Document&, RuleFeatureSet*);
On 2014/03/19 22:09:04, esprehn wrote:
> No static, this should work like the ChildInsertionNotifier or the
> SharedStyleFinder.
> 
> StyleInvalidator(document).invalidate();
> 
> There's no reason to pass in a RuleFeatureSet, you can get it from the
document.

The point of the static is to enforce invalidator object lifetime to be within
the execution time of calls to computeStyleInvalidation(), which in turn
enforces that the recursion data is correctly initialized and the object can't
be run twice. This is the main reason I made a class for the tree walk.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:22: bool
checkInvalidationSetsAgainstElement(Element*);
On 2014/03/19 22:09:04, esprehn wrote:
> These should all be references.

Done.

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:35: friend class
RecursionCheckpoint;
On 2014/03/19 22:09:04, esprehn wrote:
> No friends please.

Done.

[chrishtr, 2014-03-19 23:03:41]

Renamed the class, made methods not say Style redundantly.

[esprehn, 2014-03-19 23:04:13]

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.h (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:14: static void
computeStyleInvalidation(Document&, RuleFeatureSet*);
That's not the correct way to solve this. We have lots of these objects. If
you're really paranoid use STACK_ALLOCATED().

This code should match the style of the rest of the engine.

[chrishtr, 2014-03-20 00:08:35]

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidationTreeWalk.h (right):

https://codereview.chromium.org/204873003/diff/60001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidationTreeWalk.h:14: static void
computeStyleInvalidation(Document&, RuleFeatureSet*);
On 2014/03/19 23:04:14, esprehn wrote:
> That's not the correct way to solve this. We have lots of these objects. If
> you're really paranoid use STACK_ALLOCATED().
> 
> This code should match the style of the rest of the engine.

Matching style is good. Done.

[esprehn, 2014-03-20 00:33:02]

Awesome, one nit. lgtm

https://codereview.chromium.org/204873003/diff/140001/Source/core/css/invalid...
File Source/core/css/invalidation/StyleInvalidator.h (right):

https://codereview.chromium.org/204873003/diff/140001/Source/core/css/invalid...
Source/core/css/invalidation/StyleInvalidator.h:16: StyleInvalidator(Document&);
explicit

[chrishtr, 2014-03-20 03:46:32]

Thanks!

https://codereview.chromium.org/204873003/diff/140001/Source/core/css/invalid...
File Source/core/css/invalidation/StyleInvalidator.h (right):

https://codereview.chromium.org/204873003/diff/140001/Source/core/css/invalid...
Source/core/css/invalidation/StyleInvalidator.h:16: StyleInvalidator(Document&);
On 2014/03/20 00:33:03, esprehn wrote:
> explicit

Done.

[chrishtr, 2014-03-20 03:46:42]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-03-20 03:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/204873003/160001

[commit-bot: I haz the power, 2014-03-20 04:25:22]

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

[commit-bot: I haz the power, 2014-03-20 04:25:23]

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

[chrishtr, 2014-03-20 15:47:31]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-03-20 15:47:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/204873003/160001

[commit-bot: I haz the power, 2014-03-20 16:29:19]

Change committed as 169663