cc: Fix property tree animation sync bug

cc: Fix property tree animation sync bug

Property trees animation sync at commit depends on the value of
is_currently_animating value of a property tree node on the main thread.
So, we need to do this sync before calling AnimationHost::PushProperties
as AnimationHost::PushProperties can delete animations and change the
is_currently_animating value.

BUG=620978
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/94bcfedec292cc72e7b98cea38da392fbf1dc18a
Cr-Commit-Position: refs/heads/master@{#402011}

[jaydasika, 2016-06-22 22:45:43]

Description was changed from

==========
cc: Fix property tree animation sync bug

Property trees animation sync at commit depends on the value of
is_currently_animating value of a property tree node on the main thread.
So, we need to do this sync before calling AnimationHost::PushProperties
as AnimationHost::PushProperties can delete animations and change the
is_currently_animating value.

BUG=620978
==========

to

==========
cc: Fix property tree animation sync bug

Property trees animation sync at commit depends on the value of
is_currently_animating value of a property tree node on the main thread.
So, we need to do this sync before calling AnimationHost::PushProperties
as AnimationHost::PushProperties can delete animations and change the
is_currently_animating value.

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

[jaydasika, 2016-06-22 22:49:57]

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

[jaydasika, 2016-06-22 22:49:57]

This fixes the bug but I am having a hard time trying to write a unit test for
this case : is_currently_animating = true during Layer::PushPropertiesTo and it
becomes false during AnimationHost::PushProperties.

[jaydasika, 2016-06-24 20:46:37]

Added a test

[jaydasika, 2016-06-24 20:47:43]

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1694:
animation->InEffect(base::TimeTicks::Now());
Will remove this.

[ajuma, 2016-06-24 21:02:34]

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1658:
AddAnimatedTransformToPlayer(player_.get(), 1.0, 5, 5);
If you use the version of AddAnimatedTransformToPlayer that takes
start_operations and end_operations, you can set them both to the same
transform, and then you can check for equality to that transform instead of
checking for inequality in CommitCompleteOnThread.

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1681: void UpdateLayerTreeHost()
override {
How about doing this in WillBeginMainFrame? Then the animation will get ticked
(I think) at its starting point on the main thread, so you won't need to set a
rotation below in order to change the main thread transform.

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1704:
MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestChangeAnimationPlayer);
Can this be a single thread test too? (using sync_tree instead of pending_tree
in CommitCompleteOnThread)

[jaydasika, 2016-06-24 21:54:44]

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1658:
AddAnimatedTransformToPlayer(player_.get(), 1.0, 5, 5);
On 2016/06/24 21:02:34, ajuma wrote:
> If you use the version of AddAnimatedTransformToPlayer that takes
> start_operations and end_operations, you can set them both to the same
> transform, and then you can check for equality to that transform instead of
> checking for inequality in CommitCompleteOnThread.

Done.

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1681: void UpdateLayerTreeHost()
override {
On 2016/06/24 21:02:34, ajuma wrote:
> How about doing this in WillBeginMainFrame? Then the animation will get ticked
> (I think) at its starting point on the main thread, so you won't need to set a
> rotation below in order to change the main thread transform.

Done.

https://codereview.chromium.org/2090793002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1704:
MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestChangeAnimationPlayer);
On 2016/06/24 21:02:34, ajuma wrote:
> Can this be a single thread test too? (using sync_tree instead of pending_tree
> in CommitCompleteOnThread)

Done.

[jaydasika, 2016-06-24 22:30:34]

Use AddAnimatedTransformToPlayer that takes in TransforOpaerations instead of
delta.

[ajuma, 2016-06-24 22:32:48]

Thanks, lgtm

[jaydasika, 2016-06-24 22:34:53]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-06-24 22:35:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 23:29:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-24 23:32:50]

Description was changed from

==========
cc: Fix property tree animation sync bug

Property trees animation sync at commit depends on the value of
is_currently_animating value of a property tree node on the main thread.
So, we need to do this sync before calling AnimationHost::PushProperties
as AnimationHost::PushProperties can delete animations and change the
is_currently_animating value.

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

to

==========
cc: Fix property tree animation sync bug

Property trees animation sync at commit depends on the value of
is_currently_animating value of a property tree node on the main thread.
So, we need to do this sync before calling AnimationHost::PushProperties
as AnimationHost::PushProperties can delete animations and change the
is_currently_animating value.

BUG=620978
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/94bcfedec292cc72e7b98cea38da392fbf1dc18a
Cr-Commit-Position: refs/heads/master@{#402011}
==========

[commit-bot: I haz the power, 2016-06-24 23:32:51]

Patchset 5 (id:??) landed as
https://crrev.com/94bcfedec292cc72e7b98cea38da392fbf1dc18a
Cr-Commit-Position: refs/heads/master@{#402011}