Rebuild PropertyTrees When Layer Attach to New LayerTreeHost

Rebuild PropertyTrees When Layer Attach to New LayerTreeHost

When detach a layer from old LayerTreeHost and attach it to new one,
both old and new LayerTreeHost need to rebuild their property trees.
Layer now has stale indices for property trees, invalidate them.

Old LayerTreeHost also needs to be rebuilt because detaching a layer
might change the property trees' structures.

BUG=497952
R=enne, vollick
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c4d2e74d6d68fd7ed55ec11df3151ec4208dfffa
Cr-Commit-Position: refs/heads/master@{#333417}

[enne (OOO), 2015-06-08 21:49:00]

lgtm

[Ian Vollick, 2015-06-09 00:18:51]

https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc#newcode138
cc/layers/layer.cc:138: InvalidatePropertyTreesIndices();
Is there a chance that we're detaching/attaching a subtree rather than a single
layer?

If we'll never have a tiny subtree, then please DCHECK that there can be no
children.

If we could have a subtree, then you're looking at a walk over that subtree to
set all the indices to -1 which would be unfortunate -- the sequence numbers
were added so we could avoid that very thing
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...).
It seems like setting needs_rebuild = true implies that all the indices are
dirty. Perhaps PropertyTrees should stop being a dumb struct and define a
SetNeedsRebuild fn that sets the needs rebuild bit as well as bumping the
sequence number?

[enne (OOO), 2015-06-09 00:22:31]

On 2015/06/09 at 00:18:51, vollick wrote:
> https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc#newcode138
> cc/layers/layer.cc:138: InvalidatePropertyTreesIndices();
> Is there a chance that we're detaching/attaching a subtree rather than a
single layer?
> 
> If we'll never have a tiny subtree, then please DCHECK that there can be no
children.
> 
> If we could have a subtree, then you're looking at a walk over that subtree to
set all the indices to -1 which would be unfortunate -- the sequence numbers
were added so we could avoid that very thing
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...).
It seems like setting needs_rebuild = true implies that all the indices are
dirty. Perhaps PropertyTrees should stop being a dumb struct and define a
SetNeedsRebuild fn that sets the needs rebuild bit as well as bumping the
sequence number?

The subtree gets handled recursively by SetLayerTreeHost.  This function calls
SetLayerTreeHost on all of its children, so they all get set to invalid indices
already.

[Ian Vollick, 2015-06-09 00:23:12]

On 2015/06/09 at 00:22:31, enne wrote:
> On 2015/06/09 at 00:18:51, vollick wrote:
> > https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc
> > File cc/layers/layer.cc (right):
> > 
> >
https://codereview.chromium.org/1166233002/diff/1/cc/layers/layer.cc#newcode138
> > cc/layers/layer.cc:138: InvalidatePropertyTreesIndices();
> > Is there a chance that we're detaching/attaching a subtree rather than a
single layer?
> > 
> > If we'll never have a tiny subtree, then please DCHECK that there can be no
children.
> > 
> > If we could have a subtree, then you're looking at a walk over that subtree
to set all the indices to -1 which would be unfortunate -- the sequence numbers
were added so we could avoid that very thing
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...).
It seems like setting needs_rebuild = true implies that all the indices are
dirty. Perhaps PropertyTrees should stop being a dumb struct and define a
SetNeedsRebuild fn that sets the needs rebuild bit as well as bumping the
sequence number?
> 
> The subtree gets handled recursively by SetLayerTreeHost.  This function calls
SetLayerTreeHost on all of its children, so they all get set to invalid indices
already.

That's awesome. Then this seems a-ok to me. lgtm!

[enne (OOO), 2015-06-09 00:25:58]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-06-09 00:26:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1166233002/1

[commit-bot: I haz the power, 2015-06-09 01:21:43]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-09 01:22:26]

Patchset 1 (id:??) landed as
https://crrev.com/c4d2e74d6d68fd7ed55ec11df3151ec4208dfffa
Cr-Commit-Position: refs/heads/master@{#333417}