cc : Use effect node data to determine which opacity wins at commit.

cc : Use effect node data to determine which opacity wins at commit.

Since the effect tree tracks whether there is a currently running
opaciy animation, it can be used to determine whether the opacity in
effect tree node should be overwriiten at commit time.

BUG=609205
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2368fd5868423ab3d0bd38ff968c65b19089065f
Cr-Commit-Position: refs/heads/master@{#393029}

[jaydasika, 2016-05-10 23:23:23]

Description was changed from

==========
cc : Use effect node data to determine which opacity wins at commit.

Since the effect tree tracks whether there is a currently running
opaciy animation, it can be used to determine whether the opacity in
effect tree node should be overwriiten at commit time.

BUG=609205
==========

to

==========
cc : Use effect node data to determine which opacity wins at commit.

Since the effect tree tracks whether there is a currently running
opaciy animation, it can be used to determine whether the opacity in
effect tree node should be overwriiten at commit time.

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

[jaydasika, 2016-05-10 23:38:45]

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

[jaydasika, 2016-05-10 23:38:46]

PTAL

https://codereview.chromium.org/1968803002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (left):

https://codereview.chromium.org/1968803002/diff/1/cc/layers/layer_impl.cc#old...
cc/layers/layer_impl.cc:803: return;
Cleanup from the last CL.

https://codereview.chromium.org/1968803002/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (left):

https://codereview.chromium.org/1968803002/diff/1/cc/layers/layer_impl.h#oldc...
cc/layers/layer_impl.h:559: 
Clean up unrelated to this CL

[ajuma, 2016-05-11 00:11:00]

Generally looks good.

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:570: if (LayerImpl* layer =
LayerById(layer_id_to_opacity.first)) {
We'll need a map from layer ids to node ids (which will eventually become a map
from stable node ids to node ids) so that we don't need to find the owning layer
here, but that can be done in a separate CL.

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:581: opacity_animations_map_.clear();
What kind of test coverage do we have for this logic right now? Are there tests
that fail if we don't update the value in the property tree node, or if we don't
clear the map?

[jaydasika, 2016-05-11 01:00:54]

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:570: if (LayerImpl* layer =
LayerById(layer_id_to_opacity.first)) {
On 2016/05/11 00:10:59, ajuma wrote:
> We'll need a map from layer ids to node ids (which will eventually become a
map
> from stable node ids to node ids) so that we don't need to find the owning
layer
> here, but that can be done in a separate CL.

Acknowledged.

https://codereview.chromium.org/1968803002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:581: opacity_animations_map_.clear();
On 2016/05/11 00:11:00, ajuma wrote:
> What kind of test coverage do we have for this logic right now? Are there
tests
> that fail if we don't update the value in the property tree node, or if we
don't
> clear the map?

There is only one unit test hitting this path :
LayerTreeHostAnimationTestCheckerboardDoesntStartAnimations. I have added one
more.

[ajuma, 2016-05-11 17:22:29]

lgtm

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:855: // Changing opacity forces us to
rebuild proeprty trees. We want to
Hmm! If the layer whose opacity changed already has an effect node, we shouldn't
need to rebuild. But that's unrelated to this CL so can be fixed in follow-up.

Also, s/proeprty/property

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:862:
layer_tree_host()->property_trees()->needs_rebuild = true;
How about directly setting is_currently_animating to false instead of
rebuilding?

[jaydasika, 2016-05-11 18:08:45]

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:855: // Changing opacity forces us to
rebuild proeprty trees. We want to
On 2016/05/11 17:22:28, ajuma wrote:
> Hmm! If the layer whose opacity changed already has an effect node, we
shouldn't
> need to rebuild. But that's unrelated to this CL so can be fixed in follow-up.
> 
> Also, s/proeprty/property

Setting opacity on a layer doesn't set opacity on the effect tree immediately.
Instead, it depends on the rebuild of property trees to do that. So, I have
changed this test to change the opacity on the effect node directly (so that the
rebuild in case 2 can be removed). But I agree, we shouldn't be rebuilding if
the layer already has an effect node.

https://codereview.chromium.org/1968803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:862:
layer_tree_host()->property_trees()->needs_rebuild = true;
On 2016/05/11 17:22:28, ajuma wrote:
> How about directly setting is_currently_animating to false instead of
> rebuilding?

Done.

[jaydasika, 2016-05-11 19:09:00]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-05-11 19:09:01]

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/1968803002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-11 19:09:38]

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

[commit-bot: I haz the power, 2016-05-11 19:38:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-11 19:41:05]

Description was changed from

==========
cc : Use effect node data to determine which opacity wins at commit.

Since the effect tree tracks whether there is a currently running
opaciy animation, it can be used to determine whether the opacity in
effect tree node should be overwriiten at commit time.

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

to

==========
cc : Use effect node data to determine which opacity wins at commit.

Since the effect tree tracks whether there is a currently running
opaciy animation, it can be used to determine whether the opacity in
effect tree node should be overwriiten at commit time.

BUG=609205
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2368fd5868423ab3d0bd38ff968c65b19089065f
Cr-Commit-Position: refs/heads/master@{#393029}
==========

[commit-bot: I haz the power, 2016-05-11 19:41:06]

Patchset 3 (id:??) landed as
https://crrev.com/2368fd5868423ab3d0bd38ff968c65b19089065f
Cr-Commit-Position: refs/heads/master@{#393029}