compositor: Tick the UI animations from cc, instead of from timer callbacks.

compositor: Tick the UI animations from cc, instead of from timer callbacks.

Update the animations in the UI in response to the animation step in the compositor,
instead of from a timer callback. This should make it more difficult for rogue UI
animations to negatively impact the system too much.

BUG=371071
R=ajuma@chromium.org, sky@chromium.org

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

[sadrul, 2014-05-22 23:48:05]

Hi! Do you mind taking a look at this and see if this looks reasonable?

I am not sure about the call to SetNeedsAnimate() from Compositor::DidCommit().
Should that actually be somewhere else?

I am having difficulty figuring out (or reliably reproducing) the test failure
on linux_chromium_rel (and win) trybots. Do you have any insights? Could this be
related to the call to SetNeedsAnimate()?

[sadrul, 2014-05-23 01:03:21]

On 2014/05/22 23:48:05, sadrul wrote:
> Hi! Do you mind taking a look at this and see if this looks reasonable?
> 
> I am not sure about the call to SetNeedsAnimate() from
Compositor::DidCommit().
> Should that actually be somewhere else?
> 
> I am having difficulty figuring out (or reliably reproducing) the test failure
> on linux_chromium_rel (and win) trybots. Do you have any insights? Could this
be
> related to the call to SetNeedsAnimate()?

Looks like SingleThreadProxy just never calls
LayerTreeHost::UpdateClientAnimations(), and so Compositor::Animate() is never
called at all. Which means the animations never end, causing the failures in the
tests. I added a call to UpdateClientAnimations() in
SingleThreadProxy::CompositeImmediately(), just before AnimateLayers() is called
(since ThreadProxy also calls UpdateClientAnimations() just before calling
AnimateLayers()). Is this the right fix?

[chromium-reviews, 2014-05-23 16:45:14]

On Thu, May 22, 2014 at 9:03 PM, <sadrul@chromium.org> wrote:

> On 2014/05/22 23:48:05, sadrul wrote:
>
>> Hi! Do you mind taking a look at this and see if this looks reasonable?
>>
>
>  I am not sure about the call to SetNeedsAnimate() from
>>
> Compositor::DidCommit().
>
>> Should that actually be somewhere else?
>>
>
>  I am having difficulty figuring out (or reliably reproducing) the test
>> failure
>> on linux_chromium_rel (and win) trybots. Do you have any insights? Could
>> this
>>
> be
>
>> related to the call to SetNeedsAnimate()?
>>
>
> Looks like SingleThreadProxy just never calls
> LayerTreeHost::UpdateClientAnimations(), and so Compositor::Animate() is
> never
> called at all. Which means the animations never end, causing the failures
> in the
> tests. I added a call to UpdateClientAnimations() in
> SingleThreadProxy::CompositeImmediately(), just before AnimateLayers() is
> called
> (since ThreadProxy also calls UpdateClientAnimations() just before calling
> AnimateLayers()). Is this the right fix?


Currently single thread compositing is a very different path than threaded,
it expects the caller of Composite() to do the layout() and Animate() calls
itself.

This will change with https://codereview.chromium.org/134623005/. But for
now, you should be calling the Animate() stuff before calling Composite()
from ui::Compositor::Draw before calling LTH::Composite().

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajuma, 2014-05-23 17:10:00]

