cc : Track transform animation changes on transform tree

cc : Track transform animation changes on transform tree

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6c3404e93ce4c3817c33d064f6f9cdca4934c04c
Cr-Commit-Position: refs/heads/master@{#394642}

[jaydasika, 2016-05-18 00:45:18]

Description was changed from

==========
cc : Track transform animation changes on transform tree

BUG=609208
==========

to

==========
cc : Track transform animation changes on transform tree

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

[jaydasika, 2016-05-18 00:57:22]

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

[jaydasika, 2016-05-18 00:57:23]

PTAL

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

https://codereview.chromium.org/1987123002/diff/1/cc/layers/layer.cc#newcode1654
cc/layers/layer.cc:1654: if (node->owner_id == id())
I will make this use the map in a follow up. My plan is to rename
PropertyTrees::ResetFlags(which is no longer used) to something like
PropertyTrees::Property and have a
PropertyTrees::IsInIdToIndexMap(PropertyTrees::Property, id) and use that here
and elsewhere.

[ajuma, 2016-05-18 17:09:12]

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
File cc/animation/element_animations.h (right):

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
cc/animation/element_animations.h:233: void
UpdateAnimationState(TargetProperty::Type property);
This name is confusingly close to the existing UpdateState method. Maybe call it
UpdateClientAnimationState or UpdateAnimatingProperties?

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
cc/animation/element_animations.h:291: bool
potentially_animating_opacity_for_pending_elements_;
Wdyt of creating a struct for this 4-tuple, and then using instances of that
struct for opacity and transform? That might simplify some code, and will also
make it easier to add more target types (e.g. filter animations will need this
too once filters are owned by effect nodes).

[jaydasika, 2016-05-18 21:42:37]

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
File cc/animation/element_animations.h (right):

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
cc/animation/element_animations.h:233: void
UpdateAnimationState(TargetProperty::Type property);
On 2016/05/18 17:09:12, ajuma wrote:
> This name is confusingly close to the existing UpdateState method. Maybe call
it
> UpdateClientAnimationState or UpdateAnimatingProperties?

Done.

https://codereview.chromium.org/1987123002/diff/1/cc/animation/element_animat...
cc/animation/element_animations.h:291: bool
potentially_animating_opacity_for_pending_elements_;
On 2016/05/18 17:09:12, ajuma wrote:
> Wdyt of creating a struct for this 4-tuple, and then using instances of that
> struct for opacity and transform? That might simplify some code, and will also
> make it easier to add more target types (e.g. filter animations will need this
> too once filters are owned by effect nodes).

Done.

[ajuma, 2016-05-18 22:03:30]

Just a few more nits, but then lgtm.

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

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.cc:38: transform_animation_() {}
Are these lines needed (does the default initialization happen without them)?

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.cc:1122: default:
Please add a NOTREACHED here.

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
File cc/animation/element_animations.h (right):

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.h:283: struct AnimationStateForClient {
nit: PropertyAnimationState might be a better name for this

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.h:296: struct AnimationStateForClient
opacity_animation_;
opacity_animation_state_

[jaydasika, 2016-05-19 00:56:42]

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

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.cc:38: transform_animation_() {}
On 2016/05/18 22:03:30, ajuma wrote:
> Are these lines needed (does the default initialization happen without them)?

Removed

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.cc:1122: default:
On 2016/05/18 22:03:30, ajuma wrote:
> Please add a NOTREACHED here.

Done.

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
File cc/animation/element_animations.h (right):

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.h:283: struct AnimationStateForClient {
On 2016/05/18 22:03:30, ajuma wrote:
> nit: PropertyAnimationState might be a better name for this

Done.

https://codereview.chromium.org/1987123002/diff/20001/cc/animation/element_an...
cc/animation/element_animations.h:296: struct AnimationStateForClient
opacity_animation_;
On 2016/05/18 22:03:30, ajuma wrote:
> opacity_animation_state_

Done.

[jaydasika, 2016-05-19 02:11:41]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-05-19 02:11:42]

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/1987123002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-19 02:12:09]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-19 02:42:00]

Description was changed from

==========
cc : Track transform animation changes on transform tree

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

to

==========
cc : Track transform animation changes on transform tree

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6c3404e93ce4c3817c33d064f6f9cdca4934c04c
Cr-Commit-Position: refs/heads/master@{#394642}
==========

[commit-bot: I haz the power, 2016-05-19 02:42:02]

Patchset 4 (id:??) landed as
https://crrev.com/6c3404e93ce4c3817c33d064f6f9cdca4934c04c
Cr-Commit-Position: refs/heads/master@{#394642}