Improving window auto management between workspaces

ash/wm/workspace/workspace_manager2.cc

 // 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 "ash/wm/workspace/workspace_manager2.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "ash/ash_switches.h"
 #include "ash/root_window_controller.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "ash/wm/base_layout_manager.h"
 #include "ash/wm/property_util.h"
 #include "ash/wm/shelf_layout_manager.h"
 #include "ash/wm/window_animations.h"
 #include "ash/wm/window_properties.h"
 #include "ash/wm/window_util.h"
+#include "ash/wm/workspace/auto_window_management.h"
 #include "ash/wm/workspace/desktop_background_fade_controller.h"
 #include "ash/wm/workspace/workspace_animations.h"
 #include "ash/wm/workspace/workspace_layout_manager2.h"
 #include "ash/wm/workspace/workspace2.h"
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/root_window.h"
@@ skipping 560 matching lines @@
 }
 
 void WorkspaceManager2::OnWindowAddedToWorkspace(Workspace2* workspace,
                                                  Window* child) {
   child->SetProperty(kWorkspaceKey, workspace);
   // Do nothing (other than updating shelf visibility) as the right parent was
   // chosen by way of GetParentForNewWindow() or we explicitly moved the window
   // to the workspace.
   if (workspace == active_workspace_)
     UpdateShelfVisibility();
+
+  RearrangeVisibleWindowOnShow(child);
 }
 
 void WorkspaceManager2::OnWillRemoveWindowFromWorkspace(Workspace2* workspace,
                                                         Window* child) {
+  RearrangeVisibleWindowOnHide(child);
   child->ClearProperty(kWorkspaceKey);
 }
 
 void WorkspaceManager2::OnWindowRemovedFromWorkspace(Workspace2* workspace,
                                                      Window* child) {
   if (workspace->ShouldMoveToPending())
     MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_WINDOW_REMOVED);
 }
 
 void WorkspaceManager2::OnWorkspaceChildWindowVisibilityChanged(
     Workspace2* workspace,
     Window* child) {
+
+  if (child->IsVisible())

[sky, 2012/10/17 15:59:38]

I don't get why this needs to be before the ShouldMoveToPending call. You
previous said this mattered, but I don't see that. Your tests work fine if this
looks like:

  if (workspace->ShouldMoveToPending()) {
    MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_VISIBILITY_CHANGED);
  } else {
    if (child->TargetVisibility())
      RearrangeVisibleWindowOnShow(child);
    else
      RearrangeVisibleWindowOnHide(child);
    if (workspace == active_workspace_)
      UpdateShelfVisibility();
  }

[Mr4D (OOO till 08-26), 2012/10/17 18:43:56]

I am tired arguing. You get it the way and I fix the bugs as they come in. Done.

[sky, 2012/10/18 16:11:19]

The reason it should be like this is that if ShouldMoveToPending() returns true
then there should be no visible windows in the workspace. Additionally the
workspace may be deleted shortly. In other words, doing anything if
ShouldMoveToPending() is true is unnecessary.

If you keep the code above ShouldMoveToPending() it begs the question why? Why
would we need to do something to a workspace that is no longer interesting and
may be deleted.

+    RearrangeVisibleWindowOnShow(child);
+  else
+    RearrangeVisibleWindowOnHide(child);
+
   if (workspace->ShouldMoveToPending())
     MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_VISIBILITY_CHANGED);
   else if (workspace == active_workspace_)
     UpdateShelfVisibility();
 }
 
 void WorkspaceManager2::OnWorkspaceWindowChildBoundsChanged(
     Workspace2* workspace,
     Window* child) {
   if (workspace == active_workspace_)
@@ skipping 90 matching lines @@
     new_workspace->window()->Show();
   ReparentWindow(window, new_workspace->window(), NULL);
   if (is_active) {
     SetActiveWorkspace(new_workspace, SWITCH_TRACKED_BY_WORKSPACE_CHANGED,
                        base::TimeDelta());
   }
 }
 
 }  // namespace internal
 }  // namespace ash