Refactor SetInOverview

athena/wm/window_manager_impl.cc

Index: athena/wm/window_manager_impl.cc
diff --git a/athena/wm/window_manager_impl.cc b/athena/wm/window_manager_impl.cc
index 35722c1adfa69f27a7743b3c6003d85468c88051..8d972f59d3cdd67b8130846fab692c5fccd521d7 100644
--- a/athena/wm/window_manager_impl.cc
+++ b/athena/wm/window_manager_impl.cc
@@ -193,23 +193,36 @@ void WindowManagerImpl::ToggleSplitView() {
   }
 }
 
-void WindowManagerImpl::ToggleOverview() {
-  if (IsOverviewModeActive()) {
-    SetInOverview(false);
-
-    // Activate the window which was active prior to entering overview.
-    const aura::Window::Windows windows =
-        window_list_provider_->GetWindowList();
-    if (!windows.empty()) {
-      aura::Window* window = windows.back();
-      // Show the window in case the exit overview animation has finished and
-      // |window| was hidden.
-      window->Show();
-
-      wm::ActivateWindow(window);
-    }
-  } else {
-    SetInOverview(true);
+void WindowManagerImpl::EnterOverview() {
+  if (IsOverviewModeActive())
+    return;
+
+  bezel_controller_->set_left_right_delegate(NULL);
+  FOR_EACH_OBSERVER(WindowManagerObserver, observers_, OnOverviewModeEnter());
+
+  // Note: The window_list_provider_ resembles the exact window list of the
+  // container, so no re-stacking is required before showing the OverviewMode.
+  overview_ = WindowOverviewMode::Create(
+      container_.get(), window_list_provider_.get(),
+      split_view_controller_.get(), this);
+}
+
+// Will reactivate the previously active window

[pkotwicz, 2014/10/09 21:13:59]

Nit: This comment should be in the window_manager.h
How about: "Exits overview and activates the previously active activity."

[Greg Levin, 2014/10/10 21:13:45]

Done.

+void WindowManagerImpl::ExitOverview() {
+  if (!IsOverviewModeActive())
+    return;
+
+  ExitOverviewNoActivate();
+
+  // Activate the window which was active prior to entering overview.
+  const aura::Window::Windows windows = window_list_provider_->GetWindowList();
+  if (!windows.empty()) {
+    aura::Window* window_to_activate = windows.back();
+
+    // Show the window in case the exit overview animation has finished and
+    // |window| was hidden.
+    window_to_activate->Show();
+    wm::ActivateWindow(window_to_activate);
   }
 }
 
@@ -217,25 +230,14 @@ bool WindowManagerImpl::IsOverviewModeActive() {
   return overview_;
 }
 
-void WindowManagerImpl::SetInOverview(bool active) {
-  bool in_overview = !!overview_;
-  if (active == in_overview)
+// Only handles overview mode, will not actiavte any windows

[pkotwicz, 2014/10/09 21:13:59]

Nit: The comment should be in the .h file
For the comment, how about: "Exits overview mode without changing activation.
The caller should ensure that a window is active after exiting overview mode."

[Greg Levin, 2014/10/10 21:13:45]

Done.

+void WindowManagerImpl::ExitOverviewNoActivate() {
+  if (!IsOverviewModeActive())
     return;
 
-  bezel_controller_->set_left_right_delegate(
-      active ? NULL : split_view_controller_.get());
-  if (active) {
-    FOR_EACH_OBSERVER(WindowManagerObserver, observers_, OnOverviewModeEnter());
-
-    // Note: The window_list_provider_ resembles the exact window list of the
-    // container, so no re-stacking is required before showing the OverviewMode.
-    overview_ = WindowOverviewMode::Create(
-        container_.get(), window_list_provider_.get(),
-        split_view_controller_.get(), this);
-  } else {
-    overview_.reset();
-    FOR_EACH_OBSERVER(WindowManagerObserver, observers_, OnOverviewModeExit());
-  }
+  bezel_controller_->set_left_right_delegate(split_view_controller_.get());
+  overview_.reset();
+  FOR_EACH_OBSERVER(WindowManagerObserver, observers_, OnOverviewModeExit());
 }
 
 void WindowManagerImpl::InstallAccelerators() {
@@ -266,12 +268,11 @@ WindowListProvider* WindowManagerImpl::GetWindowListProvider() {
 }
 
 void WindowManagerImpl::OnSelectWindow(aura::Window* window) {
-  SetInOverview(false);
+  ExitOverviewNoActivate();
 
   // Show the window in case the exit overview animation has finished and
   // |window| was hidden.
   window->Show();
-

[pkotwicz, 2014/10/09 21:13:59]

Nit: Keep the white space.

In this case, it is ambiguous whether removing the whitespace is a positive
change. Given that you are not modifying the code around the whitespace it is
preferable not to modify the whitespace in this CL.

[Greg Levin, 2014/10/10 21:13:45]

Done.

   wm::ActivateWindow(window);
 
   if (split_view_controller_->IsSplitViewModeActive()) {
@@ -305,7 +306,7 @@ void WindowManagerImpl::OnSelectWindow(aura::Window* window) {
 void WindowManagerImpl::OnSelectSplitViewWindow(aura::Window* left,
                                                 aura::Window* right,
                                                 aura::Window* to_activate) {
-  SetInOverview(false);
+  ExitOverviewNoActivate();
   FOR_EACH_OBSERVER(WindowManagerObserver, observers_, OnSplitViewModeEnter());
   split_view_controller_->ActivateSplitMode(left, right, to_activate);
 }
@@ -323,7 +324,10 @@ bool WindowManagerImpl::OnAcceleratorFired(int command_id,
                                            const ui::Accelerator& accelerator) {
   switch (command_id) {
     case CMD_TOGGLE_OVERVIEW:
-      ToggleOverview();
+      if (IsOverviewModeActive())
+        ExitOverview();
+      else
+        EnterOverview();
       break;
     case CMD_TOGGLE_SPLIT_VIEW:
       ToggleSplitView();