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);

[sky, 2012/10/16 17:25:34]

Don't you only want to do any of the calls to auto-management if the window is
TargetVisibilty()? Alternatively the check could be in the functions.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

That is handled by the RearrangeVisible... functions already. I don't see a
reason why we should add that here to the code as well.

 }
 
 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) {
+

[sky, 2012/10/16 17:25:34]

Move this to 630ish.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

No, we want to do that BEFORE we move the window between workspaces.

[sky, 2012/10/16 22:01:14]

This doesn't move the window between workspaces, but it may move others. Are you
sure you don't need to do it on both sides? At a minimum remove newline @ 620
and add better comments.

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

Done.

The auto management code takes all associated workspace windows into account and
positions the others... (and or this one - dependent on the call). For the
question of: "Don't we need that on both sides" - I thought the same, but I was
not able to come up with a single test case where it went wrong. If we ever get
there I am gladly adding it. :)

+  if (child->IsVisible())

[sky, 2012/10/16 22:01:14]

Are you sure you don't want TargetVisibilty here?

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

Okay - I guess we should probably do something about the name "IsVisible" since
it is misleading.. Done.

+    RearrangeVisibleWindowOnShow(child);
+  else
+    RearrangeVisibleWindowOnHide(child);
+
   if (workspace->ShouldMoveToPending())
     MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_VISIBILITY_CHANGED);
   else if (workspace == active_workspace_)