Fixes bug where FocusController would stack a layer directly above a

ash/wm/workspace/workspace_manager_unittest.cc

Index: ash/wm/workspace/workspace_manager_unittest.cc
diff --git a/ash/wm/workspace/workspace_manager_unittest.cc b/ash/wm/workspace/workspace_manager_unittest.cc
index b4d9f7ac92e3b127b8cae60f5826031fa0599fc0..7a66afc38539874d603a3ab45e4c2fb3bfbe18dc 100644
--- a/ash/wm/workspace/workspace_manager_unittest.cc
+++ b/ash/wm/workspace/workspace_manager_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "ash/wm/workspace/workspace_manager.h"
 
+#include <map>
+
 #include "ash/ash_switches.h"
 #include "ash/root_window_controller.h"
 #include "ash/screen_ash.h"
@@ -39,6 +41,44 @@ using aura::Window;
 namespace ash {
 namespace internal {
 
+// Returns a string containing the names of all the children of |window| (in
+// order). Each entry is separated by a space.
+std::string GetWindowNames(const aura::Window* window) {
+  std::string result;
+  for (size_t i = 0; i < window->children().size(); ++i) {
+    if (i != 0)
+      result += " ";
+    result += window->children()[i]->name();
+  }
+  return result;
+}
+
+// Returns a string containing the names of windows corresponding to each of the
+// child layers of |window|'s layer. Any layers that don't correspond to a child
+// Window of |window| are ignored. The result is ordered based on the layer
+// ordering.
+std::string GetLayerNames(const aura::Window* window) {
+  typedef std::map<const ui::Layer*, std::string> LayerToWindowNameMap;
+  LayerToWindowNameMap window_names;
+  for (size_t i = 0; i < window->children().size(); ++i) {
+    window_names[window->children()[i]->layer()] =
+        window->children()[i]->name();
+  }
+
+  std::string result;
+  const std::vector<ui::Layer*>& layers(window->layer()->children());
+  for (size_t i = 0; i < layers.size(); ++i) {
+    LayerToWindowNameMap::iterator layer_i =
+        window_names.find(layers[i]);
+    if (layer_i != window_names.end()) {
+      if (!result.empty())
+        result += " ";
+      result += layer_i->second;
+    }
+  }
+  return result;
+}
+
 class WorkspaceManagerTest : public test::AshTestBase {
  public:
   WorkspaceManagerTest() : manager_(NULL) {}
@@ -1447,5 +1487,82 @@ TEST_F(WorkspaceManagerTest, AnimatedNormToMaxToNormRepositionsRemaining) {
   EXPECT_EQ("0,48 256x512", window2->bounds().ToString());
 }
 
+// This tests simulates a browser and an app and verifies the ordering of the
+// windows and layers doesn't get out of sync as various operations occur. Its
+// really testing code in FocusController, but easier to simulate here. Just as
+// with a real browser the browser here has a transient child window
+// (corresponds to the status bubble).
+TEST_F(WorkspaceManagerTest, VerifyLayerOrdering) {
+  aura::test::EventGenerator generator(
+      Shell::GetPrimaryRootWindow(), gfx::Point());
+  generator.MoveMouseTo(5, 5);

[stevenjb, 2013/03/22 18:28:15]

Could you document why the initial MoveMouseTo?

[sky, 2013/03/22 19:30:15]

That's not necessary and left over from another test. I'll nuke it.

+
+  scoped_ptr<Window> browser(
+      aura::test::CreateTestWindowWithDelegate(
+          NULL,
+          aura::client::WINDOW_TYPE_NORMAL,
+          gfx::Rect(5, 6, 7, 8),
+          NULL));
+  browser->SetName("browser");
+  SetDefaultParentByPrimaryRootWindow(browser.get());
+  browser->Show();
+  wm::ActivateWindow(browser.get());
+
+  // |status_bubble| is made a transient child of |browser| and as a result
+  // owned by |browser|.
+  aura::test::TestWindowDelegate* status_bubble_delegate =
+      aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate();
+  status_bubble_delegate->set_can_focus(false);
+  Window* status_bubble =
+      aura::test::CreateTestWindowWithDelegate(
+          status_bubble_delegate,
+          aura::client::WINDOW_TYPE_POPUP,
+          gfx::Rect(5, 6, 7, 8),
+          NULL);
+  browser->AddTransientChild(status_bubble);
+  SetDefaultParentByPrimaryRootWindow(status_bubble);
+  status_bubble->SetName("status_bubble");
+
+  scoped_ptr<Window> app(
+      aura::test::CreateTestWindowWithDelegate(
+          NULL,
+          aura::client::WINDOW_TYPE_NORMAL,
+          gfx::Rect(5, 6, 7, 8),
+          NULL));
+  app->SetName("app");
+  SetDefaultParentByPrimaryRootWindow(app.get());
+
+  aura::Window* parent = browser->parent();
+
+  app->Show();
+  wm::ActivateWindow(app.get());
+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Minimize the app, focus should go the browser.
+  app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);

[stevenjb, 2013/03/22 18:28:15]

Test that focus does go to the browser here? I know that's not what we are
testing, but it seems like it would help document the conditions for the test
setup better than just the comment.

[sky, 2013/03/22 19:30:15]

Sure.

+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Minimize the browser.
+  browser->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);

[stevenjb, 2013/03/22 18:28:15]

Mostly out of curiosity, where does focus go here? I assume neither window
should be focused now?

[sky, 2013/03/22 19:30:15]

I believe it goes no where, but I think ideally we want the launcher to get
focus in this case.

+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Show the browser (which should restore it).

[stevenjb, 2013/03/22 18:28:15]

But not focus it, correct?

[sky, 2013/03/22 19:30:15]

Not sure there. The focus rules may implicitly focus it. I'm not going to
assert, but will below.

+  browser->Show();
+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Activate the browser.
+  ash::wm::ActivateWindow(browser.get());

[stevenjb, 2013/03/22 18:28:15]

Until here, right?

+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Restore the app. This differs from above code for |browser| as internally
+  // the app code does this.
+  app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);

[stevenjb, 2013/03/22 18:28:15]

Should we change the App code to use Show() instead of Restore()? It seems like
they should be consistent. Making that change doesn't seem to affect the bug, so
it seems like it should be consistent in the test (or tested separately)?

[sky, 2013/03/22 19:30:15]

Yes, but it won't make a difference for this particular test. That said,
changing the app code to use show should fix 179387. I'll add a comment to that
bug.

+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+
+  // Activate the app.
+  ash::wm::ActivateWindow(app.get());
+  EXPECT_EQ(GetWindowNames(parent), GetLayerNames(parent));
+}
+
 }  // namespace internal
 }  // namespace ash