Fix a leak in wm::AnimateOnChildWindowVisibilityChanged().

ui/wm/core/window_animations.cc

Index: ui/wm/core/window_animations.cc
diff --git a/ui/wm/core/window_animations.cc b/ui/wm/core/window_animations.cc
index a79db3a15d6c45a76d6cdb78a8e3e7dc2db5035a..9cebba63f6c852f2a9c44d088a0ed40264ae40b0 100644
--- a/ui/wm/core/window_animations.cc
+++ b/ui/wm/core/window_animations.cc
@@ -58,7 +58,8 @@ const float kWindowAnimation_Vertical_TranslateY = 15.f;
 // The subclass will determine when the animation is completed.
 class HidingWindowAnimationObserverBase : public aura::WindowObserver {
  public:
-  HidingWindowAnimationObserverBase(aura::Window* window) : window_(window) {
+  explicit HidingWindowAnimationObserverBase(aura::Window* window)
+      : window_(window) {
     window_->AddObserver(this);
   }
   virtual ~HidingWindowAnimationObserverBase() {
@@ -108,6 +109,10 @@ class HidingWindowAnimationObserverBase : public aura::WindowObserver {
     }
   }
 
+  // Returns the root of the detached layer tree. Should be called only after
+  // DetachAndRecreateLayers(). The caller does not own the result.
+  ui::Layer* detached_layer_tree_root() { return layer_owner_->root(); }
+
  protected:
   // Invoked when the hiding animation is completed.  It will delete
   // 'this', and no operation should be made on this object after this
@@ -396,31 +401,30 @@ class RotateHidingWindowAnimationObserver
     : public HidingWindowAnimationObserverBase,
       public ui::LayerAnimationObserver {
  public:
-  RotateHidingWindowAnimationObserver(aura::Window* window)
-      : HidingWindowAnimationObserverBase(window), last_sequence_(NULL) {}
+  explicit RotateHidingWindowAnimationObserver(aura::Window* window)
+      : HidingWindowAnimationObserverBase(window) {}
   virtual ~RotateHidingWindowAnimationObserver() {}
 
-  void set_last_sequence(ui::LayerAnimationSequence* last_sequence) {
-    last_sequence_ = last_sequence;
+  // The observer destroys itself after the |last_sequence| ends or is aborted.
+  // Does not take ownership of the |last_sequence| parameter, which should not
+  // be NULL.
+  void SetLastSequence(ui::LayerAnimationSequence* last_sequence) {
+    last_sequence->AddObserver(this);
   }
 
   // ui::LayerAnimationObserver:
   virtual void OnLayerAnimationEnded(
       ui::LayerAnimationSequence* sequence) OVERRIDE {
-    if (last_sequence_ == sequence)
-      OnAnimationCompleted();
+    OnAnimationCompleted();
   }
   virtual void OnLayerAnimationAborted(
       ui::LayerAnimationSequence* sequence) OVERRIDE {
-    if (last_sequence_ == sequence)
-      OnAnimationCompleted();
+    OnAnimationCompleted();
   }
   virtual void OnLayerAnimationScheduled(
       ui::LayerAnimationSequence* sequence) OVERRIDE {}
 
  private:
-  ui::LayerAnimationSequence* last_sequence_;
-
   DISALLOW_COPY_AND_ASSIGN(RotateHidingWindowAnimationObserver);
 };
 
@@ -481,10 +485,14 @@ void AddLayerAnimationsForRotate(aura::Window* window, bool show) {
           rotation.release(), duration));
   ui::LayerAnimationSequence* last_sequence =
       new ui::LayerAnimationSequence(transition.release());
-  window->layer()->GetAnimator()->ScheduleAnimation(last_sequence);
+
   if (observer) {
-    observer->set_last_sequence(last_sequence);
+    observer->SetLastSequence(last_sequence);
     observer->DetachAndRecreateLayers();
+    observer->detached_layer_tree_root()->GetAnimator()->ScheduleAnimation(

[sky, 2014/07/14 19:18:18]

Isn't the important part here your changes to SetLastSequence? In which case
can't you leave 484 old and nuke 492-495 new?

[please use gerrit instead, 2014/07/14 20:12:17]

Yes.
Yes, but then we would not have a unit test for checking the leak. Is that OK
with you? Here is why:

Calling [ window->layer()->GetAnimator()->ScheduleAnimation(last_sequence) ]
before [ observer->DetachAndRecreateLayers() ] works when running in the
browser, because scheduled animations will complete after exiting [
AddLayerAnimationsForRotate() ] function.

However, unit tests run animations in either (a) zero time or (b) not at all, in
my experience.

a) Zero time animations finish [ last_sequence ] before [ set_last_sequence() ],
so this causes a leak in the new unit test.

b) Any other speed of animations does not finish the animations in unit tests.
Only [ OnLayerAnimationScheduled() ] gets called. [ OnLayerAnimationEnded() ]
and [ OnLayerAnimationAborted() ] are never called. This also causes a leak in
the new unit test.

TL;DR: Simpler change means no unit test.

[please use gerrit instead, 2014/07/17 03:14:00]

Done.

+        last_sequence);
+  } else {
+    window->layer()->GetAnimator()->ScheduleAnimation(last_sequence);
   }
 }