Implement incremental paint property tree rebuilding

Implement incremental paint property tree rebuilding

This patch implements incremental paint property tree rebuilding based
on an invalidation scheme similar to paint invalidation. When property-
affecting values change we set FrameView/LayoutObject as needing a property
tree update. At the moment all property node updates force the entire property
subtree to be rebuilt, but TODOs have been added to add more granular
updates.

When no property invalidation occurs we still need to walk the property
trees update the builder context, but we can re-use existing nodes
instead of reconstructing them.

An underinvalidation checking mechanism has been added for debug
builds to catch cases where we miss a call to set an object as
needing a property tree update.

DO NOT COMMIT. This will be broken up into smaller patches.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[pdr., 2016-10-12 06:06:26]

Description was changed from

==========
WIP: Implement partial paint property tree rebuilding

This is not yet ready for review.
==========

to

==========
WIP: Implement partial paint property tree rebuilding

This is not yet ready for review.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-10-12 07:41:19]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 07:41:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 07:44:21]

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

[commit-bot: I haz the power, 2016-10-12 07:44:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-10-21 18:30:47]

Description was changed from

==========
WIP: Implement partial paint property tree rebuilding

This is not yet ready for review.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement incremental paint property tree rebuilding

This patch implements incremental paint property tree rebuilding based
on an invalidation scheme similar to paint invalidation. When property-
affecting values change we mark FrameView/LayoutObject as having invalid
paint properties, then rebuild the entire subtree.

When no property invalidation occurs we still need to walk the property
trees to keep the context up-to-date, but we can re-use existing nodes
instead of reconstructing them. A future patch will completely prune
the prepaint treewalk when no property tree invalidation or paint
invalidation occurs.

An underinvalidation checking mechanism has been added for debug
builds to catch cases where we miss a call to invalidate properties.


DO NOT COMMIT. This will be broken up into bite-sized patches.


BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-10-21 18:41:56]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wangxianzhu@chromium.org

[pdr., 2016-10-21 18:41:57]

@Chrishtr, this is ready for a high-level review before you head off on vacation
next week.

@WangXianzhu, could you also review this at a high-level?

Roughly 20 tests fail with this patch and there are a few TODOs still left to
squash. I still need to support invalidating main thread scrolling reasons which
will need to walk up to the nearest containing block. I'm mostly looking for a
second pair of eyes that the plan listed in the change description is a good
one.

Assuming this looks good to you all, my plan is to get all tests passing and
then cut this patch up into 2-3 smaller patches that are easier to review.

[chrishtr, 2016-10-21 22:03:09]

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3381:
setPaintPropertiesInvalid();

Add a brief comment explaining which property tree nodes are invalidated by
changed frame rect.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3620:
setPaintPropertiesInvalid();

Add a brief comment explaining which property tree nodes are invalidated by
changed scroll.

