reland -- Disable animations during aura tests.

Disable animations during aura tests.

This causes all animations scheduled during a test to complete immediately.

After making this change, I noticed some code assumed that animations would not complete synchronously. Some of this code used animation observers, and I while fixing the code I have updated it to use the preferred ImplicitAnimationObserver.

BUG=None
TEST=aura_shell_unittests,aura_unittests,compositor_unittests

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

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

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

[sky, 2012-01-24 16:53:15]

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
ash/drag_drop/drag_drop_controller.cc:36: {}
{} on previous line.

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
ash/drag_drop/drag_drop_controller.cc:37: ~DragDropAnimationObserver() {}
virtual

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
File ash/drag_drop/drag_drop_controller.h (right):

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
ash/drag_drop/drag_drop_controller.h:36: class ASH_EXPORT
DragDropAnimationObserverClient {
Move this into its own header.

http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
ash/drag_drop/drag_drop_controller.h:39: protected:
newline between 38/39

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager.cc
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
ash/wm/shelf_layout_manager.cc:31: {}
{} on previous line

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
ash/wm/shelf_layout_manager.cc:32: ~ShelfAnimationObserver() {}
virtual

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
ash/wm/shelf_layout_manager.cc:36: virtual void OnImplicitAnimationsCompleted()
OVERRIDE {
Why do we need a new class for this? Can't we make ShelfLayoutManager extend
this ImplicitAnimationObserver? Same comment for the dnd one.

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager.h
File ash/wm/shelf_layout_manager.h (right):

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
ash/wm/shelf_layout_manager.h:23: class ASH_EXPORT ShelfAnimationObserverClient
{
Move into its own header.

http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
ash/wm/shelf_layout_manager.h:26: protected:
newline between 25/26

http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:120: base::TimeTicks
get_last_step_time_for_test() { return last_step_time_; }
Method name should match field being returned. Also, make it const.

http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:241: static bool
disable_animations_for_test_;
You should add a description to the class that when during tests animations
complete immediately.

[Ian Vollick, 2012-01-24 20:05:18]

On 2012/01/24 16:53:15, sky wrote:
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> File ash/drag_drop/drag_drop_controller.cc (right):
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> ash/drag_drop/drag_drop_controller.cc:36: {}
> {} on previous line.
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> ash/drag_drop/drag_drop_controller.cc:37: ~DragDropAnimationObserver() {}
> virtual
Done.
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> File ash/drag_drop/drag_drop_controller.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> ash/drag_drop/drag_drop_controller.h:36: class ASH_EXPORT
> DragDropAnimationObserverClient {
> Move this into its own header.
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ash/drag_drop/drag_drop_cont...
> ash/drag_drop/drag_drop_controller.h:39: protected:
> newline between 38/39
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager.cc
> File ash/wm/shelf_layout_manager.cc (right):
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
> ash/wm/shelf_layout_manager.cc:31: {}
> {} on previous line
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
> ash/wm/shelf_layout_manager.cc:32: ~ShelfAnimationObserver() {}
> virtual
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
> ash/wm/shelf_layout_manager.cc:36: virtual void
OnImplicitAnimationsCompleted()
> OVERRIDE {
> Why do we need a new class for this? Can't we make ShelfLayoutManager extend
> this ImplicitAnimationObserver? Same comment for the dnd one.
> 
I would have preferred to do this, but the base class,
ImplicitAnimationObserver, is not an interface class (it keeps track of which
implicit animations have yet to complete), so it would break the multiple
inheritance rules in the style guide.

> http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager.h
> File ash/wm/shelf_layout_manager.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
> ash/wm/shelf_layout_manager.h:23: class ASH_EXPORT
ShelfAnimationObserverClient
> {
> Move into its own header.
Done.
> 
>
http://codereview.chromium.org/9222018/diff/5011/ash/wm/shelf_layout_manager....
> ash/wm/shelf_layout_manager.h:26: protected:
> newline between 25/26
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animator.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.h:120: base::TimeTicks
> get_last_step_time_for_test() { return last_step_time_; }
> Method name should match field being returned. Also, make it const.
> 
Done.
>
http://codereview.chromium.org/9222018/diff/5011/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.h:241: static bool
> disable_animations_for_test_;
> You should add a description to the class that when during tests animations
> complete immediately.
Done.

[sky, 2012-01-24 20:53:06]

We're a bit lose on only pure virtual inheritance for cases like this
(WidgetDelegate comes to mind, but there are others), but you're right that
generally we try to avoid multiple inheritance.

LGTM

[Ian Vollick, 2012-01-26 21:33:41]

On 2012/01/24 20:53:06, sky wrote:
> We're a bit lose on only pure virtual inheritance for cases like this
> (WidgetDelegate comes to mind, but there are others), but you're right that
> generally we try to avoid multiple inheritance.
> 
> LGTM

Scott, can you please take another look.

I've had to make some more changes to make sure my use of
ImplicitAnimationObserver is safe here. Now if either the drag drop controller
or the shelf layer manager get destroyed while an animation is in progress, the
observers should be cleaned up correctly.

[sky, 2012-01-26 22:11:19]

http://codereview.chromium.org/9222018/diff/16002/ash/app_list/app_list.cc
File ash/app_list/app_list.cc (right):

http://codereview.chromium.org/9222018/diff/16002/ash/app_list/app_list.cc#ne...
ash/app_list/app_list.cc:125: if (!window)
I don't like these null checks. If we need the app list to be destroyed before
shellwindowid_defaultcontainer we should change ~shell around appropriately.

http://codereview.chromium.org/9222018/diff/16002/ash/drag_drop/drag_drop_con...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/9222018/diff/16002/ash/drag_drop/drag_drop_con...
ash/drag_drop/drag_drop_controller.cc:239: DragDropAnimationObserver* observer =
new DragDropAnimationObserver(this);
Do we really need to support n of these? Can't we create a single instance in
the constructor that is owned by this object? Same comment with the shelf code.

[Ian Vollick, 2012-01-27 16:54:26]

On 2012/01/26 22:11:19, sky wrote:
> http://codereview.chromium.org/9222018/diff/16002/ash/app_list/app_list.cc
> File ash/app_list/app_list.cc (right):
> 
>
http://codereview.chromium.org/9222018/diff/16002/ash/app_list/app_list.cc#ne...
> ash/app_list/app_list.cc:125: if (!window)
> I don't like these null checks. If we need the app list to be destroyed before
> shellwindowid_defaultcontainer we should change ~shell around appropriately.
Done (reverted this file to master).
> 
>
http://codereview.chromium.org/9222018/diff/16002/ash/drag_drop/drag_drop_con...
> File ash/drag_drop/drag_drop_controller.cc (right):
> 
>
http://codereview.chromium.org/9222018/diff/16002/ash/drag_drop/drag_drop_con...
> ash/drag_drop/drag_drop_controller.cc:239: DragDropAnimationObserver* observer
=
> new DragDropAnimationObserver(this);
> Do we really need to support n of these? Can't we create a single instance in
> the constructor that is owned by this object? Same comment with the shelf
code.

No, we don't need to support n. I've replaced the std::sets with scoped_ptrs in
both cases.

[sky, 2012-01-27 17:54:10]

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
File ash/drag_drop/drag_drop_animation_observer_client.h (right):

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
ash/drag_drop/drag_drop_animation_observer_client.h:18: class ASH_EXPORT
DragDropAnimationObserverClient {
I think I sent you on the wrong path here. There's no point in exposing this.
Instead make DragDropAnimationObserver take a DragDropController* and invoke
whatever method you need from there. If you want to make the method
DragDropController invokes private, forward declare DragDropAnimationObserver in
DragDropController's private section.

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
File ash/drag_drop/drag_drop_controller.cc (right):

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
ash/drag_drop/drag_drop_controller.cc:67: if (drag_image_.get() &&
active_observer_.get()) {
Why do you need to explicitly remove the animation observer in the destructors?
With Xiyuan's patch isn't that done for you?

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
File ash/drag_drop/drag_drop_controller.h (right):

https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
ash/drag_drop/drag_drop_controller.h:96: // The live observers. When the
controller is destroyed, these observers are
Update comment to reflect only one.

[Ian Vollick, 2012-01-27 20:28:39]

On 2012/01/27 17:54:10, sky wrote:
>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> File ash/drag_drop/drag_drop_animation_observer_client.h (right):
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> ash/drag_drop/drag_drop_animation_observer_client.h:18: class ASH_EXPORT
> DragDropAnimationObserverClient {
> I think I sent you on the wrong path here. There's no point in exposing this.
> Instead make DragDropAnimationObserver take a DragDropController* and invoke
> whatever method you need from there. If you want to make the method
> DragDropController invokes private, forward declare DragDropAnimationObserver
in
> DragDropController's private section.
> 
Done.
>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> File ash/drag_drop/drag_drop_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> ash/drag_drop/drag_drop_controller.cc:67: if (drag_image_.get() &&
> active_observer_.get()) {
> Why do you need to explicitly remove the animation observer in the
destructors?
> With Xiyuan's patch isn't that done for you?
> 

Yes, that's true as long as the observer is destructed (completely) before the
notifications are sent. I've changed the code so that the observer is destructed
at the beginning of ~DragDropController. In the case of the shelf manager, the
manager does not own the layers, so the observers will get cleaned up
automatically at the right time.

>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> File ash/drag_drop/drag_drop_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/24021/ash/drag_drop/drag_...
> ash/drag_drop/drag_drop_controller.h:96: // The live observers. When the
> controller is destroyed, these observers are
> Update comment to reflect only one.

Done.

[sky, 2012-01-27 20:54:19]

http://codereview.chromium.org/9222018/diff/23005/ash/drag_drop/drag_drop_con...
File ash/drag_drop/drag_drop_controller.h (right):

http://codereview.chromium.org/9222018/diff/23005/ash/drag_drop/drag_drop_con...
ash/drag_drop/drag_drop_controller.h:74: // Overridden from
DragDropAnimationObserverClient:
This is no longer an override, so remove virtual/OVERRIDE and the update the
comment.

http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager.h
File ash/wm/shelf_layout_manager.h (right):

http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.h:85: // ShelfAnimationObserverClient overrides:
Same comment as with the dnd one.

http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.h:102: scoped_ptr<ui::ImplicitAnimationObserver>
launcher_animation_observer_;
Since we always have both animating at the same time, could we have just one
observer for the two layers?

http://codereview.chromium.org/9222018/diff/23005/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_observer.h (right):

http://codereview.chromium.org/9222018/diff/23005/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_observer.h:44: virtual
ImplicitAnimationObserver* AsImplicitObserver() { return NULL; }
Ick. Add a comment why this is needed.

[Ian Vollick, 2012-01-30 14:51:37]

Most noteworthy change, the drag drop controller and shelf layout manager now
inherit from ImplicitAnimationObserver.

On 2012/01/27 20:54:19, sky wrote:
>
http://codereview.chromium.org/9222018/diff/23005/ash/drag_drop/drag_drop_con...
> File ash/drag_drop/drag_drop_controller.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/23005/ash/drag_drop/drag_drop_con...
> ash/drag_drop/drag_drop_controller.h:74: // Overridden from
> DragDropAnimationObserverClient:
> This is no longer an override, so remove virtual/OVERRIDE and the update the
> comment.
Comment has been updated.
> 
>
http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager.h
> File ash/wm/shelf_layout_manager.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager...
> ash/wm/shelf_layout_manager.h:85: // ShelfAnimationObserverClient overrides:
> Same comment as with the dnd one.
Comment has been updated.
> 
>
http://codereview.chromium.org/9222018/diff/23005/ash/wm/shelf_layout_manager...
> ash/wm/shelf_layout_manager.h:102: scoped_ptr<ui::ImplicitAnimationObserver>
> launcher_animation_observer_;
> Since we always have both animating at the same time, could we have just one
> observer for the two layers?
Yes. There is a bit of a tricky issue with activation -- we require both scoped
settings objects to remain in scope until all of the implicit animations have
been scheduled. This required getting rid of AnimateWidgetTo, but this didn't
result in too much code duplication.
> 
>
http://codereview.chromium.org/9222018/diff/23005/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animation_observer.h (right):
> 
>
http://codereview.chromium.org/9222018/diff/23005/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_observer.h:44: virtual
> ImplicitAnimationObserver* AsImplicitObserver() { return NULL; }
> Ick. Add a comment why this is needed.

It's not. As we discussed, I've changed the code so that scoped settings objects
now only take implicit observers, so there's no need for this. I've also changed
all code that added non-implicit observers.

[sky, 2012-01-30 15:58:55]

https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
File ui/gfx/compositor/layer_animation_observer.h (right):

https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
ui/gfx/compositor/layer_animation_observer.h:49: virtual void
OnAttachedToSequence(LayerAnimationSequence* sequence);
Where is this invoked from?

https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
ui/gfx/compositor/layer_animation_observer.h:101: std::set<const
LayerAnimationSequence*> unfinished_animations_;
Is this the same as attached_sequences_?

[Ian Vollick, 2012-01-30 17:46:53]

On 2012/01/30 15:58:55, sky wrote:
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
> File ui/gfx/compositor/layer_animation_observer.h (right):
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
> ui/gfx/compositor/layer_animation_observer.h:49: virtual void
> OnAttachedToSequence(LayerAnimationSequence* sequence);
> Where is this invoked from?
Thanks for catching this -- it was meant to be called from
LayerAnimationObserver::AttachedToSequence. I've added a couple of unit tests to
cover the cases this code is meant to handle (attaching and detaching from an
in-flight animation).
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
> ui/gfx/compositor/layer_animation_observer.h:101: std::set<const
> LayerAnimationSequence*> unfinished_animations_;
> Is this the same as attached_sequences_?
Good question. They are not presently the same. For example, you could continue
to observe a sequence after it completes the first time (sequences can be
reused). Although you would still be attached to this sequence, it would no
longer belong to the collection of sequences the implicit observer is waiting to
complete. But perhaps this a bug. Maybe the implicit observer should stop
observing a sequence when it completes. What do you think?

[sky, 2012-01-30 18:20:28]

On Mon, Jan 30, 2012 at 9:46 AM,  <vollick@chromium.org> wrote:
> On 2012/01/30 15:58:55, sky wrote:
>
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
>>
>> File ui/gfx/compositor/layer_animation_observer.h (right):
>
>
>
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
>>
>> ui/gfx/compositor/layer_animation_observer.h:49: virtual void
>> OnAttachedToSequence(LayerAnimationSequence* sequence);
>> Where is this invoked from?
>
> Thanks for catching this -- it was meant to be called from
> LayerAnimationObserver::AttachedToSequence. I've added a couple of unit
> tests to
> cover the cases this code is meant to handle (attaching and detaching from
> an
> in-flight animation).
>
>
>
>
https://chromiumcodereview.appspot.com/9222018/diff/42002/ui/gfx/compositor/l...
>>
>> ui/gfx/compositor/layer_animation_observer.h:101: std::set<const
>> LayerAnimationSequence*> unfinished_animations_;
>> Is this the same as attached_sequences_?
>
> Good question. They are not presently the same. For example, you could
> continue
> to observe a sequence after it completes the first time (sequences can be
> reused). Although you would still be attached to this sequence, it would no
> longer belong to the collection of sequences the implicit observer is
> waiting to
> complete. But perhaps this a bug. Maybe the implicit observer should stop
> observing a sequence when it completes. What do you think?

I tend to think folks that are using the implicit observer are going
for the easy API approach and unlikely to do anything sophisticated.
As creating and/or managing sequences is not the easy API approach I
tend to think it should stop observing a sequence when it completes.

  -Scott

[Ian Vollick, 2012-01-30 20:25:28]

> >> ui/gfx/compositor/layer_animation_observer.h:101: std::set<const
> >> LayerAnimationSequence*> unfinished_animations_;
> >> Is this the same as attached_sequences_?
> >
> > Good question. They are not presently the same. For example, you could
> > continue
> > to observe a sequence after it completes the first time (sequences can be
> > reused). Although you would still be attached to this sequence, it would no
> > longer belong to the collection of sequences the implicit observer is
> > waiting to
> > complete. But perhaps this a bug. Maybe the implicit observer should stop
> > observing a sequence when it completes. What do you think?
> 
> I tend to think folks that are using the implicit observer are going
> for the easy API approach and unlikely to do anything sophisticated.
> As creating and/or managing sequences is not the easy API approach I
> tend to think it should stop observing a sequence when it completes.

Done. This required changing the API for LayerAnimationObserver slightly. In
almost every case, it revealed code where people were using
LayerAnimationObserver where ImplicitAnimationObserver would be more
appropriate. I've updated this code. I've also  added
ImplicitAnimationObserver::StopObservingImplicitAnimations for use when you are
about to start observing some new implicit animations and you want to forget
about the old ones.

[sky, 2012-01-30 22:20:19]

https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
File ui/gfx/compositor/layer_animation_observer.h (right):

https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
ui/gfx/compositor/layer_animation_observer.h:56: void DetachFromAllSequences();
Add a description of this. Maybe it should be named similar to the variant
below, StopObserving.

https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
ui/gfx/compositor/layer_animation_observer.h:58: AttachedSequences&
attached_sequences() { return attached_sequences_; }
I don't think you want subclasses modifying this. Can it be:

const AttachedSequences& attached_sequences() const?

https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
ui/gfx/compositor/layer_animation_observer.h:85: void
StopObservingImplicitAnimations();
Can this be protected?

[Ian Vollick, 2012-01-31 13:58:19]

On 2012/01/30 22:20:19, sky wrote:
>
https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
> File ui/gfx/compositor/layer_animation_observer.h (right):
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
> ui/gfx/compositor/layer_animation_observer.h:56: void
DetachFromAllSequences();
> Add a description of this. Maybe it should be named similar to the variant
> below, StopObserving.
Sounds good. Done.
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
> ui/gfx/compositor/layer_animation_observer.h:58: AttachedSequences&
> attached_sequences() { return attached_sequences_; }
> I don't think you want subclasses modifying this. Can it be:
> 
> const AttachedSequences& attached_sequences() const?
Done.
> 
>
https://chromiumcodereview.appspot.com/9222018/diff/42005/ui/gfx/compositor/l...
> ui/gfx/compositor/layer_animation_observer.h:85: void
> StopObservingImplicitAnimations();
> Can this be protected?

Done.

[sky, 2012-01-31 16:54:59]

LGTM

[commit-bot: I haz the power, 2012-02-01 14:20:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/9222018/45001

[commit-bot: I haz the power, 2012-02-01 16:45:45]

Try job failure for 9222018-45001 (retry) on win_rel for step "unit_tests".
It's a second try, previously, steps "unit_tests, browser_tests, ui_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...