cc :: Move some tracking of layer_property_changed to main thread.

cc :: Move some tracking of layer_property_changed to main thread.

This CL computes layer_property_changed for some properties that do not change
on the compositor on the main thread and pushes the computed value to
compositor. This is necessary to make computation of layer_property_changed on
compositor independant of the layer hierarchy information.

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03a99c40d0ef00bc160dee70c11f230927412b4d
Cr-Commit-Position: refs/heads/master@{#374983}

[jaydasika, 2016-02-10 00:41:39]

Description was changed from

==========
cc :: Move some tracking of layer_property_changed to main thread.

This CL computes layer_property_changed for some properties that do not change
on the compositor on the main thread and pushes the computed value to
compositor. This is necessary to make computation of layer_property_changed on
compositor independant of the layer hierarchy information.

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

to

==========
cc :: Move some tracking of layer_property_changed to main thread.

This CL computes layer_property_changed for some properties that do not change
on the compositor on the main thread and pushes the computed value to
compositor. This is necessary to make computation of layer_property_changed on
compositor independant of the layer hierarchy information.

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

[jaydasika, 2016-02-10 01:00:15]

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

[jaydasika, 2016-02-10 01:00:16]

This CL moves layer_property_changed tracking of all properties that do not
change on compositor (except bounds, position, transform_origin) to main thread.

[ajuma, 2016-02-10 15:31:32]

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

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1237: // to all descendants. So, it is enough to check it
only for the current
Also say why it will have already propagated to descendants.

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:61:
EXPECT_TRUE(grand_child->subtree_property_changed());
Also verify that needs_push_properties is true for each of these layers.

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:525:
EXECUTE_AND_VERIFY_SUBTREE_CHANGED(root->SetMaskLayer(dummy_layer1.get()));
Does the subtree_property_changed() bit get cleared anywhere between these
tests? (If not, the remaining tests would pass as long as the first one set
subtree_property_changed on everything.)

[jaydasika, 2016-02-10 16:33:40]

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:61:
EXPECT_TRUE(grand_child->subtree_property_changed());
On 2016/02/10 15:31:32, ajuma wrote:
> Also verify that needs_push_properties is true for each of these layers.

Do we need to push properties if layer_property_changed is the only change? (I
mean for cases we weren't pushing properties before this CL)

[ajuma, 2016-02-10 16:40:18]

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:61:
EXPECT_TRUE(grand_child->subtree_property_changed());
On 2016/02/10 16:33:40, jaydasika wrote:
> On 2016/02/10 15:31:32, ajuma wrote:
> > Also verify that needs_push_properties is true for each of these layers.
> 
> Do we need to push properties if layer_property_changed is the only change? (I
> mean for cases we weren't pushing properties before this CL) 

If we don't push properties, would the layer_property_changed_ of the
corresponding LayerImpl get set some other way?

[jaydasika, 2016-02-10 19:18:25]

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

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1237: // to all descendants. So, it is enough to check it
only for the current
On 2016/02/10 15:31:32, ajuma wrote:
> Also say why it will have already propagated to descendants.

Done.

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:61:
EXPECT_TRUE(grand_child->subtree_property_changed());
On 2016/02/10 16:40:18, ajuma wrote:
> On 2016/02/10 16:33:40, jaydasika wrote:
> > On 2016/02/10 15:31:32, ajuma wrote:
> > > Also verify that needs_push_properties is true for each of these layers.
> > 
> > Do we need to push properties if layer_property_changed is the only change?
(I
> > mean for cases we weren't pushing properties before this CL) 
> 
> If we don't push properties, would the layer_property_changed_ of the
> corresponding LayerImpl get set some other way?

Added SetNeedsPushProperties to SetSubtreeProeprtyChanged and added the checks
here.

https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:525:
EXECUTE_AND_VERIFY_SUBTREE_CHANGED(root->SetMaskLayer(dummy_layer1.get()));
On 2016/02/10 15:31:32, ajuma wrote:
> Does the subtree_property_changed() bit get cleared anywhere between these
> tests? (If not, the remaining tests would pass as long as the first one set
> subtree_property_changed on everything.)

Added code to reset it.

[ajuma, 2016-02-10 19:43:18]

On 2016/02/10 19:18:25, jaydasika wrote:
> https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:1237: // to all descendants. So, it is enough to check it
> only for the current
> On 2016/02/10 15:31:32, ajuma wrote:
> > Also say why it will have already propagated to descendants.
> 
> Done.
> 
>
https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
> File cc/layers/layer_unittest.cc (right):
> 
>
https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
> cc/layers/layer_unittest.cc:61:
> EXPECT_TRUE(grand_child->subtree_property_changed());
> On 2016/02/10 16:40:18, ajuma wrote:
> > On 2016/02/10 16:33:40, jaydasika wrote:
> > > On 2016/02/10 15:31:32, ajuma wrote:
> > > > Also verify that needs_push_properties is true for each of these layers.
> > > 
> > > Do we need to push properties if layer_property_changed is the only
change?
> (I
> > > mean for cases we weren't pushing properties before this CL) 
> > 
> > If we don't push properties, would the layer_property_changed_ of the
> > corresponding LayerImpl get set some other way?
> 
> Added SetNeedsPushProperties to SetSubtreeProeprtyChanged and added the checks
> here.
> 
>
https://codereview.chromium.org/1688563002/diff/40001/cc/layers/layer_unittes...
> cc/layers/layer_unittest.cc:525:
> EXECUTE_AND_VERIFY_SUBTREE_CHANGED(root->SetMaskLayer(dummy_layer1.get()));
> On 2016/02/10 15:31:32, ajuma wrote:
> > Does the subtree_property_changed() bit get cleared anywhere between these
> > tests? (If not, the remaining tests would pass as long as the first one set
> > subtree_property_changed on everything.)
> 
> Added code to reset it.

Thanks, lgtm.

[jaydasika, 2016-02-11 01:48:38]

Some CHECKs were failing on some bots because of not clearing
needs_push_properties of mask and replica layer. PTAL at the new changes.

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:552:
dummy_layer1->PushPropertiesTo(dummy_layer1_impl.get()));
When we add the mask layer, we need to push it also.

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:574:
dummy_layer2->PushPropertiesTo(dummy_layer1_impl.get()));
Adding replica layer and we need to push its properties too.

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:589:
dummy_layer2->PushPropertiesTo(dummy_layer1_impl.get()));
Replica layer gets a transform node (because its at boundary of 3d rendering
context) and so we need to push its properties here.

[ajuma, 2016-02-11 14:22:16]

lgtm

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:574:
dummy_layer2->PushPropertiesTo(dummy_layer1_impl.get()));
On 2016/02/11 01:48:38, jaydasika wrote:
> Adding replica layer and we need to push its properties too.

Nit: push to layer2, not layer1. Actually, it might be clearer to push both of
the layers (since that's what would happen in reality, right?).

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:589:
dummy_layer2->PushPropertiesTo(dummy_layer1_impl.get()));
On 2016/02/11 01:48:38, jaydasika wrote:
> Replica layer gets a transform node (because its at boundary of 3d rendering
> context) and so we need to push its properties here.

Here too, push to layer2, and it might be clearer to push both the layers.

[jaydasika, 2016-02-11 19:24:24]

Summary of discussion with ajuma@ :

Masks and Replicas are not damaged when the owning layer gets damaged. This is
because masks don't change whether a layer is damaged and replicas are tracked
explicitly by damage tracker, so we need not track them here.

This matches both the current behavior and this patch.

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1688563002/diff/100001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:589:
dummy_layer2->PushPropertiesTo(dummy_layer1_impl.get()));
On 2016/02/11 14:22:16, ajuma wrote:
> On 2016/02/11 01:48:38, jaydasika wrote:
> > Replica layer gets a transform node (because its at boundary of 3d rendering
> > context) and so we need to push its properties here.
> 
> Here too, push to layer2, and it might be clearer to push both the layers.

Done.

[jaydasika, 2016-02-11 19:24:36]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-02-11 19:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1688563002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-02-11 19:27:06]

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

[commit-bot: I haz the power, 2016-02-11 21:07:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-16 22:37:59]

Description was changed from

==========
cc :: Move some tracking of layer_property_changed to main thread.

This CL computes layer_property_changed for some properties that do not change
on the compositor on the main thread and pushes the computed value to
compositor. This is necessary to make computation of layer_property_changed on
compositor independant of the layer hierarchy information.

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

to

==========
cc :: Move some tracking of layer_property_changed to main thread.

This CL computes layer_property_changed for some properties that do not change
on the compositor on the main thread and pushes the computed value to
compositor. This is necessary to make computation of layer_property_changed on
compositor independant of the layer hierarchy information.

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03a99c40d0ef00bc160dee70c11f230927412b4d
Cr-Commit-Position: refs/heads/master@{#374983}
==========

[commit-bot: I haz the power, 2016-02-16 22:37:59]

Patchset 7 (id:??) landed as
https://crrev.com/03a99c40d0ef00bc160dee70c11f230927412b4d
Cr-Commit-Position: refs/heads/master@{#374983}