(it's the scroll offset one right? and it only needs its matrix contents
adjusted?)

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:726: void
setPaintPropertiesInvalid() { m_paintPropertiesValid = false; }
...Invalid is different nomenclature from invalidations on other tree walks for
layout, paint invalidation, etc.
What common naming scheme should we use?

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:213:
std::unique_ptr<ObjectPaintProperties> clone() const {
It would be cool to make this with copy constructors to make it future-proof to
added objects. But
I think that might not be possible because you'd have to subclass RefPtr or
something.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:147: #ifndef
NDEBUG
#if DCHECK_IS_ON

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:155: if
(properties && properties->paintOffsetTranslation())
This conditional is not needed, and therefore line 156 and 163 can be combined
into a single line at 169.

It appears you can do the same kind of thing in some/most of the other sites, so
the general code structure would
be:

if (!paintPropertiesValid) {
  update the property tree nodes
}

unconditionally update the context.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:307: static
FloatPoint3D transformOrigin(const LayoutBox& box) {
This method is now getting very long, refactor it?

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:463:
properties->clearLocalBorderBoxProperties();
Why is this change needed but wasn't in the old code?

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:79: if
(!localContext.needToRebuildPaintPropertyTrees) {
Perhaps add a TODO somewhere about unifying the paint invalidation and property
tree bits for SPv2.
Though this would have the side-effect of always requiring paint properties
rebuild if paint was
invalidated, and vice-versa...

[chrishtr, 2016-10-21 22:08:18]

On 2016/10/21 at 22:03:09, chrishtr wrote:
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:3381:
setPaintPropertiesInvalid();
> 
> Add a brief comment explaining which property tree nodes are invalidated by
changed frame rect.
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:3620:
setPaintPropertiesInvalid();
> 
> Add a brief comment explaining which property tree nodes are invalidated by
changed scroll.
> 
> (it's the scroll offset one right? and it only needs its matrix contents
adjusted?)
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.h:726: void
setPaintPropertiesInvalid() { m_paintPropertiesValid = false; }
> ...Invalid is different nomenclature from invalidations on other tree walks
for layout, paint invalidation, etc.
> What common naming scheme should we use?
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:213:
std::unique_ptr<ObjectPaintProperties> clone() const {
> It would be cool to make this with copy constructors to make it future-proof
to added objects. But
> I think that might not be possible because you'd have to subclass RefPtr or
something.
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:147: #ifndef
NDEBUG
> #if DCHECK_IS_ON
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:155: if
(properties && properties->paintOffsetTranslation())
> This conditional is not needed, and therefore line 156 and 163 can be combined
into a single line at 169.
> 
> It appears you can do the same kind of thing in some/most of the other sites,
so the general code structure would
> be:
> 
> if (!paintPropertiesValid) {
>   update the property tree nodes
> }
> 
> unconditionally update the context.
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:307: static
FloatPoint3D transformOrigin(const LayoutBox& box) {
> This method is now getting very long, refactor it?
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:463:
properties->clearLocalBorderBoxProperties();
> Why is this change needed but wasn't in the old code?
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):
> 
>
https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:79: if
(!localContext.needToRebuildPaintPropertyTrees) {
> Perhaps add a TODO somewhere about unifying the paint invalidation and
property tree bits for SPv2.
> Though this would have the side-effect of always requiring paint properties
rebuild if paint was
> invalidated, and vice-versa...

http://i.imgur.com/CR0jTty.jpg

[pdr., 2016-10-21 22:09:59]

Just noticed a small optimization we should probably add in this patch: we only
need to force a subtree to be rebuilt if the topology of the tree changes
(either added, removed, or the property tree builder context changes in some
way).

[pdr., 2016-10-26 03:10:49]

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3381:
setPaintPropertiesInvalid();
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> Add a brief comment explaining which property tree nodes are invalidated by
changed frame rect.

Done

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3620:
setPaintPropertiesInvalid();
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> Add a brief comment explaining which property tree nodes are invalidated by
changed scroll.
> 
> (it's the scroll offset one right? and it only needs its matrix contents
adjusted?)

Done. I tightened these invalidations up a bit as well (only in scroll offset
and contents size changed functions now).

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:726: void
setPaintPropertiesInvalid() { m_paintPropertiesValid = false; }
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> ...Invalid is different nomenclature from invalidations on other tree walks
for layout, paint invalidation, etc.
> What common naming scheme should we use?

I've switched this to the following which is similar to layout and paint
invalidation:
setNeedsPropertyUpdate()
clearNeedsPropertyUpdate()

I've also added comments about how we can expand this in the future. Possibly
something along these lines:
setNeedsLocalPropertyUpdate()
setNeedsSubtreePropertyUpdate()
...etc

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:213:
std::unique_ptr<ObjectPaintProperties> clone() const {
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> It would be cool to make this with copy constructors to make it future-proof
to added objects. But
> I think that might not be possible because you'd have to subclass RefPtr or
something.

I played with this for a while but couldn't find a clean implementation using
copy constructors :/ They are not allowed on RefCounted objects, and using a
copy constructor for ObjectPaintProperties ended up being more confusing because
it works differently than the regular properties.

I have put all clone functions (ObjectPaintProperties, and {Transform, Effect,
etc..}PaintPropertyNode) behind debug checks and added comments describing what
they do.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:147: #ifndef
NDEBUG
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> #if DCHECK_IS_ON

Done

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:155: if
(properties && properties->paintOffsetTranslation())
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> This conditional is not needed, and therefore line 156 and 163 can be combined
into a single line at 169.
> 
> It appears you can do the same kind of thing in some/most of the other sites,
so the general code structure would
> be:
> 
> if (!paintPropertiesValid) {
>   update the property tree nodes
> }
> 
> unconditionally update the context.

This approach is very nice.

Done.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:307: static
FloatPoint3D transformOrigin(const LayoutBox& box) {
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> This method is now getting very long, refactor it?

Done. Split out the svg-specific code into its own function.

https://codereview.chromium.org/2404213004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:463:
properties->clearLocalBorderBoxProperties();
On 2016/10/21 at 22:03:09, chrishtr - OOO wrote:
> Why is this change needed but wasn't in the old code?

This is just a cleanup so we don't leave unnecessary localBorderBoxProperties
objects when a object loses a layer.

[pdr., 2016-10-26 03:20:30]

@WangXianzhu, I think this is finally ready for a high-level review. I'd like to
split this into several patches to land so the details don't matter as much as
the overall design.

Question: A trigger for rebuilding properties is
shouldCheckForPaintInvalidationRegardlessOfPaintInvalidationState, but many
things cause this invalidation. Is there a smaller set of paint invalidation
flags that should trigger property tree updates?

[pdr., 2016-10-26 03:23:51]

Description was changed from

==========
Implement incremental paint property tree rebuilding

This patch implements incremental paint property tree rebuilding based
on an invalidation scheme similar to paint invalidation. When property-
affecting values change we mark FrameView/LayoutObject as having invalid
paint properties, then rebuild the entire subtree.

When no property invalidation occurs we still need to walk the property
trees to keep the context up-to-date, but we can re-use existing nodes
instead of reconstructing them. A future patch will completely prune
the prepaint treewalk when no property tree invalidation or paint
invalidation occurs.

An underinvalidation checking mechanism has been added for debug
builds to catch cases where we miss a call to invalidate properties.


DO NOT COMMIT. This will be broken up into bite-sized patches.


BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement incremental paint property tree rebuilding

This patch implements incremental paint property tree rebuilding based
on an invalidation scheme similar to paint invalidation. When property-
affecting values change we set FrameView/LayoutObject as needing a property
tree update. At the moment all property node updates force the entire property
subtree to be rebuilt, but TODOs have been added to add more granular
updates.

When no property invalidation occurs we still need to walk the property
trees update the builder context, but we can re-use existing nodes
instead of reconstructing them.

An underinvalidation checking mechanism has been added for debug
builds to catch cases where we miss a call to set an object as
needing a property tree update.

DO NOT COMMIT. This will be broken up into smaller patches.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-10-26 19:21:51]

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:2152: // 32 bits have been
used in the first word, and 19 in the second.
Update the '19' to '20'.

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h:30: }
Can you just use the constructor at the call sites?

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:320: //
FIXME(pdr): Refactor this so all non-root SVG objects use the same
Nit: s/FIXME/TODO/

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:103:
localContext.needToUpdatePaintPropertySubtree = true;
I think we don't need to set needToUpdatePaintPropertySubtree but just
object.setNeedsPaintPropertyUpdate().

About your question in https://codereview.chromium.org/2404213004/#msg13, I
think we can use mayNeedpaintInvalidation() || shouldDoFullPaintInvalidation()
here to exclude the cases that the object needs checking paint invalidation just
because of having selection and/or childShouldCheckForPaintInvalidation().

[pdr., 2016-10-26 23:10:19]

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h:30: }
On 2016/10/26 at 19:21:51, Xianzhu wrote:
> Can you just use the constructor at the call sites?

Good idea, done.

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:320: //
FIXME(pdr): Refactor this so all non-root SVG objects use the same
On 2016/10/26 at 19:21:51, Xianzhu wrote:
> Nit: s/FIXME/TODO/

Done

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:103:
localContext.needToUpdatePaintPropertySubtree = true;
On 2016/10/26 at 19:21:51, Xianzhu wrote:
> I think we don't need to set needToUpdatePaintPropertySubtree but just
object.setNeedsPaintPropertyUpdate().
> 

Initially I'd like to rebuild entire subtrees anytime
object.needsPaintPropertyUpdate is set. This way we don't yet have to worry
about descendant properties that point to stale properties on this object. I'd
then like to improve this in the future. Does that sound reasonable to you, or
do you think this should be thought out more before landing parts of this patch?

I have two ideas for improving setNeedsPaintPropertyUpdate:
* We could introduce a local-only (or even transform-only, clip-only, etc) bit
such as setNeedsLocalPaintPropertyUpdate that does not rebuild the entire
subtree. This would be for the case when a transform changes, for example.
* Another option is to only rebuild a property subtree if nodes change. If just
the values of property nodes change (as opposed to new ones added or deleted),
all child nodes will still be valid and we do not need to rebuild the subtree at
all.

Does that sound like a good idea? Thoughts welcome.

> About your question in https://codereview.chromium.org/2404213004/#msg13, I
think we can use mayNeedpaintInvalidation() || shouldDoFullPaintInvalidation()
here to exclude the cases that the object needs checking paint invalidation just
because of having selection and/or childShouldCheckForPaintInvalidation().

Updated! Can you take another look at this conditional and see if my comments
can be improved?

[Xianzhu, 2016-10-26 23:19:40]

lgtm

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2404213004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:103:
localContext.needToUpdatePaintPropertySubtree = true;
On 2016/10/26 23:10:19, pdr. wrote:
> On 2016/10/26 at 19:21:51, Xianzhu wrote:
> > I think we don't need to set needToUpdatePaintPropertySubtree but just
> object.setNeedsPaintPropertyUpdate().
> > 
> 
> Initially I'd like to rebuild entire subtrees anytime
> object.needsPaintPropertyUpdate is set. This way we don't yet have to worry
> about descendant properties that point to stale properties on this object. I'd
> then like to improve this in the future. Does that sound reasonable to you, or
> do you think this should be thought out more before landing parts of this
patch?
> 
> I have two ideas for improving setNeedsPaintPropertyUpdate:
> * We could introduce a local-only (or even transform-only, clip-only, etc) bit
> such as setNeedsLocalPaintPropertyUpdate that does not rebuild the entire
> subtree. This would be for the case when a transform changes, for example.
> * Another option is to only rebuild a property subtree if nodes change. If
just
> the values of property nodes change (as opposed to new ones added or deleted),
> all child nodes will still be valid and we do not need to rebuild the subtree
at
> all.
> 
> Does that sound like a good idea? Thoughts welcome.
>

SGTM.

(I overlooked the previous needToUpdatePaintPropertySubtree = true and
misunderstood the above code when writing the previous comment.)

> > About your question in https://codereview.chromium.org/2404213004/#msg13, I
> think we can use mayNeedpaintInvalidation() || shouldDoFullPaintInvalidation()
> here to exclude the cases that the object needs checking paint invalidation
just
> because of having selection and/or childShouldCheckForPaintInvalidation().
> 
> Updated! Can you take another look at this conditional and see if my comments
> can be improved?