cc :: Move tracking of layer_property_changed to main thread (2)

cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002.
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[jaydasika, 2016-02-12 20:26:32]

Description was changed from

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to crrev.com/1688563002. Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
==========

to

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to crrev.com/1688563002. Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-02-12 22:00:30]

Description was changed from

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to crrev.com/1688563002. Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. Properties moved in this CL
are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-02-12 22:00:54]

Description was changed from

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. Properties moved in this CL
are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. 
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-02-12 22:02:10]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, enne@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-02-12 22:02:10]

With this CL all properties that don't change on the compositor are moved.

[ajuma, 2016-02-12 23:57:04]

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1384: transform_node->data.transform_changed = false;
When updating the transform tree, this gets propagated to descendants, so those
descendants also need to get this cleared (and their owning layers won't
necessarily be pushing properties). Perhaps this clearing should happen when
pushing property trees rather than when pushing layers (so just walk the
transform tree and clear this bit on every node?).

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:712: transform_node->data.transform_changed = false;
Same comment as in Layer::PushProperties.

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:842: transform_node->data.transform_changed = false;
An alternative to doing this layer-by-layer would be to clear this on the entire
transform tree at the same time (and call this in LayerTreeHostImpl::DrawLayers,
at the same place where we call RestAllChangeTrackingForSubtree on the root).

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1040: NoteLayerPropertyChanged();
Is this still needed? LayerImpl::SetBounds should (outside of tests) only get
called during PushProperties, in which case we've already accounted for the
change on the other tree.

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:979: node->data.transform_changed = true;
There's another case to consider, I think: if node->parent_id !=
node->data.source_node_id and source_to_parent_updates_allowed() is true, then
we also need to check if the source_node's transform changed (since such a
change can move this node too). Please add a test for this.

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:138: bool transform_changed : 1;
Please add a comment explaining what this means (e.g., which transform changed,
when it changed, why we need to keep track).

[jaydasika, 2016-02-16 22:30:36]

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1384: transform_node->data.transform_changed = false;
On 2016/02/12 23:57:03, ajuma wrote:
> When updating the transform tree, this gets propagated to descendants, so
those
> descendants also need to get this cleared (and their owning layers won't
> necessarily be pushing properties). Perhaps this clearing should happen when
> pushing property trees rather than when pushing layers (so just walk the
> transform tree and clear this bit on every node?).

Done.

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:712: transform_node->data.transform_changed = false;
On 2016/02/12 23:57:03, ajuma wrote:
> Same comment as in Layer::PushProperties.

Done.

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:842: transform_node->data.transform_changed = false;
On 2016/02/12 23:57:03, ajuma wrote:
> An alternative to doing this layer-by-layer would be to clear this on the
entire
> transform tree at the same time (and call this in
LayerTreeHostImpl::DrawLayers,
> at the same place where we call RestAllChangeTrackingForSubtree on the root).

Done.

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1040: NoteLayerPropertyChanged();
On 2016/02/12 23:57:03, ajuma wrote:
> Is this still needed? LayerImpl::SetBounds should (outside of tests) only get
> called during PushProperties, in which case we've already accounted for the
> change on the other tree.

In Layer, we set SubtreePropertyChanged to true only when mask_to_bounds is
true. So, we need to handle the other case here. (And we can't to do this on
Layer because Layer currently supports changing entire subtree, not individual
layers).

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:979: node->data.transform_changed = true;
On 2016/02/12 23:57:03, ajuma wrote:
> There's another case to consider, I think: if node->parent_id !=
> node->data.source_node_id and source_to_parent_updates_allowed() is true, then
> we also need to check if the source_node's transform changed (since such a
> change can move this node too). Please add a test for this.

Done.

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:138: bool transform_changed : 1;
On 2016/02/12 23:57:03, ajuma wrote:
> Please add a comment explaining what this means (e.g., which transform
changed,
> when it changed, why we need to keep track).

Done.

[ajuma, 2016-02-16 22:55:50]

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1040: NoteLayerPropertyChanged();
On 2016/02/16 22:30:36, jaydasika wrote:
> On 2016/02/12 23:57:03, ajuma wrote:
> > Is this still needed? LayerImpl::SetBounds should (outside of tests) only
get
> > called during PushProperties, in which case we've already accounted for the
> > change on the other tree.
> 
> In Layer, we set SubtreePropertyChanged to true only when mask_to_bounds is
> true. So, we need to handle the other case here. (And we can't to do this on
> Layer because Layer currently supports changing entire subtree, not individual
> layers). 

Ah, makes sense. Please add a comment about this (or just remove the 'if' and
call NoteLayerPropertyChanged unconditionally).