This looks pretty good, just a few comments.

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/composito...
ui/compositor/compositor.cc:344: host_->SetNeedsAnimate();
This should be moved to the end of ::Animate above.

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/layer_ani...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/layer_ani...
ui/compositor/layer_animator.cc:744: start_time =
LayerAnimatorCollection::GetInstance()->last_tick_time();
It's possible for LayerAnimatorCollection to have active animators, but to have
an old last_tick_time (e.g. suppose we just added another animator to the
collection, but Progress hasn't been called yet). One fix would be for
LayerAnimatorCollection::StartAnimator to set last_tick_time to
gfx::FrameTime::Now() if the collection was previously empty. (It would also be
good to have a unit test that verifies that
LayerAnimationCollection::last_tick_time gets updated in this situation.)

https://codereview.chromium.org/291843012/diff/110001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller_unittest.cc:48:
layer->GetAnimator()->Step(step_time +
controller.animator()->Step(...)
is more consistent with the previous lines.

[sadrul, 2014-05-23 18:02:42]

> > Looks like SingleThreadProxy just never calls
> > LayerTreeHost::UpdateClientAnimations(), and so Compositor::Animate() is
> > never
> > called at all. Which means the animations never end, causing the failures
> > in the
> > tests. I added a call to UpdateClientAnimations() in
> > SingleThreadProxy::CompositeImmediately(), just before AnimateLayers() is
> > called
> > (since ThreadProxy also calls UpdateClientAnimations() just before calling
> > AnimateLayers()). Is this the right fix?
> 
> 
> Currently single thread compositing is a very different path than threaded,
> it expects the caller of Composite() to do the layout() and Animate() calls
> itself.
> 
> This will change with https://codereview.chromium.org/134623005/. But for
> now, you should be calling the Animate() stuff before calling Composite()
> from ui::Compositor::Draw before calling LTH::Composite().

Ah, thanks! Done (removed the change in SingleThreadProxy, and added a call to
Animate() in Compositor::Draw() between the calls to Layout() and
host_->Composite()

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/composito...
ui/compositor/compositor.cc:344: host_->SetNeedsAnimate();
On 2014/05/23 17:10:00, ajuma wrote:
> This should be moved to the end of ::Animate above.

Done.

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/layer_ani...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/compositor/layer_ani...
ui/compositor/layer_animator.cc:744: start_time =
LayerAnimatorCollection::GetInstance()->last_tick_time();
On 2014/05/23 17:10:00, ajuma wrote:
> It's possible for LayerAnimatorCollection to have active animators, but to
have
> an old last_tick_time (e.g. suppose we just added another animator to the
> collection, but Progress hasn't been called yet). One fix would be for
> LayerAnimatorCollection::StartAnimator to set last_tick_time to
> gfx::FrameTime::Now() if the collection was previously empty. (It would also
be
> good to have a unit test that verifies that
> LayerAnimationCollection::last_tick_time gets updated in this situation.)

Done.

https://codereview.chromium.org/291843012/diff/110001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/291843012/diff/110001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller_unittest.cc:48:
layer->GetAnimator()->Step(step_time +
On 2014/05/23 17:10:00, ajuma wrote:
> controller.animator()->Step(...)
> is more consistent with the previous lines.

Done.

[sadrul, 2014-05-23 18:09:47]

+danakj@ for compositor.cc|h changes.

+sky@ for non-compositor changes (all these changes are in tests, changing the
calls from gfx::AnimationContainerElement::Step() to ui::LayerAnimator::Step())

[ajuma, 2014-05-23 18:12:16]

Thanks, lgtm.

[piman, 2014-05-23 18:20:05]

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:262: Animate(now);
I'm not sure I understand what in the animation code triggers a compositor frame
to make sure we call Animate() the first time.

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:324:
LayerAnimatorCollection::GetInstance()->Progress(frame_begin_time);
We have one compositor per root window, which are on their own schedule.
Doesn't this mean that we'll evaluate all animations from all layer for each
compositor? I.e. N times if you have N root windows?

Couldn't the LayerAnimatorCollection be one per compositor, so that you only
animate each layer once per frame?

[sadrul, 2014-05-23 18:32:27]

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:262: Animate(now);
On 2014/05/23 18:20:06, piman wrote:
> I'm not sure I understand what in the animation code triggers a compositor
frame
> to make sure we call Animate() the first time.

I am not sure if there is anything to ensure this first call. Ali might know
more.

Do you suggest an explicit call to the compositor from the collection?

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:324:
LayerAnimatorCollection::GetInstance()->Progress(frame_begin_time);
On 2014/05/23 18:20:06, piman wrote:
> We have one compositor per root window, which are on their own schedule.
> Doesn't this mean that we'll evaluate all animations from all layer for each
> compositor? I.e. N times if you have N root windows?
> 
> Couldn't the LayerAnimatorCollection be one per compositor, so that you only
> animate each layer once per frame?

Ah, good point. Right now, the LayerAnimator does not know about the compositor.
Should there be a LayerAnimationDelegate::GetCompositor(), so that the animator
can get to the right collection to add itself to/remove itself from?

[ajuma, 2014-05-23 19:15:14]

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:262: Animate(now);
On 2014/05/23 18:32:28, sadrul wrote:
> On 2014/05/23 18:20:06, piman wrote:
> > I'm not sure I understand what in the animation code triggers a compositor
> frame
> > to make sure we call Animate() the first time.
> 
> I am not sure if there is anything to ensure this first call. Ali might know
> more.
> 
> Do you suggest an explicit call to the compositor from the collection?

There doesn't seem to be anything to ensure this. (In the existing code, adding
a LayerAnimatior to an empty AnimationContainer causes the AnimationContainer to
start its timer.) An explicit call to the compositor when adding an animator to
an empty collection seems reasonable (perhaps via a minimal
LayerAnimatorCollection<something> interface rather than exposing all of
Compositor to the collection).

https://codereview.chromium.org/291843012/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:324:
LayerAnimatorCollection::GetInstance()->Progress(frame_begin_time);
On 2014/05/23 18:32:28, sadrul wrote:
> On 2014/05/23 18:20:06, piman wrote:
> > We have one compositor per root window, which are on their own schedule.
> > Doesn't this mean that we'll evaluate all animations from all layer for each
> > compositor? I.e. N times if you have N root windows?
> > 
> > Couldn't the LayerAnimatorCollection be one per compositor, so that you only
> > animate each layer once per frame?
> 
> Ah, good point. Right now, the LayerAnimator does not know about the
compositor.
> Should there be a LayerAnimationDelegate::GetCompositor(), so that the
animator
> can get to the right collection to add itself to/remove itself from?

Maybe LayerAnimationDelegate::GetCollection(), where the implementation in Layer
calls into the compositor to get the collection?

Also, if a layer with an active animator is moved from one compositor to
another, we'll need to make sure to remove the animator from the old
compositor's collection and add it to the new one.

[sadrul, 2014-05-23 21:24:45]

I have made the following changes:
 * Each ui::Compositor has its own LayerAnimatorCollection. The compositor
installs itself as its delegate.
 * A LayerAnimator gets to the Collection through the LayerAnimationDelegate
interface.
 * When a new animator is added to the Collection, the Collection triggers a
ScheduleAnimation() in the Compositor() via the delegate interface.
 * When a ui::Layer is removed or added to the layer tree, it removes/adds the
animator to the collection if it's animating.

PTAL.

[piman, 2014-05-23 21:38:56]

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/composito...
ui/compositor/compositor.cc:264: Animate(now);
nit: in threaded mode, the compositor calls Animate before Layout (I think it
makes sense). Should we do the same for consistency?

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:162: if (child->GetAnimator() &&
child->GetAnimator()->is_animating()) {
nit: GetAnimator() is non-trivial:
1- can we call it once and keep the return value?
2- it also has a side effect of creating the animator. Doesn't that mean we'll
create a LayerAnimator for every layer, even though it may never be animated?

Actually, GetAnimator() says that it never returns NULL.

[sadrul, 2014-05-23 21:56:10]

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/composito...
ui/compositor/compositor.cc:264: Animate(now);
On 2014/05/23 21:38:56, piman wrote:
> nit: in threaded mode, the compositor calls Animate before Layout (I think it
> makes sense). Should we do the same for consistency?

Done.

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/291843012/diff/150001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:162: if (child->GetAnimator() &&
child->GetAnimator()->is_animating()) {
On 2014/05/23 21:38:56, piman wrote:
> nit: GetAnimator() is non-trivial:
> 1- can we call it once and keep the return value?
> 2- it also has a side effect of creating the animator. Doesn't that mean we'll
> create a LayerAnimator for every layer, even though it may never be animated?
> 
> Actually, GetAnimator() says that it never returns NULL.

Done. Added Layer::IsAnimating() const { return animator_ &&
animator_->is_animating(); } and using that instead (here, and also in
Layer::Remove()).

[piman, 2014-05-23 22:06:27]

lgtm

[sky, 2014-05-23 22:53:37]

test changes LGTM

[ajuma, 2014-05-26 14:43:22]

lgtm with a couple nits

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/composito...
ui/compositor/compositor.cc:397: ScheduleAnimation();
host_->SetNeedsAnimate()

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/layer_ani...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/layer_ani...
ui/compositor/layer_animator_collection.cc:52: last_tick_time_ = now;
This should be moved to the start of this method, so that calls to
last_tick_time() during (*iter)->Step don't get a stale value.

[sadrul, 2014-05-28 21:41:22]

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/composito...
ui/compositor/compositor.cc:397: ScheduleAnimation();
On 2014/05/26 14:43:23, ajuma wrote:
> host_->SetNeedsAnimate()

Done (I assume you meant 'instead of' ScheduleAnimation() as opposed to 'in
addition to')

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/layer_ani...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/291843012/diff/260001/ui/compositor/layer_ani...
ui/compositor/layer_animator_collection.cc:52: last_tick_time_ = now;
On 2014/05/26 14:43:23, ajuma wrote:
> This should be moved to the start of this method, so that calls to
> last_tick_time() during (*iter)->Step don't get a stale value.

Done.

[sadrul, 2014-06-01 23:05:54]

On 2014/05/23 19:15:14, ajuma wrote:
... [snip]
> 
> Also, if a layer with an active animator is moved from one compositor to
> another, we'll need to make sure to remove the animator from the old
> compositor's collection and add it to the new one.

I made some changes to make sure the animators in the subtree are added/removed
correctly form the collection. Mind taking another look? (Patchset13 is the last
reviewed CL)

[ajuma, 2014-06-02 14:53:21]

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:175:
child->RemoveAnimatorsInTreeFromCollection(collection);
This should be moved to after the StopAnimatingProperty call below (which might
otherwise cause us to get re-added to the collection, since it could lead to
other animations starting).

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:952: collection->StartAnimator(animator_.get());
LayerAnimator::is_started_ will have a stale value if we directly Start/Stop
from Layer. How about calling into LayerAnimator and asking it to Add/Remove
itself from the collection? (As long as we Remove before disconnecting the layer
from the tree, and Add after re-adding the layer to a tree, I think the
collection that LayerAnimator gets from its delegate should be the same as the
one we'd use here.)

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer_ani...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer_ani...
ui/compositor/layer_animator_collection.cc:35: if (animators_.count(animator) >
0U)
If LayerAnimator::is_started_'s value is updated whenever we Start/Stop (and
checked before we call StopAnimator), I think this can go back to being a DCHECK
as it was in patch set 16 (and we should have an analogous DCHECK in
::StartAnimator).

[sadrul, 2014-06-02 17:53:43]

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:175:
child->RemoveAnimatorsInTreeFromCollection(collection);
On 2014/06/02 14:53:22, ajuma wrote:
> This should be moved to after the StopAnimatingProperty call below (which
might
> otherwise cause us to get re-added to the collection, since it could lead to
> other animations starting).

Done.

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:952: collection->StartAnimator(animator_.get());
On 2014/06/02 14:53:22, ajuma wrote:
> LayerAnimator::is_started_ will have a stale value if we directly Start/Stop
> from Layer. How about calling into LayerAnimator and asking it to Add/Remove
> itself from the collection? (As long as we Remove before disconnecting the
layer
> from the tree, and Add after re-adding the layer to a tree, I think the
> collection that LayerAnimator gets from its delegate should be the same as the
> one we'd use here.)

Done.

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer_ani...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/291843012/diff/500001/ui/compositor/layer_ani...
ui/compositor/layer_animator_collection.cc:35: if (animators_.count(animator) >
0U)
On 2014/06/02 14:53:22, ajuma wrote:
> If LayerAnimator::is_started_'s value is updated whenever we Start/Stop (and
> checked before we call StopAnimator), I think this can go back to being a
DCHECK
> as it was in patch set 16 (and we should have an analogous DCHECK in
> ::StartAnimator).

Done.

[ajuma, 2014-06-02 17:57:28]

Thanks, latest changes lgtm.

[sadrul, 2014-06-03 02:02:35]

Committed in r274404 (git-cl crashed before it could update the CL).

[sadrul, 2014-06-04 05:23:32]

I made some changes to fix the memory leak issues that caused the CL to be
reverted.

The change: when an aura::Window is destroyed, force-complete all active
animations of the layers in the layer-tree rooted at the Window's layer.

Patchset 24 is the previously landed CL. Patchset 27 is the CL with the fix
(relevant changes are in aura/window.cc, compositor/layer.cc, and
compositor/layer.h). Mind taking another look at this?

[ajuma, 2014-06-04 16:40:11]

Could you explain a bit more about the leaks you were running into?

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
I think we have cases where animations are expected to outlive the destruction
of the Window (the animation from http://crbug.com/341682 comes to mind as an
example). AIUI, in these cases the ownership of the animating layer is
transferred to an observer (like HidingWindowAnimationObserver) that destroys
the layer when the animation is completed.

[ajuma, 2014-06-04 17:13:11]

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
On 2014/06/04 16:40:12, ajuma wrote:

> http://crbug.com/341682

Sorry, that should be http://crbug.com/341862.

[sadrul, 2014-06-04 17:39:12]

On 2014/06/04 16:40:11, ajuma wrote:
> Could you explain a bit more about the leaks you were running into?
> 

A sample log from a leak:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

Another from an earlier try:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b...

The leaks show up from this CL because currently, all tests call a
RunAllPendingInMessageLoop() at the end of each test, which completes the posted
tasks, causing the timers to finish, and the animations to complete. I used a
test-controller and/or a DrawWaiterForTest to explicitly stop the animations to
avoid the leaks (https://codereview.chromium.org/307323002/), but as sky@ noted,
it is very easy to miss, and not very obvious.

Destroying a Window destroys its Layer. This causes all of its child layers to
get detached from the layer tree. So I think it makes sense to immediately
complete the animations for these layers too.

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
On 2014/06/04 17:13:12, ajuma wrote:
> On 2014/06/04 16:40:12, ajuma wrote:
> 
> > http://crbug.com/341682
> 
> Sorry, that should be http://crbug.com/341862.

For animating a window closing, we usually clone the layers first, and animate
them. The cloned layers becomes siblings to the original layers. So
destroying/stopping animations in the original layers do not affect the
animating cloned layers.

[ajuma, 2014-06-04 19:14:41]

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
On 2014/06/04 17:39:13, sadrul wrote:
> On 2014/06/04 17:13:12, ajuma wrote:
> > On 2014/06/04 16:40:12, ajuma wrote:
> > 
> > > http://crbug.com/341682
> > 
> > Sorry, that should be http://crbug.com/341862.
> 
> For animating a window closing, we usually clone the layers first, and animate
> them. The cloned layers becomes siblings to the original layers. So
> destroying/stopping animations in the original layers do not affect the
> animating cloned layers.

Ah, things may have changed since the last time I was looking at the layer
cloning code (the cloning used to happen during OnWindowDestroying below, and
animations in progress before cloning were expected to continue). I'll defer to
sky@.

[sky, 2014-06-04 22:43:42]

On 2014/06/04 17:39:12, sadrul wrote:
> On 2014/06/04 16:40:11, ajuma wrote:
> > Could you explain a bit more about the leaks you were running into?
> > 
> 
> A sample log from a leak:
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> 
> Another from an earlier try:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b...
> 
> The leaks show up from this CL because currently, all tests call a
> RunAllPendingInMessageLoop() at the end of each test, which completes the
posted
> tasks, causing the timers to finish, and the animations to complete. I used a
> test-controller and/or a DrawWaiterForTest to explicitly stop the animations
to
> avoid the leaks (https://codereview.chromium.org/307323002/), but as sky@
noted,
> it is very easy to miss, and not very obvious.
> 
> Destroying a Window destroys its Layer. This causes all of its child layers to
> get detached from the layer tree. So I think it makes sense to immediately
> complete the animations for these layers too.

Sounds reasonable to me. In fact we made removing a layer stop bounds animations
too. See Layer::Remove .

> https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
>
https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
> ui/aura/window.cc:221: layer()->CompleteAllAnimations();
> On 2014/06/04 17:13:12, ajuma wrote:
> > On 2014/06/04 16:40:12, ajuma wrote:
> > 
> > > http://crbug.com/341682
> > 
> > Sorry, that should be http://crbug.com/341862.
> 
> For animating a window closing, we usually clone the layers first, and animate
> them. The cloned layers becomes siblings to the original layers. So
> destroying/stopping animations in the original layers do not affect the
> animating cloned layers.

[sadrul, 2014-06-04 23:00:53]

On 2014/06/04 22:43:42, sky wrote:
> On 2014/06/04 17:39:12, sadrul wrote:
> > On 2014/06/04 16:40:11, ajuma wrote:
> > > Could you explain a bit more about the leaks you were running into?
> > > 
> > 
> > A sample log from a leak:
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > 
> > Another from an earlier try:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b...
> > 
> > The leaks show up from this CL because currently, all tests call a
> > RunAllPendingInMessageLoop() at the end of each test, which completes the
> posted
> > tasks, causing the timers to finish, and the animations to complete. I used
a
> > test-controller and/or a DrawWaiterForTest to explicitly stop the animations
> to
> > avoid the leaks (https://codereview.chromium.org/307323002/), but as sky@
> noted,
> > it is very easy to miss, and not very obvious.
> > 
> > Destroying a Window destroys its Layer. This causes all of its child layers
to
> > get detached from the layer tree. So I think it makes sense to immediately
> > complete the animations for these layers too.
> 
> Sounds reasonable to me. In fact we made removing a layer stop bounds
animations
> too. See Layer::Remove .

Cool. Thanks! Should I go ahead with attempting to land again?

[sky, 2014-06-04 23:32:43]

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
I think you only want to do this if layer()->owner() == this. Otherwise someone
else owns the layer and presumably the animation should continue. Also, it would
be nice to have test coverage that animations are stopped.

[sadrul, 2014-06-05 01:03:36]

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/291843012/diff/680001/ui/aura/window.cc#newco...
ui/aura/window.cc:221: layer()->CompleteAllAnimations();
On 2014/06/04 23:32:45, sky wrote:
> I think you only want to do this if layer()->owner() == this. Otherwise
someone
> else owns the layer and presumably the animation should continue.

Done.

> Also, it would
> be nice to have test coverage that animations are stopped.

Done.

[sky, 2014-06-05 01:40:48]

LGTM

[sadrul, 2014-06-05 03:11:53]

Committed patchset #28 manually as r274988 (presubmit successful).

[darin (slow to review), 2014-06-05 05:56:21]

On 2014/06/05 03:11:53, sadrul wrote:
> Committed patchset #28 manually as r274988 (presubmit successful).

It looks like this may have broken:
LayerWithNullDelegateTest.SwitchLayerPreservesCCLayerState