cc : Track opacity animation changes on effect tree

cc : Track opacity animation changes on effect tree

BUG=609205
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9cb21c771b787047cf24b129859b8d6636a0c61d
Cr-Commit-Position: refs/heads/master@{#392751}

[jaydasika, 2016-05-05 21:53:12]

Description was changed from

==========
cc : Track if there are any running opacity animations on effect tree

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

to

==========
cc : Track if there are any running opacity animations on effect tree

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

[jaydasika, 2016-05-05 22:20:09]

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

[jaydasika, 2016-05-05 22:20:10]

PTAL

https://codereview.chromium.org/1957533002/diff/20001/cc/proto/property_tree....
File cc/proto/property_tree.proto (right):

https://codereview.chromium.org/1957533002/diff/20001/cc/proto/property_tree....
cc/proto/property_tree.proto:87: // NEXT ID: 15
Will update this after rebase.

[jaydasika, 2016-05-05 23:16:37]

s/has_animated_opacity/has_potential_opacity_animation

[ajuma, 2016-05-06 00:15:36]

https://codereview.chromium.org/1957533002/diff/60001/cc/animation/element_an...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/1957533002/diff/60001/cc/animation/element_an...
cc/animation/element_animations.cc:1127: return true;
It'd be nice to make this logic more similar to what we currently have for
updating whether transform is potentially animating. Maybe add an
UpdateOpacityIsAnimating method that checks whether there's been a change in
opacity currently or potentially animating, and if so notifies clients about
both values? And then (in a separate CL) change
UpdateTransformIsPotentiallyAnimating to UpdateTransformIsAnimating, with the
same behavior of updating both currently and potentially animating?

[jaydasika, 2016-05-06 23:44:41]

Done

[jaydasika, 2016-05-06 23:53:41]

Description was changed from

==========
cc : Track if there are any running opacity animations on effect tree

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

to

==========
cc : Track opacity animation changes on effect tree

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

[jaydasika, 2016-05-07 01:27:01]

Added a test that is similar to the one we have for transforms to
element_animations_unittest.cc.

[ajuma, 2016-05-09 16:56:55]

https://codereview.chromium.org/1957533002/diff/140001/cc/animation/element_a...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:3119: ElementListType::ACTIVE));
Please also test the case where potentially-animating is true but
currently-animating is false (that is, when Animation::InEffect is false for a
running animation).

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1674: if (!node)
When does this happen? (That is, when does a layer that's getting animation
notifications have an invalid property tree index?)

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1689: node->data.has_potential_opacity_animation =
has_potential_animation;
This also needs to take Layer::OpacityCanAnimateOnImplThread into account (like
property_tree_builder does).

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:792: if (!node)
Here too, when does the layer have an invalid property tree index?

https://codereview.chromium.org/1957533002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:9856:
EXPECT_FALSE(node->data.has_potential_opacity_animation);
Please also test the case where currently-animating is false but
potentially-animating is true.

[jaydasika, 2016-05-10 20:22:29]

https://codereview.chromium.org/1957533002/diff/140001/cc/animation/element_a...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:3119: ElementListType::ACTIVE));
On 2016/05/09 16:56:55, ajuma wrote:
> Please also test the case where potentially-animating is true but
> currently-animating is false (that is, when Animation::InEffect is false for a
> running animation).

Done.

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1674: if (!node)
On 2016/05/09 16:56:55, ajuma wrote:
> When does this happen? (That is, when does a layer that's getting animation
> notifications have an invalid property tree index?)

In a lot of tests, almost all LayerTreeHostAnimationTests

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1689: node->data.has_potential_opacity_animation =
has_potential_animation;
On 2016/05/09 16:56:55, ajuma wrote:
> This also needs to take Layer::OpacityCanAnimateOnImplThread into account
(like
> property_tree_builder does).

Done.

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:792: if (!node)
On 2016/05/09 16:56:55, ajuma wrote:
> Here too, when does the layer have an invalid property tree index?

In a couple of tests. I have modified those tests to build property trees first
and removed this.

https://codereview.chromium.org/1957533002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:9856:
EXPECT_FALSE(node->data.has_potential_opacity_animation);
On 2016/05/09 16:56:55, ajuma wrote:
> Please also test the case where currently-animating is false but
> potentially-animating is true.

Done.

[ajuma, 2016-05-10 20:46:20]

Thanks, lgtm

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1957533002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1674: if (!node)
On 2016/05/10 20:22:29, jaydasika wrote:
> On 2016/05/09 16:56:55, ajuma wrote:
> > When does this happen? (That is, when does a layer that's getting animation
> > notifications have an invalid property tree index?)
> 
> In a lot of tests, almost all LayerTreeHostAnimationTests

Ah, this is on Layer, so it might be that we're just starting an animation but
haven't built property trees yet.

https://codereview.chromium.org/1957533002/diff/180001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/1957533002/diff/180001/cc/animation/element_a...
cc/animation/element_animations.cc:1471: element_id(), list_type, change_type,
is_animating);
Nit: Use curly braces here (multi-line 'if' statement).

[jaydasika, 2016-05-10 21:26:26]

https://codereview.chromium.org/1957533002/diff/180001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/1957533002/diff/180001/cc/animation/element_a...
cc/animation/element_animations.cc:1471: element_id(), list_type, change_type,
is_animating);
On 2016/05/10 20:46:20, ajuma wrote:
> Nit: Use curly braces here (multi-line 'if' statement).

Done.

[jaydasika, 2016-05-10 22:26:26]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-05-10 22:26:26]

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/1957533002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-05-10 22:26:56]

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

[commit-bot: I haz the power, 2016-05-10 22:37:18]

Description was changed from

==========
cc : Track opacity animation changes on effect tree

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

to

==========
cc : Track opacity animation changes on effect tree

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

[commit-bot: I haz the power, 2016-05-10 22:37:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-10 22:38:41]

Description was changed from

==========
cc : Track opacity animation changes on effect tree

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

to

==========
cc : Track opacity animation changes on effect tree

BUG=609205
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9cb21c771b787047cf24b129859b8d6636a0c61d
Cr-Commit-Position: refs/heads/master@{#392751}
==========

[commit-bot: I haz the power, 2016-05-10 22:38:42]

Patchset 11 (id:??) landed as
https://crrev.com/9cb21c771b787047cf24b129859b8d6636a0c61d
Cr-Commit-Position: refs/heads/master@{#392751}