Make Aura and compositor stacking methods more intuitive.

aura::Window::StackChildAbove() and ui::Layer::StackAbove()
stacked entity A directly above entity B... unless A was
already somewhere above B in the stack, in which case they'd
do nothing.  I see the caveat as making these methods less
useful, and I'm unable to think of any benefits from it, so
this change makes the methods place A directly above B in
all cases (possibly lowering A in the stacking order if it
was previously far above B).

This also fixes an issue in the Aura shell's shadow-stacking
code where ShadowController could inadvertently raise a
window to the top of the stacking order.

BUG=101977
TEST=added

Review URL: http://codereview.chromium.org/8653005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111264 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 127 insertions, 23 deletions

diff --git a/ui/aura/window.cc b/ui/aura/window.cc
index 3e6f6c62..6b4c5d2 100644
--- a/ui/aura/window.cc
+++ b/ui/aura/window.cc
@@ -196,18 +196,17 @@ void Window::StackChildAbove(Window* child, Window* other) {
   DCHECK_EQ(this, child->parent());
   DCHECK_EQ(this, other->parent());
 
-  size_t child_i =
+  const size_t child_i =
       std::find(children_.begin(), children_.end(), child) - children_.begin();
-  size_t other_i =
+  const size_t other_i =
       std::find(children_.begin(), children_.end(), other) - children_.begin();
-  if (child_i > other_i)
-    return;  // Already in front of |other|.
+  if (child_i == other_i + 1)
+    return;
 
-  // Reorder children.
+  const size_t dest_i = child_i < other_i ? other_i : other_i + 1;
   children_.erase(children_.begin() + child_i);
-  children_.insert(children_.begin() + other_i, child);
+  children_.insert(children_.begin() + dest_i, child);
 
-  // Reorder the layer.
   layer()->StackAbove(child->layer(), other->layer());
 
   // Stack any transient children that share the same parent to be in front of
diff --git a/ui/aura/window_unittest.cc b/ui/aura/window_unittest.cc
index a74c9b2..9d5f368 100644
--- a/ui/aura/window_unittest.cc
+++ b/ui/aura/window_unittest.cc
@@ -271,8 +271,8 @@ TEST_F(WindowTest, StackChildAtTop) {
   EXPECT_EQ(child2.layer(), parent.layer()->children()[0]);
 }
 
-// Various assertions for StackAtTop.
-TEST_F(WindowTest, StackAtTop) {
+// Various assertions for StackChildAbove.
+TEST_F(WindowTest, StackChildAbove) {
   Window parent(NULL);
   parent.Init(ui::Layer::LAYER_HAS_NO_TEXTURE);
   Window child1(NULL);
@@ -307,7 +307,7 @@ TEST_F(WindowTest, StackAtTop) {
   EXPECT_EQ(child2.layer(), parent.layer()->children()[1]);
   EXPECT_EQ(child3.layer(), parent.layer()->children()[2]);
 
-  // Move 1 in front of 3, resulting in [2 3 1].
+  // Move 1 in front of 3, resulting in [2, 3, 1].
   parent.StackChildAbove(&child1, &child3);
   ASSERT_EQ(3u, parent.children().size());
   EXPECT_EQ(&child2, parent.children()[0]);
@@ -317,6 +317,17 @@ TEST_F(WindowTest, StackAtTop) {
   EXPECT_EQ(child2.layer(), parent.layer()->children()[0]);
   EXPECT_EQ(child3.layer(), parent.layer()->children()[1]);
   EXPECT_EQ(child1.layer(), parent.layer()->children()[2]);
+
+  // Moving 1 in front of 2 should lower it, resulting in [2, 1, 3].
+  parent.StackChildAbove(&child1, &child2);
+  ASSERT_EQ(3u, parent.children().size());
+  EXPECT_EQ(&child2, parent.children()[0]);
+  EXPECT_EQ(&child1, parent.children()[1]);
+  EXPECT_EQ(&child3, parent.children()[2]);
+  ASSERT_EQ(3u, parent.layer()->children().size());
+  EXPECT_EQ(child2.layer(), parent.layer()->children()[0]);
+  EXPECT_EQ(child1.layer(), parent.layer()->children()[1]);
+  EXPECT_EQ(child3.layer(), parent.layer()->children()[2]);
 }
 
 // Various destruction assertions.
diff --git a/ui/aura_shell/shadow_controller_unittest.cc b/ui/aura_shell/shadow_controller_unittest.cc
index 523543f..d045741 100644
--- a/ui/aura_shell/shadow_controller_unittest.cc
+++ b/ui/aura_shell/shadow_controller_unittest.cc
@@ -2,15 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ui/aura_shell/shadow_controller.h"
+
+#include <algorithm>
+#include <vector>
+
 #include "base/memory/scoped_ptr.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/shadow_types.h"
 #include "ui/aura/desktop.h"
 #include "ui/aura/window.h"
 #include "ui/aura_shell/shadow.h"
-#include "ui/aura_shell/shadow_controller.h"
 #include "ui/aura_shell/shell.h"
 #include "ui/aura_shell/test/aura_shell_test_base.h"
+#include "ui/gfx/compositor/layer.h"
 
 namespace aura_shell {
 namespace test {
@@ -48,9 +53,6 @@ TEST_F(ShadowControllerTest, Shadow) {
   // The shadow's layer should have the same parent as the window's.
   EXPECT_EQ(window->parent()->layer(), shadow->layer()->parent());
 
-  // TODO(derat): Test stacking (after adding additional methods to ui::Layer so
-  // that stacking order can be queried).
-
   // When we remove the window from the hierarchy, its shadow should be removed
   // too.
   window->parent()->RemoveChild(window.get());
@@ -86,5 +88,46 @@ TEST_F(ShadowControllerTest, ShadowBounds) {
   EXPECT_EQ(kNewBounds, shadow->content_bounds());
 }
 
+// Test that shadows are stacked correctly.
+TEST_F(ShadowControllerTest, Stacking) {
+  scoped_ptr<aura::Window> window(new aura::Window(NULL));
+  window->SetType(aura::WINDOW_TYPE_NORMAL);
+  window->Init(ui::Layer::LAYER_HAS_TEXTURE);
+  window->SetParent(NULL);
+  window->Show();
+
+  // Create a second window.  It will appear above the first window.
+  scoped_ptr<aura::Window> window2(new aura::Window(NULL));
+  window2->SetType(aura::WINDOW_TYPE_NORMAL);
+  window2->Init(ui::Layer::LAYER_HAS_TEXTURE);
+  window2->SetParent(NULL);
+  window2->Show();
+
+  // Enable a shadow on the first window.
+  window->SetIntProperty(aura::kShadowTypeKey, aura::SHADOW_TYPE_RECTANGULAR);
+  internal::ShadowController::TestApi api(
+      aura_shell::Shell::GetInstance()->shadow_controller());
+  const internal::Shadow* shadow = api.GetShadowForWindow(window.get());
+  ASSERT_TRUE(shadow != NULL);
+
+  // Check that the second window is above the first window and that the first
+  // window is above its shadow.
+  ui::Layer* parent_layer = window->layer()->parent();
+  ASSERT_EQ(parent_layer, shadow->layer()->parent());
+  ASSERT_EQ(parent_layer, window2->layer()->parent());
+  const std::vector<ui::Layer*>& layers = parent_layer->children();
+  EXPECT_GT(std::find(layers.begin(), layers.end(), window2->layer()),
+            std::find(layers.begin(), layers.end(), window->layer()));
+  EXPECT_GT(std::find(layers.begin(), layers.end(), window->layer()),
+            std::find(layers.begin(), layers.end(), shadow->layer()));
+
+  // Raise the first window to the top and check that its shadow comes with it.
+  window->parent()->StackChildAtTop(window.get());
+  EXPECT_GT(std::find(layers.begin(), layers.end(), window->layer()),
+            std::find(layers.begin(), layers.end(), shadow->layer()));
+  EXPECT_GT(std::find(layers.begin(), layers.end(), shadow->layer()),
+            std::find(layers.begin(), layers.end(), window2->layer()));
+}
+
 }  // namespace test
 }  // namespace aura_shell
diff --git a/ui/gfx/compositor/layer.cc b/ui/gfx/compositor/layer.cc
index 26e2699..67fe014 100644
--- a/ui/gfx/compositor/layer.cc
+++ b/ui/gfx/compositor/layer.cc
@@ -135,22 +135,23 @@ void Layer::StackAbove(Layer* child, Layer* other) {
   DCHECK_NE(child, other);
   DCHECK_EQ(this, child->parent());
   DCHECK_EQ(this, other->parent());
-  size_t child_i =
+
+  const size_t child_i =
       std::find(children_.begin(), children_.end(), child) - children_.begin();
-  size_t other_i =
+  const size_t other_i =
       std::find(children_.begin(), children_.end(), other) - children_.begin();
-  if (child_i > other_i)
-    return;  // Already in front of |other|.
+  if (child_i == other_i + 1)
+    return;
 
-  // Reorder children.
+  const size_t dest_i = child_i < other_i ? other_i : other_i + 1;
   children_.erase(children_.begin() + child_i);
-  children_.insert(children_.begin() + other_i, child);
+  children_.insert(children_.begin() + dest_i, child);
 
   SetNeedsToRecomputeHole();
 
 #if defined(USE_WEBKIT_COMPOSITOR)
   child->web_layer_.removeFromParent();
-  web_layer_.insertChild(child->web_layer_, other_i);
+  web_layer_.insertChild(child->web_layer_, dest_i);
 #endif
 }
 
diff --git a/ui/gfx/compositor/layer.h b/ui/gfx/compositor/layer.h
index 19949a1..5bc3819 100644
--- a/ui/gfx/compositor/layer.h
+++ b/ui/gfx/compositor/layer.h
@@ -72,8 +72,9 @@ class COMPOSITOR_EXPORT Layer :
   // Stacks |child| above all other children.
   void StackAtTop(Layer* child);
 
-  // Stacks |child| above |other|, both of which must be children of this layer.
-  // Does nothing if |other| is already above |child|.
+  // Stacks |child| directly above |other|.  Both must be children of this
+  // layer.  Note that if |child| is initially stacked even higher, calling this
+  // method will result in |child| being lowered in the stacking order.
   void StackAbove(Layer* child, Layer* other);
 
   // Returns the child Layers.
@@ -301,6 +302,7 @@ class COMPOSITOR_EXPORT Layer :
 
   Layer* parent_;
 
+  // This layer's children, in bottom-to-top stacking order.
   std::vector<Layer*> children_;
 
   ui::Transform transform_;
diff --git a/ui/gfx/compositor/layer_unittest.cc b/ui/gfx/compositor/layer_unittest.cc
index 2e96cca..253683c 100644
--- a/ui/gfx/compositor/layer_unittest.cc
+++ b/ui/gfx/compositor/layer_unittest.cc
@@ -90,6 +90,19 @@ bool IsSameAsPNGFile(const SkBitmap& gen_bmp, FilePath ref_img_path) {
   return true;
 }
 
+// Returns a comma-separated list of the names of |layer|'s children in
+// bottom-to-top stacking order.
+std::string GetLayerChildrenNames(const Layer& layer) {
+  std::string names;
+  for (std::vector<Layer*>::const_iterator it = layer.children().begin();
+       it != layer.children().end(); ++it) {
+    if (!names.empty())
+      names += ",";
+    names += (*it)->name();
+  }
+  return names;
+}
+
 
 // There are three test classes in here that configure the Compositor and
 // Layer's slightly differently:
@@ -1047,6 +1060,41 @@ TEST_F(LayerWithNullDelegateTest, Visibility) {
 #endif
 }
 
+// Checks that stacking-related methods behave as advertised.
+TEST_F(LayerWithNullDelegateTest, Stacking) {
+  scoped_ptr<Layer> root(new Layer(Layer::LAYER_HAS_NO_TEXTURE));
+  scoped_ptr<Layer> l1(new Layer(Layer::LAYER_HAS_TEXTURE));
+  scoped_ptr<Layer> l2(new Layer(Layer::LAYER_HAS_TEXTURE));
+  scoped_ptr<Layer> l3(new Layer(Layer::LAYER_HAS_TEXTURE));
+  l1->set_name("1");
+  l2->set_name("2");
+  l3->set_name("3");
+  root->Add(l3.get());
+  root->Add(l2.get());
+  root->Add(l1.get());
+
+  // Layers' children are stored in bottom-to-top order.
+  EXPECT_EQ("3,2,1", GetLayerChildrenNames(*root.get()));
+
+  root->StackAtTop(l3.get());
+  EXPECT_EQ("2,1,3", GetLayerChildrenNames(*root.get()));
+
+  root->StackAtTop(l1.get());
+  EXPECT_EQ("2,3,1", GetLayerChildrenNames(*root.get()));
+
+  root->StackAtTop(l1.get());
+  EXPECT_EQ("2,3,1", GetLayerChildrenNames(*root.get()));
+
+  root->StackAbove(l2.get(), l3.get());
+  EXPECT_EQ("3,2,1", GetLayerChildrenNames(*root.get()));
+
+  root->StackAbove(l1.get(), l3.get());
+  EXPECT_EQ("3,1,2", GetLayerChildrenNames(*root.get()));
+
+  root->StackAbove(l2.get(), l1.get());
+  EXPECT_EQ("3,1,2", GetLayerChildrenNames(*root.get()));
+}
+
 // Checks that the invalid rect assumes correct values when setting bounds.
 // TODO(vollick): for USE_WEBKIT_COMPOSITOR, use WebKit's dirty rect.
 TEST_F(LayerWithNullDelegateTest,