cc : Rebuild property trees when opacity changes only if its required.

cc : Rebuild property trees when opacity changes only if its required.

If the change in opacity is from a non-1 value to a non-1 value we need
not rebuild property trees as an effect node already exists and we only
need to update it.

BUG=611903
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8d6efe2e2ad947cce26006896b87d594a4885c80
Cr-Commit-Position: refs/heads/master@{#394131}

[jaydasika, 2016-05-16 20:13:35]

Description was changed from

==========
cc : Rebuild property trees when opacity changes only if its required.

If the change in opacity is from a non-1 value to a non-1 value we need
not rebuild property trees as an effect node already exists and we only
need to update it.

BUG=611903
==========

to

==========
cc : Rebuild property trees when opacity changes only if its required.

If the change in opacity is from a non-1 value to a non-1 value we need
not rebuild property trees as an effect node already exists and we only
need to update it.

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

[jaydasika, 2016-05-16 20:16:06]

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

[jaydasika, 2016-05-16 20:16:06]

PTAL

[ajuma, 2016-05-16 22:05:20]

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

https://codereview.chromium.org/1979413002/diff/1/cc/layers/layer.cc#newcode470
cc/layers/layer.cc:470: bool force_rebuild = opacity == 1.f || opacity_ == 1.f;
Please add a comment explaining why we need to unconditionally rebuild in this
case.

https://codereview.chromium.org/1979413002/diff/1/cc/layers/layer.cc#newcode472
cc/layers/layer.cc:472: SetSubtreePropertyChanged();
I think this is only needed if we rebuild (Layer::SetTransform also calls this
even when not rebuilding, and that seems unnecessary too). Please move this call
and update the test to check for this.

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9668:
root->SetForceRenderSurfaceForTesting(true);
Is this line needed? (Won't the root always get a surface?)

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9674: PropertyTrees* property_trees
= root->layer_tree_host()->property_trees();
"host()->property_trees()" here and in a couple places below (no need for
indirection through the root layer)

[jaydasika, 2016-05-17 01:40:01]

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

https://codereview.chromium.org/1979413002/diff/1/cc/layers/layer.cc#newcode470
cc/layers/layer.cc:470: bool force_rebuild = opacity == 1.f || opacity_ == 1.f;
On 2016/05/16 22:05:20, ajuma wrote:
> Please add a comment explaining why we need to unconditionally rebuild in this
> case.

Done.

https://codereview.chromium.org/1979413002/diff/1/cc/layers/layer.cc#newcode472
cc/layers/layer.cc:472: SetSubtreePropertyChanged();
On 2016/05/16 22:05:20, ajuma wrote:
> I think this is only needed if we rebuild (Layer::SetTransform also calls this
> even when not rebuilding, and that seems unnecessary too). Please move this
call
> and update the test to check for this.

I think we will need to mark both on the layer and property trees because if we
rebuild property trees for some other reason, we should not loose the damage
tracking info.

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9668:
root->SetForceRenderSurfaceForTesting(true);
On 2016/05/16 22:05:20, ajuma wrote:
> Is this line needed? (Won't the root always get a surface?)

Done.

https://codereview.chromium.org/1979413002/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9674: PropertyTrees* property_trees
= root->layer_tree_host()->property_trees();
On 2016/05/16 22:05:20, ajuma wrote:
> "host()->property_trees()" here and in a couple places below (no need for
> indirection through the root layer)

Done.

https://codereview.chromium.org/1979413002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (left):

https://codereview.chromium.org/1979413002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:548: PropertyTrees::ResetFlags::TRANSFORM_TREE);
TRANSFORM_TREE flag is used only here outside tests. I will remove it in a
follow-up

[ajuma, 2016-05-17 15:11:43]

Thanks, lgtm

[jaydasika, 2016-05-17 15:33:40]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-05-17 15:33:55]

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

[commit-bot: I haz the power, 2016-05-17 15:37:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-17 15:39:42]

Description was changed from

==========
cc : Rebuild property trees when opacity changes only if its required.

If the change in opacity is from a non-1 value to a non-1 value we need
not rebuild property trees as an effect node already exists and we only
need to update it.

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

to

==========
cc : Rebuild property trees when opacity changes only if its required.

If the change in opacity is from a non-1 value to a non-1 value we need
not rebuild property trees as an effect node already exists and we only
need to update it.

BUG=611903
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8d6efe2e2ad947cce26006896b87d594a4885c80
Cr-Commit-Position: refs/heads/master@{#394131}
==========

[commit-bot: I haz the power, 2016-05-17 15:39:43]

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