LayerTreeHost::SetAnimationEvents should use AnimationRegistrar

LayerTreeHost::SetAnimationEvents should use AnimationRegistrar

This makes LayerTreeHost::SetAnimationEvents iterate over all active
animation controllers registered with its AnimationRegistrar instead
of iterating over the layer tree. This allows us to properly deliver
animation events to a layer that has been temporarily removed from
the layer tree when SetAnimationEvents is called.

BUG=196284
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192835

[Ian Vollick, 2013-04-04 18:38:58]

I really like this. Just a few minor comments.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller.cc (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:138: set_force_sync();
other_controller->UpdateActivation(NormalActivation)?

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:369: if (animation->is_impl_only()) {
nit: no braces.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:546: (events ||
active_animations_[i]->received_finished_event()))) {
Please put (events || active_animations_[i]->received_finished_event()) in a
temporary and add a comment to the temporary explaining why the rules are
different on each thread.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:553:
!active_animations_[j]->received_finished_event()))) {
Same thing here (these complicated conditions are quite hard to follow).

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:685: bool
LayerAnimationController::HasActiveObserver() {
HasActiveValueObserver

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller.h (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.h:55: // Transfers ownership of all
animations to other_controller, replacing
I think it would really help if you explained how this is different from
ReplaceImplThreadAnimations. In particular, that this is for transferring
animations between main-thread controllers, so no monkeying with run states is
required.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller_unittest.cc:230: }
Great test!

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller_unittest.cc:246: // Ensure both
animations have been transfered.
Can you also confirm that the run states are identical? (To ensure that this
behaves differently than ReplaceImplThreadAnimations).

[ajuma, 2013-04-05 14:38:18]

PTAL.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller.cc (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:138: set_force_sync();
On 2013/04/04 18:38:58, vollick wrote:
> other_controller->UpdateActivation(NormalActivation)?

Done.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:369: if (animation->is_impl_only()) {
On 2013/04/04 18:38:58, vollick wrote:
> nit: no braces.

Since the 'else' requires braces, so does the 'if' (according to the style
guide).

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:546: (events ||
active_animations_[i]->received_finished_event()))) {
On 2013/04/04 18:38:58, vollick wrote:
> Please put (events || active_animations_[i]->received_finished_event()) in a
> temporary and add a comment to the temporary explaining why the rules are
> different on each thread.

Done.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:553:
!active_animations_[j]->received_finished_event()))) {
On 2013/04/04 18:38:58, vollick wrote:
> Same thing here (these complicated conditions are quite hard to follow).

Done.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.cc:685: bool
LayerAnimationController::HasActiveObserver() {
On 2013/04/04 18:38:58, vollick wrote:
> HasActiveValueObserver

Done.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller.h (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller.h:55: // Transfers ownership of all
animations to other_controller, replacing
On 2013/04/04 18:38:58, vollick wrote:
> I think it would really help if you explained how this is different from
> ReplaceImplThreadAnimations. In particular, that this is for transferring
> animations between main-thread controllers, so no monkeying with run states is
> required.

Done.

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
cc/animation/layer_animation_controller_unittest.cc:246: // Ensure both
animations have been transfered.
On 2013/04/04 18:38:58, vollick wrote:
> Can you also confirm that the run states are identical? (To ensure that this
> behaves differently than ReplaceImplThreadAnimations).

Done.

[Ian Vollick, 2013-04-05 15:07:11]

lgtm

On 2013/04/05 14:38:18, ajuma wrote:
> PTAL.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> File cc/animation/layer_animation_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.cc:138: set_force_sync();
> On 2013/04/04 18:38:58, vollick wrote:
> > other_controller->UpdateActivation(NormalActivation)?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.cc:369: if (animation->is_impl_only())
{
> On 2013/04/04 18:38:58, vollick wrote:
> > nit: no braces.
> 
> Since the 'else' requires braces, so does the 'if' (according to the style
> guide).
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.cc:546: (events ||
> active_animations_[i]->received_finished_event()))) {
> On 2013/04/04 18:38:58, vollick wrote:
> > Please put (events || active_animations_[i]->received_finished_event()) in a
> > temporary and add a comment to the temporary explaining why the rules are
> > different on each thread.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.cc:553:
> !active_animations_[j]->received_finished_event()))) {
> On 2013/04/04 18:38:58, vollick wrote:
> > Same thing here (these complicated conditions are quite hard to follow).
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.cc:685: bool
> LayerAnimationController::HasActiveObserver() {
> On 2013/04/04 18:38:58, vollick wrote:
> > HasActiveValueObserver
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> File cc/animation/layer_animation_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller.h:55: // Transfers ownership of all
> animations to other_controller, replacing
> On 2013/04/04 18:38:58, vollick wrote:
> > I think it would really help if you explained how this is different from
> > ReplaceImplThreadAnimations. In particular, that this is for transferring
> > animations between main-thread controllers, so no monkeying with run states
is
> > required.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> File cc/animation/layer_animation_controller_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/13465014/diff/10001/cc/animation/layer...
> cc/animation/layer_animation_controller_unittest.cc:246: // Ensure both
> animations have been transfered.
> On 2013/04/04 18:38:58, vollick wrote:
> > Can you also confirm that the run states are identical? (To ensure that this
> > behaves differently than ReplaceImplThreadAnimations).
> 
> Done.

[commit-bot: I haz the power, 2013-04-05 15:07:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/13465014/17013

[commit-bot: I haz the power, 2013-04-05 15:07:36]

Presubmit check for 13465014-17013 failed and returned exit status 1.

INFO:root:Found 12 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    webkit/compositor_bindings/web_layer_impl.cc

Presubmit checks took 1.4s to calculate.

[ajuma, 2013-04-05 15:08:36]

On 2013/04/05 15:07:36, I haz the power (commit-bot) wrote:
> Presubmit check for 13465014-17013 failed and returned exit status 1.
> 
> INFO:root:Found 12 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     webkit/compositor_bindings/web_layer_impl.cc
> 
> Presubmit checks took 1.4s to calculate.

+enne for webkit/compositor_bindings OWNERS.

[enne (OOO), 2013-04-05 15:27:53]

webkit/compositor_bindings lgtm

[commit-bot: I haz the power, 2013-04-06 18:18:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/13465014/29001

[commit-bot: I haz the power, 2013-04-06 23:38:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-07 00:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/13465014/29001

[commit-bot: I haz the power, 2013-04-07 00:16:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 16:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/13465014/29001

[commit-bot: I haz the power, 2013-04-08 18:06:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/13465014/29001

[commit-bot: I haz the power, 2013-04-08 18:08:37]

Change committed as 192835