cc: Push Animation Finished State and Use Finished State for IsCompleted

cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are four parts that are changed by this CL:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

3. It used to be that when animation no longer affects either pending
and active tree, that animation is removed during activation. After this
CL, animation marked as finished on main thread will get to same stage,
but sending out FINISHED event is still needed. To do this, don't
delete animations during activation, instead mark animations not
affecting trees as finished during UpdateState.

4. For animations removed on main thread, the compositor thread cannot
distinguish it from a FINISHED animation on main thread. When main
thread receives FINISHED event from compositor thread and found that
the animation no longer exists, it is safe to delete the animation on
the compositor thread during commit.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2796013003
Cr-Commit-Position: refs/heads/master@{#467008}
Committed: https://chromium.googlesource.com/chromium/src/+/b1e280d1a61f77855418913417c941c7faff176c

[weiliangc, 2017-04-04 22:12:48]

Description was changed from

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are two part that are not working correctly here:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
==========

to

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are two part that are not working correctly here:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[weiliangc, 2017-04-04 22:13:12]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 22:14:01]

Dry run: 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, 2017-04-04 22:21:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:21:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2017-04-05 14:45:18]

Ah I only ran the AnimationPlayerTests, will fix rest of unittests.

[weiliangc, 2017-04-11 19:53:23]

Description was changed from

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are two part that are not working correctly here:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are four parts that are changed by this CL:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

3. It used to be that when animation no longer affects either pending
and active tree, that animation is removed during activation. After this
CL, animation marked as finished on main thread will get to same stage,
but sending out FINISHED event is still needed. To do this, don't
delete animations during activation, instead mark animations not
affecting trees as finished during UpdateState.

4. For animations removed on main thread, the compositor thread cannot
distinguish it from a FINISHED animation on main thread. When main
thread receives FINISHED event from compositor thread and found that
the animation no longer exists, it is safe to delete the animation on
the compositor thread during commit.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[weiliangc, 2017-04-11 19:55:09]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 19:55:42]

Dry run: 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, 2017-04-11 21:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 21:04:54]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-04-11 23:32:02]

lgtm

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

https://codereview.chromium.org/2796013003/diff/20001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:2699: // Finshed animations are
pushed, but animations_impl hasn't yet ticked
'Finished'

https://codereview.chromium.org/2796013003/diff/20001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:3350: // Activation should cause the
animation to be deleted.
This comment needs to be updated.

https://codereview.chromium.org/2796013003/diff/20001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:3352:
player_impl_->GetAnimationById(animation_id)->run_state());
Please also add one more PushProperties() and after that a check that
player_impl_->has_any_animation() is false.

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

https://codereview.chromium.org/2796013003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1468: // Check that eventurally
the animation is removed.
'eventually'

[weiliangc, 2017-04-24 19:47:11]

https://codereview.chromium.org/2796013003/diff/40001/cc/animation/element_an...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2796013003/diff/40001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:3358:
player_->animation_host()->SetAnimationEvents(std::move(events));
Just heads up that since we need the finished event pushed to main thread, I
have to manually call into animation host here in the unittest.

[weiliangc, 2017-04-24 20:58:46]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 20:59:26]

Dry run: 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, 2017-04-24 23:48:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 23:48:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[weiliangc, 2017-04-25 14:52:29]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2017-04-25 14:52:29]

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/2796013003/#ps80001
(title: "clear events before update state in unittest")

[commit-bot: I haz the power, 2017-04-25 14:52:51]

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, 2017-04-25 16:19:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493131949160770,
"parent_rev": "05b481280cf7cedb90c986f4b46aaaf87e781eca", "commit_rev":
"b1e280d1a61f77855418913417c941c7faff176c"}

[commit-bot: I haz the power, 2017-04-25 16:19:55]

Description was changed from

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are four parts that are changed by this CL:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

3. It used to be that when animation no longer affects either pending
and active tree, that animation is removed during activation. After this
CL, animation marked as finished on main thread will get to same stage,
but sending out FINISHED event is still needed. To do this, don't
delete animations during activation, instead mark animations not
affecting trees as finished during UpdateState.

4. For animations removed on main thread, the compositor thread cannot
distinguish it from a FINISHED animation on main thread. When main
thread receives FINISHED event from compositor thread and found that
the animation no longer exists, it is safe to delete the animation on
the compositor thread during commit.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are four parts that are changed by this CL:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

3. It used to be that when animation no longer affects either pending
and active tree, that animation is removed during activation. After this
CL, animation marked as finished on main thread will get to same stage,
but sending out FINISHED event is still needed. To do this, don't
delete animations during activation, instead mark animations not
affecting trees as finished during UpdateState.

4. For animations removed on main thread, the compositor thread cannot
distinguish it from a FINISHED animation on main thread. When main
thread receives FINISHED event from compositor thread and found that
the animation no longer exists, it is safe to delete the animation on
the compositor thread during commit.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2796013003
Cr-Commit-Position: refs/heads/master@{#467008}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1e280d1a61f77855418913417c9...
==========

[commit-bot: I haz the power, 2017-04-25 16:20:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b1e280d1a61f77855418913417c9...