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

[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);
 }
 
 void WorkspaceManager2::OnWindowRemovedFromWorkspace(Workspace2* workspace,
                                                      Window* child) {
   if (workspace->ShouldMoveToPending())
     MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_WINDOW_REMOVED);
 }
 
 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_)
     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