Remove ShellDelegate::GetCycleWindowList. Because alt-tab operates on

all windows now there is no need to push the implementation to the
delegate and it can instead be consolidated. Also added a couple more
test cases, and fixed a bug that occurred if there is a single window
but it wasn't active.

BUG=120987
TEST=see bug, also covered by test.
R=jamescook@chromium.org


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

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

15 files changed, 134 insertions, 110 deletions

diff --git a/ash/focus_cycler.cc b/ash/focus_cycler.cc
index 242aab4..aa9cf64 100644
--- a/ash/focus_cycler.cc
+++ b/ash/focus_cycler.cc
@@ -5,13 +5,12 @@
 #include "ash/focus_cycler.h"
 
 #include "ash/shell.h"
-#include "ash/shell_delegate.h"
 #include "ash/system/tray/system_tray.h"
+#include "ash/wm/window_cycle_controller.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/focus/focus_search.h"
 #include "ui/aura/window.h"
 #include "ui/aura/client/activation_client.h"
-
 #include "ui/views/accessible_pane_view.h"
 
 namespace ash {
@@ -62,15 +61,13 @@ void FocusCycler::RotateFocus(Direction direction) {
       break;
 
     if (index == browser_index) {
-      // Activate the browser window.
-      const std::vector<aura::Window*>& windows =
-          Shell::GetInstance()->delegate()->GetCycleWindowList(
-              ShellDelegate::SOURCE_LAUNCHER);
-      if (!windows.empty()) {
-        aura::client::GetActivationClient(windows[0]->GetRootWindow())->
-            ActivateWindow(windows[0]);
-        break;
-      }
+      // Activate the first window.
+      WindowCycleController::Direction window_direction =
+          direction == FORWARD ? WindowCycleController::FORWARD :
+                                 WindowCycleController::BACKWARD;
+      ash::Shell::GetInstance()->window_cycle_controller()->HandleCycleWindow(
+          window_direction, false);
+      break;
     } else {
       if (FocusWidget(widgets_[index]))
         break;
diff --git a/ash/shell/shell_delegate_impl.cc b/ash/shell/shell_delegate_impl.cc
index 4ceca7b..b736473 100644
--- a/ash/shell/shell_delegate_impl.cc
+++ b/ash/shell/shell_delegate_impl.cc
@@ -68,16 +68,6 @@ ash::AppListViewDelegate* ShellDelegateImpl::CreateAppListViewDelegate() {
   return ash::shell::CreateAppListViewDelegate();
 }
 
-std::vector<aura::Window*> ShellDelegateImpl::GetCycleWindowList(
-    CycleSource source) const {
-  aura::Window* default_container = ash::Shell::GetInstance()->GetContainer(
-      ash::internal::kShellWindowId_DefaultContainer);
-  std::vector<aura::Window*> windows = default_container->children();
-  // Window cycling expects the topmost window at the front of the list.
-  std::reverse(windows.begin(), windows.end());
-  return windows;
-}
-
 void ShellDelegateImpl::StartPartialScreenshot(
     ash::ScreenshotDelegate* screenshot_delegate) {
   ash::PartialScreenshotView::StartPartialScreenshot(screenshot_delegate);
diff --git a/ash/shell/shell_delegate_impl.h b/ash/shell/shell_delegate_impl.h
index 6322cd4..7698830 100644
--- a/ash/shell/shell_delegate_impl.h
+++ b/ash/shell/shell_delegate_impl.h
@@ -30,8 +30,6 @@ class ShellDelegateImpl : public ash::ShellDelegate {
   virtual void Exit() OVERRIDE;
   virtual void NewWindow(bool incognito) OVERRIDE;
   virtual ash::AppListViewDelegate* CreateAppListViewDelegate() OVERRIDE;
-  virtual std::vector<aura::Window*> GetCycleWindowList(
-      CycleSource source) const OVERRIDE;
   virtual void StartPartialScreenshot(
       ash::ScreenshotDelegate* screenshot_delegate) OVERRIDE;
   virtual ash::LauncherDelegate* CreateLauncherDelegate(
diff --git a/ash/shell_delegate.h b/ash/shell_delegate.h
index 9c6929b..e571471 100644
--- a/ash/shell_delegate.h
+++ b/ash/shell_delegate.h
@@ -35,15 +35,6 @@ class UserWallpaperDelegate;
 // Delegate of the Shell.
 class ASH_EXPORT ShellDelegate {
  public:
-  // Source requesting the window list.
-  enum CycleSource {
-    // Windows are going to be used for alt-tab (or F5).
-    SOURCE_KEYBOARD,
-
-    // Windows are going to be cycled from the launcher.
-    SOURCE_LAUNCHER,
-  };
-
   // The Shell owns the delegate.
   virtual ~ShellDelegate() {}
 
@@ -72,14 +63,6 @@ class ASH_EXPORT ShellDelegate {
   // the created delegate.
   virtual AppListViewDelegate* CreateAppListViewDelegate() = 0;
 
-  // Returns a list of windows to cycle with keyboard shortcuts (e.g. alt-tab
-  // or the window switching key).  If |order_by_activity| is true then windows
-  // are returned in most-recently-used order with the currently active window
-  // at the front of the list.  Otherwise any order may be returned.  The list
-  // does not contain NULL pointers.
-  virtual std::vector<aura::Window*> GetCycleWindowList(
-      CycleSource source) const = 0;
-
   // Invoked to start taking partial screenshot.
   virtual void StartPartialScreenshot(
       ScreenshotDelegate* screenshot_delegate) = 0;
diff --git a/ash/test/test_shell_delegate.cc b/ash/test/test_shell_delegate.cc
index 7c9f4c3..3489412 100644
--- a/ash/test/test_shell_delegate.cc
+++ b/ash/test/test_shell_delegate.cc
@@ -51,18 +51,6 @@ AppListViewDelegate* TestShellDelegate::CreateAppListViewDelegate() {
   return NULL;
 }
 
-std::vector<aura::Window*> TestShellDelegate::GetCycleWindowList(
-    CycleSource source) const {
-  // We just use the Shell's default container of windows, so tests can be
-  // written with the usual CreateTestWindowWithId() calls. But window cycling
-  // expects the topmost window at the front of the list, so reverse the order.
-  aura::Window* default_container = Shell::GetInstance()->GetContainer(
-      internal::kShellWindowId_DefaultContainer);
-  std::vector<aura::Window*> windows = default_container->children();
-  std::reverse(windows.begin(), windows.end());
-  return windows;
-}
-
 void TestShellDelegate::StartPartialScreenshot(
     ScreenshotDelegate* screenshot_delegate) {
   if (screenshot_delegate)
diff --git a/ash/test/test_shell_delegate.h b/ash/test/test_shell_delegate.h
index 0766032..f822617 100644
--- a/ash/test/test_shell_delegate.h
+++ b/ash/test/test_shell_delegate.h
@@ -26,8 +26,6 @@ class TestShellDelegate : public ShellDelegate {
   virtual void Exit() OVERRIDE;
   virtual void NewWindow(bool incognito) OVERRIDE;
   virtual AppListViewDelegate* CreateAppListViewDelegate() OVERRIDE;
-  virtual std::vector<aura::Window*> GetCycleWindowList(
-      CycleSource source) const OVERRIDE;
   virtual void StartPartialScreenshot(
       ScreenshotDelegate* screenshot_delegate) OVERRIDE;
   virtual LauncherDelegate* CreateLauncherDelegate(
diff --git a/ash/wm/window_cycle_controller.cc b/ash/wm/window_cycle_controller.cc
index 0df8bed..771ea2b 100644
--- a/ash/wm/window_cycle_controller.cc
+++ b/ash/wm/window_cycle_controller.cc
@@ -4,8 +4,11 @@
 
 #include "ash/wm/window_cycle_controller.h"
 
+#include <algorithm>
+
 #include "ash/shell.h"
 #include "ash/shell_delegate.h"
+#include "ash/shell_window_ids.h"
 #include "ash/wm/window_cycle_list.h"
 #include "ash/wm/window_util.h"
 #include "ui/aura/event.h"
@@ -122,13 +125,28 @@ void WindowCycleController::AltKeyReleased() {
   StopCycling();
 }
 
+// static
+std::vector<aura::Window*> WindowCycleController::BuildWindowList() {
+  aura::Window* default_container = ash::Shell::GetInstance()->GetContainer(
+    ash::internal::kShellWindowId_DefaultContainer);
+  WindowCycleList::WindowList windows = default_container->children();
+  // Removes unfocusable windows.
+  WindowCycleList::WindowList::iterator last =
+      std::remove_if(
+          windows.begin(),
+          windows.end(),
+          std::not1(std::ptr_fun(ash::wm::CanActivateWindow)));
+  windows.erase(last, windows.end());
+  // Window cycling expects the topmost window at the front of the list.
+  std::reverse(windows.begin(), windows.end());
+  return windows;
+}
+
 //////////////////////////////////////////////////////////////////////////////
 // WindowCycleController, private:
 
 void WindowCycleController::StartCycling() {
-  windows_.reset(new WindowCycleList(
-      ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
-          ShellDelegate::SOURCE_KEYBOARD)));
+  windows_.reset(new WindowCycleList(BuildWindowList()));
 }
 
 void WindowCycleController::Step(Direction direction) {
diff --git a/ash/wm/window_cycle_controller.h b/ash/wm/window_cycle_controller.h
index fc2e514..301a6ab 100644
--- a/ash/wm/window_cycle_controller.h
+++ b/ash/wm/window_cycle_controller.h
@@ -6,6 +6,8 @@
 #define ASH_WM_WINDOW_CYCLE_CONTROLLER_H_
 #pragma once
 
+#include <vector>
+
 #include "ash/ash_export.h"
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
@@ -49,6 +51,14 @@ class ASH_EXPORT WindowCycleController {
   // Returns true if we are in the middle of a window cycling gesture.
   bool IsCycling() const { return windows_.get() != NULL; }
 
+  // Returns the WindowCycleList. Really only useful for testing.
+  const WindowCycleList* windows() const { return windows_.get(); }
+
+  // Returns the set of windows to cycle through. This method creates the vector
+  // based on the current set of windows. As a result it is not necessarily the
+  // same as the set of windows being iterated over.
+  static std::vector<aura::Window*> BuildWindowList();
+
  private:
   // Call to start cycling windows.  You must call StopCycling() when done.
   void StartCycling();
diff --git a/ash/wm/window_cycle_controller_unittest.cc b/ash/wm/window_cycle_controller_unittest.cc
index 3aaac40..006d766 100644
--- a/ash/wm/window_cycle_controller_unittest.cc
+++ b/ash/wm/window_cycle_controller_unittest.cc
@@ -10,6 +10,7 @@
 #include "ash/shell_window_ids.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/test_shell_delegate.h"
+#include "ash/wm/window_cycle_list.h"
 #include "ash/wm/window_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/aura/test/test_windows.h"
@@ -24,23 +25,13 @@ using aura::test::CreateTestWindowWithId;
 using aura::test::TestWindowDelegate;
 using aura::Window;
 
-class WindowCycleControllerTest : public test::AshTestBase {
- public:
-  WindowCycleControllerTest() {}
-  virtual ~WindowCycleControllerTest() {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WindowCycleControllerTest);
-};
+typedef test::AshTestBase WindowCycleControllerTest;
 
 TEST_F(WindowCycleControllerTest, HandleCycleWindowBaseCases) {
   WindowCycleController* controller =
       Shell::GetInstance()->window_cycle_controller();
 
   // Cycling doesn't crash if there are no windows.
-  std::vector<Window*> windows = Shell::GetInstance()->delegate()->
-      GetCycleWindowList(ShellDelegate::SOURCE_KEYBOARD);
-  EXPECT_TRUE(windows.empty());
   controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
 
   // Create a single test window.
@@ -56,6 +47,30 @@ TEST_F(WindowCycleControllerTest, HandleCycleWindowBaseCases) {
   EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
 }
 
+// Verifies if there is only one window and it isn't active that cycling
+// activates it.
+TEST_F(WindowCycleControllerTest, SingleWindowNotActive) {
+  WindowCycleController* controller =
+      Shell::GetInstance()->window_cycle_controller();
+
+  // Create a single test window.
+  Window* default_container =
+      ash::Shell::GetInstance()->GetContainer(
+          internal::kShellWindowId_DefaultContainer);
+  scoped_ptr<Window> window0(CreateTestWindowWithId(0, default_container));
+  wm::ActivateWindow(window0.get());
+  EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
+
+  // Rotate focus, this should move focus to another window that isn't part of
+  // the default container.
+  Shell::GetInstance()->RotateFocus(Shell::FORWARD);
+  EXPECT_FALSE(wm::IsActiveWindow(window0.get()));
+
+  // Cycling should activate the window.
+  controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
+  EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
+}
+
 TEST_F(WindowCycleControllerTest, HandleCycleWindow) {
   WindowCycleController* controller =
       Shell::GetInstance()->window_cycle_controller();
@@ -70,17 +85,17 @@ TEST_F(WindowCycleControllerTest, HandleCycleWindow) {
   scoped_ptr<Window> window0(CreateTestWindowWithId(0, default_container));
   wm::ActivateWindow(window0.get());
 
-  // Window lists should return the topmost window in front.
-  std::vector<Window*> windows = Shell::GetInstance()->delegate()->
-      GetCycleWindowList(ShellDelegate::SOURCE_KEYBOARD);
-  ASSERT_EQ(3u, windows.size());
-  ASSERT_EQ(window0.get(), windows[0]);
-  ASSERT_EQ(window1.get(), windows[1]);
-  ASSERT_EQ(window2.get(), windows[2]);
-
   // Simulate pressing and releasing Alt-tab.
   EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
   controller->HandleCycleWindow(WindowCycleController::FORWARD, true);
+
+  // Window lists should return the topmost window in front.
+  ASSERT_TRUE(controller->windows());
+  ASSERT_EQ(3u, controller->windows()->windows().size());
+  ASSERT_EQ(window0.get(), controller->windows()->windows()[0]);
+  ASSERT_EQ(window1.get(), controller->windows()->windows()[1]);
+  ASSERT_EQ(window2.get(), controller->windows()->windows()[2]);
+
   controller->AltKeyReleased();
   EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
 
@@ -167,6 +182,55 @@ TEST_F(WindowCycleControllerTest, HandleCycleWindow) {
   EXPECT_FALSE(wm::IsActiveWindow(window2.get()));
 }
 
+// Cycles between a maximized and normal window.
+TEST_F(WindowCycleControllerTest, MaximizedWindow) {
+  // Create a couple of test windows.
+  Window* default_container =
+      ash::Shell::GetInstance()->GetContainer(
+          internal::kShellWindowId_DefaultContainer);
+  scoped_ptr<Window> window0(CreateTestWindowWithId(0, default_container));
+  scoped_ptr<Window> window1(CreateTestWindowWithId(1, default_container));
+
+  wm::MaximizeWindow(window1.get());
+  wm::ActivateWindow(window1.get());
+  EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
+
+  // Rotate focus, this should move focus to window0.
+  WindowCycleController* controller =
+      Shell::GetInstance()->window_cycle_controller();
+  controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
+  EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
+
+  // One more time.
+  controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
+  EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
+}
+
+// Cycles to a minimized window.
+TEST_F(WindowCycleControllerTest, Minimized) {
+  // Create a couple of test windows.
+  Window* default_container =
+      ash::Shell::GetInstance()->GetContainer(
+          internal::kShellWindowId_DefaultContainer);
+  scoped_ptr<Window> window0(CreateTestWindowWithId(0, default_container));
+  scoped_ptr<Window> window1(CreateTestWindowWithId(1, default_container));
+
+  wm::MinimizeWindow(window1.get());
+  wm::ActivateWindow(window0.get());
+  EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
+
+  // Rotate focus, this should move focus to window1 and unminimize it.
+  WindowCycleController* controller =
+      Shell::GetInstance()->window_cycle_controller();
+  controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
+  EXPECT_FALSE(wm::IsWindowMinimized(window1.get()));
+  EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
+
+  // One more time back to w0.
+  controller->HandleCycleWindow(WindowCycleController::FORWARD, false);
+  EXPECT_TRUE(wm::IsActiveWindow(window0.get()));
+}
+
 }  // namespace
 
 }  // namespace ash
diff --git a/ash/wm/window_cycle_list.cc b/ash/wm/window_cycle_list.cc
index 5e7480d..7ea875b 100644
--- a/ash/wm/window_cycle_list.cc
+++ b/ash/wm/window_cycle_list.cc
@@ -33,8 +33,7 @@ WindowCycleList::~WindowCycleList() {
 }
 
 void WindowCycleList::Step(Direction direction) {
-  // Ensure we have more than one window to step to.
-  if (windows_.size() <= 1)
+  if (windows_.empty())
     return;
 
   if (current_index_ == -1) {
@@ -42,6 +41,8 @@ void WindowCycleList::Step(Direction direction) {
     // provided window list.  Just switch to the first (or last) one.
     current_index_ = (direction == FORWARD ? 0 : windows_.size() - 1);
   } else {
+    if (windows_.size() == 1)
+      return;
     // We're in a valid cycle, so step forward or backward.
     current_index_ += (direction == FORWARD ? 1 : -1);
   }
diff --git a/ash/wm/window_cycle_list.h b/ash/wm/window_cycle_list.h
index aad0658..be12da6 100644
--- a/ash/wm/window_cycle_list.h
+++ b/ash/wm/window_cycle_list.h
@@ -34,6 +34,8 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver {
   // Cycles to the next or previous window based on |direction|.
   void Step(Direction direction);
 
+  const WindowList& windows() const { return windows_; }
+
  private:
   // Returns the index of |window| in |windows_| or -1 if it isn't there.
   int GetWindowIndex(aura::Window* window);
diff --git a/chrome/browser/ui/views/ash/chrome_shell_delegate.cc b/chrome/browser/ui/views/ash/chrome_shell_delegate.cc
index af6184c..4c0006f 100644
--- a/chrome/browser/ui/views/ash/chrome_shell_delegate.cc
+++ b/chrome/browser/ui/views/ash/chrome_shell_delegate.cc
@@ -4,10 +4,7 @@
 
 #include "chrome/browser/ui/views/ash/chrome_shell_delegate.h"
 
-#include <algorithm>
-
 #include "ash/launcher/launcher_types.h"
-#include "ash/shell_window_ids.h"
 #include "ash/system/tray/system_tray_delegate.h"
 #include "ash/wm/partial_screenshot_view.h"
 #include "ash/wm/window_util.h"
@@ -120,23 +117,6 @@ ChromeShellDelegate::CreateAppListViewDelegate() {
   return new AppListViewDelegate;
 }
 
-std::vector<aura::Window*> ChromeShellDelegate::GetCycleWindowList(
-    CycleSource source) const {
-  aura::Window* default_container = ash::Shell::GetInstance()->GetContainer(
-    ash::internal::kShellWindowId_DefaultContainer);
-  std::vector<aura::Window*> windows = default_container->children();
-  // Removes unforcusable windows.
-  std::vector<aura::Window*>::iterator last =
-      std::remove_if(
-          windows.begin(),
-          windows.end(),
-          std::not1(std::ptr_fun(ash::wm::CanActivateWindow)));
-  windows.erase(last, windows.end());
-  // Window cycling expects the topmost window at the front of the list.
-  std::reverse(windows.begin(), windows.end());
-  return windows;
-}
-
 void ChromeShellDelegate::StartPartialScreenshot(
     ash::ScreenshotDelegate* screenshot_delegate) {
   ash::PartialScreenshotView::StartPartialScreenshot(screenshot_delegate);
diff --git a/chrome/browser/ui/views/ash/chrome_shell_delegate.h b/chrome/browser/ui/views/ash/chrome_shell_delegate.h
index 514053c..bad11ac 100644
--- a/chrome/browser/ui/views/ash/chrome_shell_delegate.h
+++ b/chrome/browser/ui/views/ash/chrome_shell_delegate.h
@@ -47,8 +47,6 @@ class ChromeShellDelegate : public ash::ShellDelegate,
   virtual void Exit() OVERRIDE;
   virtual void NewWindow(bool is_incognito) OVERRIDE;
   virtual ash::AppListViewDelegate* CreateAppListViewDelegate() OVERRIDE;
-  virtual std::vector<aura::Window*> GetCycleWindowList(
-      CycleSource source) const OVERRIDE;
   virtual void StartPartialScreenshot(
       ash::ScreenshotDelegate* screenshot_delegate) OVERRIDE;
   virtual ash::LauncherDelegate* CreateLauncherDelegate(
diff --git a/chrome/browser/ui/views/ash/window_positioner.cc b/chrome/browser/ui/views/ash/window_positioner.cc
index 22f9197..1639901 100644
--- a/chrome/browser/ui/views/ash/window_positioner.cc
+++ b/chrome/browser/ui/views/ash/window_positioner.cc
@@ -5,7 +5,7 @@
 #include "chrome/browser/ui/views/ash/window_positioner.h"
 
 #include "ash/shell.h"
-#include "ash/shell_delegate.h"
+#include "ash/wm/window_cycle_controller.h"
 #include "ash/wm/window_util.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_delegate.h"
@@ -95,10 +95,8 @@ gfx::Rect WindowPositioner::NormalPopupPosition(
 gfx::Rect WindowPositioner::SmartPopupPosition(
     const gfx::Rect& old_pos,
     const gfx::Rect& work_area) {
-  // We get a list of all windows.
-  const std::vector<aura::Window*>& windows =
-      ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
-          ash::ShellDelegate::SOURCE_KEYBOARD);
+  const std::vector<aura::Window*> windows =
+      ash::WindowCycleController::BuildWindowList();
 
   std::vector<const gfx::Rect*> regions;
   // Process the window list and check if we can bail immediately.
diff --git a/chrome/browser/ui/window_sizer_aura.cc b/chrome/browser/ui/window_sizer_aura.cc
index 7533720..2c0c700 100644
--- a/chrome/browser/ui/window_sizer_aura.cc
+++ b/chrome/browser/ui/window_sizer_aura.cc
@@ -1,11 +1,11 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/window_sizer.h"
 
 #include "ash/shell.h"
-#include "ash/shell_delegate.h"
+#include "ash/wm/window_cycle_controller.h"
 #include "ash/wm/window_util.h"
 #include "base/compiler_specific.h"
 #include "chrome/browser/browser_process.h"
@@ -42,9 +42,8 @@ aura::Window* GetTopWindow() {
     return window;
 
   // Get a list of all windows.
-  const std::vector<aura::Window*>& windows =
-      ash::Shell::GetInstance()->delegate()->GetCycleWindowList(
-          ash::ShellDelegate::SOURCE_KEYBOARD);
+  const std::vector<aura::Window*> windows =
+      ash::WindowCycleController::BuildWindowList();
 
   if (windows.empty())
     return NULL;