Keep dedicated layers for hiding animation

ui/views/corewm/window_animations.cc

Index: ui/views/corewm/window_animations.cc
diff --git a/ui/views/corewm/window_animations.cc b/ui/views/corewm/window_animations.cc
index 42b26413129abdd166b339cd15de902c2393946e..26d1a7841a369028c829a42f7e0cd4f4e1526c88 100644
--- a/ui/views/corewm/window_animations.cc
+++ b/ui/views/corewm/window_animations.cc
@@ -34,6 +34,7 @@
 #include "ui/gfx/vector2d.h"
 #include "ui/gfx/vector3d_f.h"
 #include "ui/views/corewm/corewm_switches.h"
+#include "ui/views/corewm/transient_window_manager.h"
 #include "ui/views/corewm/window_util.h"
 
 DECLARE_WINDOW_PROPERTY_TYPE(int)
@@ -126,8 +127,44 @@ class HidingWindowAnimationObserver : public ui::ImplicitAnimationObserver,
       : window_(window) {
     window_->AddObserver(this);

[sky, 2014/03/07 15:14:54]

This code previously cared about the window so that if the window was desctroyed
it would grab the layers. Since you now grab the layers right away it seems like
it shouldn't need to be a WindowObserver nor have the window_ member. The only
thing I'm not sure about is 175-178 (new). I honestly don't understand that
code. Is it needed?

[oshima, 2014/03/08 00:14:30]

I tried that but it broke the test due to the AnimationHost not being called at
the end,
and this seems to be used by desktop aura.

(It's added here https://chromiumcodereview.appspot.com/12342028)

   }
+
   virtual ~HidingWindowAnimationObserver() {
-    STLDeleteElements(&layers_);
+  }
+
+  void DetachAndRecreateLayers() {
+    layer_.reset(views::corewm::RecreateWindowLayers(window_, true));

[sky, 2014/03/07 15:14:54]

nit: don't need views::corem here as you're in that namespace already.

[sky, 2014/03/07 15:14:54]

Layer does not own its children. So, the way you have it now you leak all the
descendants.

[Ben Goodger (Google), 2014/03/07 15:51:54]

Yeah RecreateWindowLayers seems like a weird API. How can it not leak all the
descendents (and view layers too).

Also this would reintroduce the bug that my last CL here fixed. Seems like
RecreateWindowLayers should become RecreateLayers() and do a proper deep copy of
the layer tree, returning ownership of the original tree to the caller.

[sky, 2014/03/07 16:34:57]

I'm going to change the return value of RecreateWindowLayers now as part of
another patch I'm working on.

[oshima, 2014/03/08 00:14:30]

Fixed to use DeepDeleteLayers for now. I'll update when sky's change is in.

+    // Use const window to avoid creating a TransientWindowManager
+    // if the window doesn't have one.
+    const aura::Window* const_window = window_;

[sky, 2014/03/07 15:14:54]

If you use the function in window_util it'll do the right thing and you don't
need the cast.

[oshima, 2014/03/08 00:14:30]

Ah, I didn't see that. Fixed

+    const TransientWindowManager* transient_window_manager =
+        TransientWindowManager::Get(const_window);
+    // If the window has transient children, move above the top most

[sky, 2014/03/07 15:14:54]

Document why you need to do this.

[oshima, 2014/03/08 00:14:30]

Done.

+    // transient child.
+    if (transient_window_manager) {
+      const aura::Window::Windows& transient_children =
+          transient_window_manager->transient_children();
+      aura::Window::Windows::const_iterator iter =
+          std::find(window_->parent()->children().begin(),
+                    window_->parent()->children().end(),
+                    window_);
+      aura::Window* topmost_transient_child = NULL;
+      DCHECK(iter != window_->parent()->children().end());
+      for (++iter; iter != window_->parent()->children().end(); ++iter) {
+        if (std::find(transient_children.begin(),
+                      transient_children.end(),
+                      *iter) !=
+            transient_children.end()) {
+          topmost_transient_child = *iter;
+        }
+      }
+      if (topmost_transient_child) {
+        window_->parent()->layer()->StackAbove(
+            layer_.get(), topmost_transient_child->layer());
+      }
+    }
+    // Make the new layer unvisible immediately.

[Ben Goodger (Google), 2014/03/07 15:51:54]

invisible

[oshima, 2014/03/08 00:14:30]

Done.

+    window_->layer()->SetVisible(false);
+    window_->layer()->SetOpacity(0);
   }
 
  private:
@@ -147,29 +184,32 @@ class HidingWindowAnimationObserver : public ui::ImplicitAnimationObserver,
   // Overridden from aura::WindowObserver:
   virtual void OnWindowDestroying(aura::Window* window) OVERRIDE {
     DCHECK_EQ(window, window_);
-    DCHECK(layers_.empty());
-    AcquireAllLayers(window_->layer());
     window_->RemoveObserver(this);
     window_ = NULL;
   }
 
-  void AcquireAllLayers(ui::Layer* layer) {
-    if (layer->owner()) {
-      ui::Layer* released = layer->owner()->AcquireLayer();
-      DCHECK_EQ(layer, released);
-      layers_.push_back(released);
-    }
-    std::vector<Layer*>::const_iterator it = layer->children().begin();
-    for (; it != layer->children().end(); ++it)
-      AcquireAllLayers(*it);
-  }
-
   aura::Window* window_;
-  std::vector<ui::Layer*> layers_;
+  scoped_ptr<ui::Layer> layer_;
 
   DISALLOW_COPY_AND_ASSIGN(HidingWindowAnimationObserver);
 };
 
+class ScopedLayerDetacher {

[sky, 2014/03/07 15:14:54]

Description?

[oshima, 2014/03/08 00:14:30]

Moved to header and added comment.

+ public:
+  explicit ScopedLayerDetacher(HidingWindowAnimationObserver* observer)
+      : observer_(observer) {}
+  ~ScopedLayerDetacher() {
+    observer_->DetachAndRecreateLayers();
+  }
+
+  HidingWindowAnimationObserver* observer() { return observer_; }
+
+ private:
+  HidingWindowAnimationObserver* observer_ ;  // not owned.
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedLayerDetacher);
+};
+
 void GetTransformRelativeToRoot(ui::Layer* layer, gfx::Transform* transform) {
   const Layer* root = layer;
   while (root->parent())
@@ -255,7 +295,8 @@ void AnimateHideWindowCommon(aura::Window* window,
 
   // Property sets within this scope will be implicitly animated.
   ui::ScopedLayerAnimationSettings settings(window->layer()->GetAnimator());
-  settings.AddObserver(new HidingWindowAnimationObserver(window));
+  ScopedLayerDetacher detacher(new HidingWindowAnimationObserver(window));
+  settings.AddObserver(detacher.observer());
 
   base::TimeDelta duration = GetWindowVisibilityAnimationDuration(*window);
   if (duration.ToInternalValue() > 0)
@@ -357,9 +398,11 @@ void AddLayerAnimationsForRotate(aura::Window* window, bool show) {
 
   base::TimeDelta duration = base::TimeDelta::FromMilliseconds(
       kWindowAnimation_Rotate_DurationMS);
+  scoped_ptr<ScopedLayerDetacher> layer_detacher;
 
   if (!show) {
-    new HidingWindowAnimationObserver(window);
+    layer_detacher.reset(new ScopedLayerDetacher(
+        new HidingWindowAnimationObserver(window)));
     window->layer()->GetAnimator()->SchedulePauseForProperties(
         duration * (100 - kWindowAnimation_Rotate_OpacityDurationPercent) / 100,
         ui::LayerAnimationElement::OPACITY);