https://codereview.chromium.org/1697613002/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:787: if (node && node->data.transform_changed)
Do we still have cases where layers don't have transform nodes?

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree...
cc/trees/property_tree.cc:991: node->data.transform_changed = true;
What about if just the parent node changes (and the source node doesn't)? That
still has the effect of changing the to_screen transform.

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree...
cc/trees/property_tree.h:138: // When the transform changes, we need to track it
to compute the damage rect.
This should point out that it's not just the local transform we care about, but
rather the transform to screen.

[jaydasika, 2016-02-16 23:08:22]

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree...
cc/trees/property_tree.cc:991: node->data.transform_changed = true;
On 2016/02/16 22:55:50, ajuma wrote:
> What about if just the parent node changes (and the source node doesn't)? That
> still has the effect of changing the to_screen transform.

Yes, to_screen will change in that case. But, I don't think we were handling
that case before this CL. Do you think that's a bug ?

[ajuma, 2016-02-17 00:05:33]

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree...
cc/trees/property_tree.cc:991: node->data.transform_changed = true;
On 2016/02/16 23:08:22, jaydasika wrote:
> On 2016/02/16 22:55:50, ajuma wrote:
> > What about if just the parent node changes (and the source node doesn't)?
That
> > still has the effect of changing the to_screen transform.
> 
> Yes, to_screen will change in that case. But, I don't think we were handling
> that case before this CL. Do you think that's a bug ?

Yes, if a layer's to_screen transform changed, then the layer moved, so it
should be treated as damaged.

[jaydasika, 2016-02-17 00:32:36]

https://codereview.chromium.org/1697613002/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:787: if (node && node->data.transform_changed)
On 2016/02/16 22:55:50, ajuma wrote:
> Do we still have cases where layers don't have transform nodes?

Yes, we have many such cases in tests.

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1697613002/diff/100001/cc/trees/property_tree...
cc/trees/property_tree.h:138: // When the transform changes, we need to track it
to compute the damage rect.
On 2016/02/16 22:55:50, ajuma wrote:
> This should point out that it's not just the local transform we care about,
but
> rather the transform to screen.

Done.

[ajuma, 2016-02-17 00:40:01]

https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:988: !source_to_parent_updates_allowed_))
The second condition can prevent checking if the parent_node's transform has
changed. How about separating that out (using it only to determine if we should
check source_node->data.transform_changed)?

[jaydasika, 2016-02-17 00:49:38]

https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:988: !source_to_parent_updates_allowed_))
On 2016/02/17 00:40:01, ajuma wrote:
> The second condition can prevent checking if the parent_node's transform has
> changed. How about separating that out (using it only to determine if we
should
> check source_node->data.transform_changed)?

Done.

[ajuma, 2016-02-17 00:52:17]

On 2016/02/17 00:49:38, jaydasika wrote:
>
https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree.cc
> File cc/trees/property_tree.cc (right):
> 
>
https://codereview.chromium.org/1697613002/diff/120001/cc/trees/property_tree...
> cc/trees/property_tree.cc:988: !source_to_parent_updates_allowed_))
> On 2016/02/17 00:40:01, ajuma wrote:
> > The second condition can prevent checking if the parent_node's transform has
> > changed. How about separating that out (using it only to determine if we
> should
> > check source_node->data.transform_changed)?
> 
> Done.

Thanks, lgtm.

[jaydasika, 2016-02-17 01:59:59]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-02-17 02:00:15]

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1697613002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1040: NoteLayerPropertyChanged();
On 2016/02/16 22:55:50, ajuma wrote:
> On 2016/02/16 22:30:36, jaydasika wrote:
> > On 2016/02/12 23:57:03, ajuma wrote:
> > > Is this still needed? LayerImpl::SetBounds should (outside of tests) only
> get
> > > called during PushProperties, in which case we've already accounted for
the
> > > change on the other tree.
> > 
> > In Layer, we set SubtreePropertyChanged to true only when mask_to_bounds is
> > true. So, we need to handle the other case here. (And we can't to do this on
> > Layer because Layer currently supports changing entire subtree, not
individual
> > layers). 
> 
> Ah, makes sense. Please add a comment about this (or just remove the 'if' and
> call NoteLayerPropertyChanged unconditionally).

Forgot adding a comment here, will add it in the next CL.

[commit-bot: I haz the power, 2016-02-17 02:04:29]

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

[commit-bot: I haz the power, 2016-02-17 02:46:18]

Description was changed from

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. 
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. 
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-17 02:46:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-17 02:47:11]

Description was changed from

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002. 
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc :: Move tracking of layer_property_changed to main thread (2)

This is a follow up to http://crrev.com/1688563002.
Properties moved in this CL are :
bounds, position, transform_origin

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/adfec8e15d041e310fd741e34db1589777236112
Cr-Commit-Position: refs/heads/master@{#375768}
==========

[commit-bot: I haz the power, 2016-02-17 02:47:12]

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