Improving window auto management between workspaces

ash/wm/workspace/workspace_manager2.cc

Index: ash/wm/workspace/workspace_manager2.cc
diff --git a/ash/wm/workspace/workspace_manager2.cc b/ash/wm/workspace/workspace_manager2.cc
index 148ca71485c3f19caf020f2681b22bc3c4e8a4e7..fe59eca06e95bfec4f6eae99130b776ba27707af 100644
--- a/ash/wm/workspace/workspace_manager2.cc
+++ b/ash/wm/workspace/workspace_manager2.cc
@@ -17,6 +17,7 @@
 #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"
@@ -597,10 +598,13 @@ void WorkspaceManager2::OnWindowAddedToWorkspace(Workspace2* workspace,
   // to the workspace.
   if (workspace == active_workspace_)
     UpdateShelfVisibility();
+
+  RearrangeVisibleWindowOnShow(child);
 }
 
 void WorkspaceManager2::OnWillRemoveWindowFromWorkspace(Workspace2* workspace,
                                                         Window* child) {
+  RearrangeVisibleWindowOnHide(child);
   child->ClearProperty(kWorkspaceKey);
 }
 
@@ -613,6 +617,12 @@ void WorkspaceManager2::OnWindowRemovedFromWorkspace(Workspace2* workspace,
 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_)