ash: Clean up window cycling accelerator code

* Eliminate unused CYCLE_BACKWARD_LINEAR action -- linear cycling only goes
  forward.
* Consolidate the window cycling code into helper functions

BUG=none
TEST=ash_unittests Accelerator*

Review URL: https://chromiumcodereview.appspot.com/23654011

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

ash/accelerators/accelerator_controller.cc

@@ -92,27 +92,49 @@ bool OverviewEnabled() {
       switches::kAshDisableOverviewMode);
 }
 
-bool HandleCycleWindowMRU(WindowCycleController::Direction direction,
-                          bool is_alt_down) {
-  Shell::GetInstance()->
-      window_cycle_controller()->HandleCycleWindow(direction, is_alt_down);
-  // Always report we handled the key, even if the window didn't change.
-  return true;
-}
+void HandleCycleBackwardMRU(const ui::Accelerator& accelerator) {
+  Shell* shell = Shell::GetInstance();
 
-bool HandleCycleWindowOverviewMRU(WindowSelector::Direction direction) {
-  Shell::GetInstance()->
-      window_selector_controller()->HandleCycleWindow(direction);
-  return true;
+  if (accelerator.key_code() == ui::VKEY_TAB)
+    shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_PREVWINDOW_TAB);
+
+  if (OverviewEnabled()) {
+    shell->window_selector_controller()->HandleCycleWindow(
+        WindowSelector::BACKWARD);
+    return;
+  }
+  shell->window_cycle_controller()->HandleCycleWindow(
+      WindowCycleController::BACKWARD, accelerator.IsAltDown());
 }
 
-void HandleCycleWindowLinear(CycleDirection direction) {
-  Shell::GetInstance()->
-      window_cycle_controller()->HandleLinearCycleWindow();
+void HandleCycleForwardMRU(const ui::Accelerator& accelerator) {
+  Shell* shell = Shell::GetInstance();
+
+  if (accelerator.key_code() == ui::VKEY_TAB)
+    shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_NEXTWINDOW_TAB);
+
+  if (OverviewEnabled()) {
+    shell->window_selector_controller()->HandleCycleWindow(
+        WindowSelector::FORWARD);
+    return;
+  }
+  shell->window_cycle_controller()->HandleCycleWindow(
+      WindowCycleController::FORWARD, accelerator.IsAltDown());
 }
 
-void ToggleOverviewMode() {
-  Shell::GetInstance()->window_selector_controller()->ToggleOverview();
+void HandleCycleLinear(const ui::Accelerator& accelerator) {
+  Shell* shell = Shell::GetInstance();
+
+  // TODO(jamescook): When overview becomes the default the AcceleratorAction
+  // should be renamed from CYCLE_LINEAR to TOGGLE_OVERVIEW.
+  if (OverviewEnabled()) {
+    shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_OVERVIEW_F5);
+    shell->window_selector_controller()->ToggleOverview();
+    return;
+  }
+  if (accelerator.key_code() == ui::VKEY_MEDIA_LAUNCH_APP1)
+    shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_NEXTWINDOW_F5);
+  shell->window_cycle_controller()->HandleLinearCycleWindow();
 }
 
 bool HandleAccessibleFocusCycle(bool reverse) {
@@ -522,44 +544,23 @@ bool AcceleratorController::PerformAction(int action,
   // You *MUST* return true when some action is performed. Otherwise, this
   // function might be called *twice*, via BrowserView::PreHandleKeyboardEvent
   // and BrowserView::HandleKeyboardEvent, for a single accelerator press.
+  //
+  // If your accelerator invokes more than one line of code, please either
+  // implement it in your module's controller code (like TOGGLE_MIRROR_MODE
+  // below) or pull it into a HandleFoo() function above.
   switch (action) {
     case ACCESSIBLE_FOCUS_NEXT:
       return HandleAccessibleFocusCycle(false);
     case ACCESSIBLE_FOCUS_PREVIOUS:
       return HandleAccessibleFocusCycle(true);
     case CYCLE_BACKWARD_MRU:
-      if (key_code == ui::VKEY_TAB)
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_PREVWINDOW_TAB);
-      if (OverviewEnabled())
-        return HandleCycleWindowOverviewMRU(WindowSelector::BACKWARD);
-      return HandleCycleWindowMRU(WindowCycleController::BACKWARD,
-                                  accelerator.IsAltDown());
+      HandleCycleBackwardMRU(accelerator);
+      return true;
     case CYCLE_FORWARD_MRU:
-      if (key_code == ui::VKEY_TAB)
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_NEXTWINDOW_TAB);
-      if (OverviewEnabled())
-        return HandleCycleWindowOverviewMRU(WindowSelector::FORWARD);
-      return HandleCycleWindowMRU(WindowCycleController::FORWARD,
-                                  accelerator.IsAltDown());
-    case CYCLE_BACKWARD_LINEAR:
-      if (OverviewEnabled()) {
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_OVERVIEW_F5);
-        ToggleOverviewMode();
-        return true;
-      }
-      if (key_code == ui::VKEY_MEDIA_LAUNCH_APP1)
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_PREVWINDOW_F5);
-      HandleCycleWindowLinear(CYCLE_BACKWARD);
-      return true;
-    case CYCLE_FORWARD_LINEAR:
-      if (OverviewEnabled()) {
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_OVERVIEW_F5);
-        ToggleOverviewMode();
-        return true;
-      }
-      if (key_code == ui::VKEY_MEDIA_LAUNCH_APP1)
-        shell->delegate()->RecordUserMetricsAction(UMA_ACCEL_NEXTWINDOW_F5);
-      HandleCycleWindowLinear(CYCLE_FORWARD);
+      HandleCycleForwardMRU(accelerator);
+      return true;
+    case CYCLE_LINEAR:
+      HandleCycleLinear(accelerator);
       return true;
 #if defined(OS_CHROMEOS)
     case ADD_REMOVE_DISPLAY:
@@ -717,19 +718,15 @@ bool AcceleratorController::PerformAction(int action,
     case VOLUME_MUTE:
       return shell->system_tray_delegate()->GetVolumeControlDelegate()->
           HandleVolumeMute(accelerator);
-      break;
     case VOLUME_DOWN:
       return shell->system_tray_delegate()->GetVolumeControlDelegate()->
           HandleVolumeDown(accelerator);
-      break;
     case VOLUME_UP:
       return shell->system_tray_delegate()->GetVolumeControlDelegate()->
           HandleVolumeUp(accelerator);
-      break;
     case FOCUS_LAUNCHER:
       return shell->focus_cycler()->FocusWidget(
           Launcher::ForPrimaryDisplay()->shelf_widget());
-      break;
     case FOCUS_NEXT_PANE:
       return HandleRotatePaneFocus(Shell::FORWARD);
     case FOCUS_PREVIOUS_PANE:

ash/accelerators/accelerator_controller_unittest.cc

@@ -668,14 +668,11 @@ TEST_F(AcceleratorControllerTest, GlobalAccelerators) {
   // CycleForward
   EXPECT_TRUE(ProcessWithContext(
       ui::Accelerator(ui::VKEY_TAB, ui::EF_ALT_DOWN)));
-#if defined(OS_CHROMEOS)
-  // CycleBackward
-  EXPECT_TRUE(ProcessWithContext(
-      ui::Accelerator(ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_SHIFT_DOWN)));
-  // CycleForward
+  // CycleLinear
   EXPECT_TRUE(ProcessWithContext(
       ui::Accelerator(ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_NONE)));
 
+#if defined(OS_CHROMEOS)
   // Take screenshot / partial screenshot
   // True should always be returned regardless of the existence of the delegate.
   {

ash/accelerators/accelerator_table.cc

@@ -37,8 +37,7 @@ const AcceleratorData kAcceleratorData[] = {
   { true, ui::VKEY_TAB, ui::EF_ALT_DOWN, CYCLE_FORWARD_MRU },
   { true, ui::VKEY_TAB, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN,
     CYCLE_BACKWARD_MRU },
-  { true, ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_NONE,
-      CYCLE_FORWARD_LINEAR },
+  { true, ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_NONE, CYCLE_LINEAR },
 #if defined(OS_CHROMEOS)
   { true, ui::VKEY_BROWSER_SEARCH, ui::EF_NONE, TOGGLE_APP_LIST },
   { true, ui::VKEY_WLAN, ui::EF_NONE, TOGGLE_WIFI },
@@ -103,8 +102,6 @@ const AcceleratorData kAcceleratorData[] = {
   { true, ui::VKEY_BROWSER_REFRESH,
     ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     ROTATE_WINDOW },
-  { true, ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_SHIFT_DOWN,
-    CYCLE_BACKWARD_LINEAR },
   { true, ui::VKEY_T, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, RESTORE_TAB },
   { true, ui::VKEY_PRINT, ui::EF_NONE, TAKE_SCREENSHOT },
   // On Chrome OS, Search key is mapped to LWIN. The Search key binding should
@@ -343,10 +340,9 @@ const size_t kActionsAllowedAtModalWindowLength =
 
 const AcceleratorAction kNonrepeatableActions[] = {
   // TODO(mazda): Add other actions which should not be repeated.
-  CYCLE_BACKWARD_LINEAR,
   CYCLE_BACKWARD_MRU,
-  CYCLE_FORWARD_LINEAR,
   CYCLE_FORWARD_MRU,
+  CYCLE_LINEAR,
   EXIT,
   PRINT_UI_HIERARCHIES,  // Don't fill the logs if the key is held down.
   ROTATE_SCREEN,
@@ -365,10 +361,9 @@ const size_t kNonrepeatableActionsLength =
 const AcceleratorAction kActionsAllowedInAppMode[] = {
   BRIGHTNESS_DOWN,
   BRIGHTNESS_UP,
-  CYCLE_BACKWARD_LINEAR,
   CYCLE_BACKWARD_MRU,
-  CYCLE_FORWARD_LINEAR,
   CYCLE_FORWARD_MRU,
+  CYCLE_LINEAR,
   DISABLE_CAPS_LOCK,
   EXIT,
   KEYBOARD_BRIGHTNESS_DOWN,

ash/accelerators/accelerator_table.h

@@ -45,10 +45,9 @@ enum AcceleratorAction {
   ACCESSIBLE_FOCUS_PREVIOUS,
   BRIGHTNESS_DOWN,
   BRIGHTNESS_UP,
-  CYCLE_BACKWARD_LINEAR,
   CYCLE_BACKWARD_MRU,
-  CYCLE_FORWARD_LINEAR,
   CYCLE_FORWARD_MRU,
+  CYCLE_LINEAR,
   DEBUG_TOGGLE_DEVICE_SCALE_FACTOR,
   DEBUG_TOGGLE_SHOW_DEBUG_BORDERS,
   DEBUG_TOGGLE_SHOW_FPS_COUNTER,
@@ -143,69 +142,49 @@ struct AcceleratorData {
 
 // Accelerators handled by AcceleratorController.
 ASH_EXPORT extern const AcceleratorData kAcceleratorData[];
-
-// The number of elements in kAcceleratorData.
 ASH_EXPORT extern const size_t kAcceleratorDataLength;
 
 #if !defined(NDEBUG)
 // Accelerators useful when running on desktop. Debug build only.
 ASH_EXPORT extern const AcceleratorData kDesktopAcceleratorData[];
-
-// The number of elements in kDesktopAcceleratorData.
 ASH_EXPORT extern const size_t kDesktopAcceleratorDataLength;
 #endif
 
 // Debug accelerators enabled only when "Debugging keyboard shortcuts" flag
 // (--ash-debug-shortcuts) is enabled.
 ASH_EXPORT extern const AcceleratorData kDebugAcceleratorData[];
-
-// The number of elements in kDebugAcceleratorData.
 ASH_EXPORT extern const size_t kDebugAcceleratorDataLength;
 
 // Actions that should be handled very early in Ash unless the current target
 // window is full-screen.
 ASH_EXPORT extern const AcceleratorAction kReservedActions[];
-
-// The number of elements in kReservedActions.
 ASH_EXPORT extern const size_t kReservedActionsLength;
 
 // Actions that should be handled very early in Ash unless the current target
 // window is full-screen, these actions are only handled if
 // DebugShortcutsEnabled is true (command line switch 'ash-debug-shortcuts').
 ASH_EXPORT extern const AcceleratorAction kReservedDebugActions[];
-
-// The number of elements in kReservedDebugActions.
 ASH_EXPORT extern const size_t kReservedDebugActionsLength;
 
 // Actions allowed while user is not signed in or screen is locked.
 ASH_EXPORT extern const AcceleratorAction kActionsAllowedAtLoginOrLockScreen[];
-
-// The number of elements in kActionsAllowedAtLoginOrLockScreen.
 ASH_EXPORT extern const size_t kActionsAllowedAtLoginOrLockScreenLength;
 
 // Actions allowed while screen is locked (in addition to
 // kActionsAllowedAtLoginOrLockScreen).
 ASH_EXPORT extern const AcceleratorAction kActionsAllowedAtLockScreen[];
-
-// The number of elements in kActionsAllowedAtLockScreen.
 ASH_EXPORT extern const size_t kActionsAllowedAtLockScreenLength;
 
 // Actions allowed while a modal window is up.
 ASH_EXPORT extern const AcceleratorAction kActionsAllowedAtModalWindow[];
-
-// The number of elements in kActionsAllowedAtModalWindow.
 ASH_EXPORT extern const size_t kActionsAllowedAtModalWindowLength;
 
 // Actions which will not be repeated while holding an accelerator key.
 ASH_EXPORT extern const AcceleratorAction kNonrepeatableActions[];
-
-// The number of elements in kNonrepeatableActions.
 ASH_EXPORT extern const size_t kNonrepeatableActionsLength;
 
 // Actions allowed in app mode.
 ASH_EXPORT extern const AcceleratorAction kActionsAllowedInAppMode[];
-
-// The number of elements in kActionsAllowedInAppMode.
 ASH_EXPORT extern const size_t kActionsAllowedInAppModeLength;
 
 }  // namespace ash