[ash] Don't remove an item when associated window is dragged

LauncherItem adding/removing is repeated when a Window is dragged because
it is reparented to DockedContainer during the dragging.
ShelfWindowWatcher should prevent this removing when window is dragging.

R=sky@chromium.org
BUG=NONE
TEST=ash_unittests --gtest_filter=ShelfWindowWatcherTest*.*

Review URL: https://codereview.chromium.org/108313006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243970 0039d316-1c4b-4281-b951-d872f2087c98

ash/shelf/shelf_window_watcher.cc

@@ -4,6 +4,7 @@
 
 #include "ash/shelf/shelf_window_watcher.h"
 
+#include "ash/ash_switches.h"
 #include "ash/display/display_controller.h"
 #include "ash/shelf/shelf_constants.h"
 #include "ash/shelf/shelf_item_delegate_manager.h"
@@ -12,6 +13,7 @@
 #include "ash/shelf/shelf_window_watcher_item_delegate.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
+#include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/aura/client/activation_client.h"
@@ -41,6 +43,11 @@ bool HasLauncherItemForWindow(aura::Window* window) {
   return false;
 }
 
+// Returns true if |window| is in the process of being dragged.
+bool IsDragging(aura::Window* window) {
+  return ash::wm::GetWindowState(window)->is_dragged();
+}
+
 }  // namespace
 
 namespace ash {
@@ -59,14 +66,55 @@ void ShelfWindowWatcher::RootWindowObserver::OnWindowDestroying(
   window_watcher_->OnRootWindowRemoved(window);
 }
 
+ShelfWindowWatcher::RemovedWindowObserver::RemovedWindowObserver(
+    ShelfWindowWatcher* window_watcher)
+    : window_watcher_(window_watcher) {
+}
+
+ShelfWindowWatcher::RemovedWindowObserver::~RemovedWindowObserver() {
+}
+
+void ShelfWindowWatcher::RemovedWindowObserver::OnWindowParentChanged(
+    aura::Window* window,
+    aura::Window* parent) {
+  // When |parent| is NULL, this |window| will be destroyed. In that case, its
+  // item will be removed at OnWindowDestroyed().
+  if (!parent)
+    return;
+
+  // When |parent| is changed from default container to docked container
+  // during the dragging, |window|'s item should not be removed because it will
+  // be re-parented to default container again after finishing the dragging.
+  // We don't need to check |parent| is default container because this observer
+  // is already removed from |window| when |window| is re-parented to default
+  // container.
+  if (switches::UseDockedWindows() &&
+      IsDragging(window) &&
+      parent->id() == kShellWindowId_DockedContainer)
+    return;
+
+  // When |window| is re-parented to other containers or |window| is re-parented
+  // not to |docked_container| during the dragging, its item should be removed
+  // and stop observing this |window|.
+  window_watcher_->FinishObservingRemovedWindow(window);
+}
+
+void ShelfWindowWatcher::RemovedWindowObserver::OnWindowDestroyed(
+    aura::Window* window) {
+  DCHECK(HasLauncherItemForWindow(window));
+  window_watcher_->FinishObservingRemovedWindow(window);
+}
+
 ShelfWindowWatcher::ShelfWindowWatcher(
     ShelfModel* model,
     ShelfItemDelegateManager* item_delegate_manager)
     : model_(model),
       item_delegate_manager_(item_delegate_manager),
       root_window_observer_(this),
+      removed_window_observer_(this),
       observed_windows_(this),
       observed_root_windows_(&root_window_observer_),
+      observed_removed_windows_(&removed_window_observer_),
       observed_activation_clients_(this) {
   // We can't assume all RootWindows have the same ActivationClient.
   // Add a RootWindow and its ActivationClient to the observed list.
@@ -87,7 +135,7 @@ void ShelfWindowWatcher::AddLauncherItem(aura::Window* window) {
       GetLauncherItemDetailsForWindow(window);
   LauncherItem item;
   LauncherID id = model_->next_id();
-  item.status = ash::wm::IsActiveWindow(window) ? STATUS_ACTIVE: STATUS_RUNNING;
+  item.status = wm::IsActiveWindow(window) ? STATUS_ACTIVE: STATUS_RUNNING;
   SetShelfItemDetailsForLauncherItem(&item, *item_details);
   SetLauncherIDForWindow(id, window);
   scoped_ptr<ShelfItemDelegate> item_delegate(
@@ -138,6 +186,15 @@ int ShelfWindowWatcher::GetLauncherItemIndexForWindow(
   return model_->ItemIndexByID(GetLauncherIDForWindow(window));
 }
 
+void ShelfWindowWatcher::StartObservingRemovedWindow(aura::Window* window) {
+  observed_removed_windows_.Add(window);
+}
+
+void ShelfWindowWatcher::FinishObservingRemovedWindow(aura::Window* window) {
+  observed_removed_windows_.Remove(window);
+  RemoveLauncherItem(window);
+}
+
 void ShelfWindowWatcher::OnWindowActivated(aura::Window* gained_active,
                                            aura::Window* lost_active) {
   if (gained_active && HasLauncherItemForWindow(gained_active))
@@ -148,21 +205,33 @@ void ShelfWindowWatcher::OnWindowActivated(aura::Window* gained_active,
 
 void ShelfWindowWatcher::OnWindowAdded(aura::Window* window) {
   observed_windows_.Add(window);
+
+  if (observed_removed_windows_.IsObserving(window)) {
+    // When |window| is added and it is already observed by
+    // |dragged_window_observer_|, |window| already has its item.
+    DCHECK(HasLauncherItemForWindow(window));
+    observed_removed_windows_.Remove(window);
+    return;
+  }
+
   // Add LauncherItem if |window| already has a LauncherItemDetails when it is
   // created. Don't make a new LauncherItem for the re-parented |window| that
   // already has a LauncherItem.
-  if (GetLauncherIDForWindow(window) == ash::kInvalidShelfID &&
+  if (GetLauncherIDForWindow(window) == kInvalidShelfID &&
       GetLauncherItemDetailsForWindow(window))
     AddLauncherItem(window);
 }
 
 void ShelfWindowWatcher::OnWillRemoveWindow(aura::Window* window) {
-  // Remove a child window of default container and its item if it has.
+  // Remove a child window of default container.
   if (observed_windows_.IsObserving(window))
     observed_windows_.Remove(window);
 
+  // Don't remove |window| item immediately. Instead, defer handling of removing
+  // |window|'s item to RemovedWindowObserver because |window| could be added
+  // again to default container.
   if (HasLauncherItemForWindow(window))
-    RemoveLauncherItem(window);
+    StartObservingRemovedWindow(window);
 }
 
 void ShelfWindowWatcher::OnWindowDestroying(aura::Window* window) {

ash/shelf/shelf_window_watcher.h

@@ -56,6 +56,25 @@ class ShelfWindowWatcher : public aura::client::ActivationChangeObserver,
     DISALLOW_COPY_AND_ASSIGN(RootWindowObserver);
   };
 
+  // Used to track windows that are removed. See description of
+  // ShelfWindowWatcher::StartObservingRemovedWindow() for more details.
+  class RemovedWindowObserver : public aura::WindowObserver {
+   public:
+    explicit RemovedWindowObserver(ShelfWindowWatcher* window_watcher);
+    virtual ~RemovedWindowObserver();
+
+   private:
+    // aura::WindowObserver overrides:
+    virtual void OnWindowParentChanged(aura::Window* window,
+                                       aura::Window* parent) OVERRIDE;
+    virtual void OnWindowDestroyed(aura::Window* window) OVERRIDE;
+
+    // Owned by Shell.
+    ShelfWindowWatcher* window_watcher_;
+
+    DISALLOW_COPY_AND_ASSIGN(RemovedWindowObserver);
+  };
+
   // Creates a LauncherItem for |window| that has LauncherItemDetails.
   void AddLauncherItem(aura::Window* window);
 
@@ -74,6 +93,16 @@ class ShelfWindowWatcher : public aura::client::ActivationChangeObserver,
   // Returns the index of LauncherItem associated with |window|.
   int GetLauncherItemIndexForWindow(aura::Window* window) const;
 
+  // Used when a window is removed. During the dragging a window may be removed
+  // and when the drag completes added back. When this happens we don't want to
+  // remove the shelf item. StartObservingRemovedWindow, if necessary, attaches
+  // an observer. When done, FinishObservingRemovedWindow() is invoked.
+  void StartObservingRemovedWindow(aura::Window* window);
+
+  // Stop observing |window| by RemovedWindowObserver and remove an item
+  // associated with |window|.
+  void FinishObservingRemovedWindow(aura::Window* window);
+
   // aura::client::ActivationChangeObserver overrides:
   virtual void OnWindowActivated(aura::Window* gained_active,
                                  aura::Window* lost_active) OVERRIDE;
@@ -97,12 +126,17 @@ class ShelfWindowWatcher : public aura::client::ActivationChangeObserver,
 
   RootWindowObserver root_window_observer_;
 
+  RemovedWindowObserver removed_window_observer_;
+
   // Holds all observed windows.
   ScopedObserver<aura::Window, aura::WindowObserver> observed_windows_;
 
   // Holds all observed root windows.
   ScopedObserver<aura::Window, aura::WindowObserver> observed_root_windows_;
 
+  // Holds removed windows that has an item from default container.
+  ScopedObserver<aura::Window, aura::WindowObserver> observed_removed_windows_;
+
   // Holds all observed activation clients.
   ScopedObserverWithDuplicatedSources<aura::client::ActivationClient,
       aura::client::ActivationChangeObserver> observed_activation_clients_;

ash/shelf/shelf_window_watcher_unittest.cc

@@ -4,16 +4,21 @@
 
 #include "ash/shelf/shelf_window_watcher.h"
 
+#include "ash/ash_switches.h"
 #include "ash/launcher/launcher_types.h"
 #include "ash/shelf/shelf_model.h"
 #include "ash/shelf/shelf_util.h"
 #include "ash/shell.h"
+#include "ash/shell_window_ids.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/shell_test_api.h"
+#include "ash/wm/window_resizer.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
+#include "base/command_line.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
+#include "ui/base/hit_test.h"
 
 namespace ash {
 namespace internal {
@@ -33,17 +38,14 @@ class ShelfWindowWatcherTest : public test::AshTestBase {
     test::AshTestBase::TearDown();
   }
 
-  ash::LauncherID CreateShelfItem(aura::Window* window) {
+  LauncherID CreateShelfItem(aura::Window* window) {
     LauncherID id = model_->next_id();
-    ash::LauncherItemDetails item_details;
+    LauncherItemDetails item_details;
     item_details.type = TYPE_PLATFORM_APP;
     SetShelfItemDetailsForWindow(window, item_details);
     return id;
   }
 
-  void UpdateLauncherItem(aura::Window* window) {
-  }
-
  protected:
   ShelfModel* model_;
 
@@ -166,10 +168,123 @@ TEST_F(ShelfWindowWatcherTest, MaximizeAndRestoreWindow) {
   EXPECT_FALSE(window_state->IsMaximized());
   // No new item is created after restoring a window |window|.
   EXPECT_EQ(2, model_->item_count());
-  // index and id are not changed after maximizing a window |window|.
+  // Index and id are not changed after maximizing a window |window|.
   EXPECT_EQ(index, model_->ItemIndexByID(id));
   EXPECT_EQ(id, model_->items()[index].id);
 }
 
+// Check that an item is removed when its associated Window is re-parented.
+TEST_F(ShelfWindowWatcherTest, ReparentWindow) {
+  // ShelfModel only has an APP_LIST item.
+  EXPECT_EQ(1, model_->item_count());
+
+  scoped_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
+  window->set_owned_by_parent(false);
+
+  // Create a LauncherItem for |window|.
+  LauncherID id = CreateShelfItem(window.get());
+  EXPECT_EQ(2, model_->item_count());
+
+  int index = model_->ItemIndexByID(id);
+  EXPECT_EQ(STATUS_RUNNING, model_->items()[index].status);
+
+  aura::Window* root_window = window->GetRootWindow();
+  aura::Window* default_container = Shell::GetContainer(
+      root_window,
+      kShellWindowId_DefaultContainer);
+  EXPECT_EQ(default_container, window->parent());
+
+  aura::Window* new_parent = Shell::GetContainer(
+      root_window,
+      kShellWindowId_PanelContainer);
+
+  // Check |window|'s item is removed when it is re-parented to |new_parent|
+  // which is not default container.
+  new_parent->AddChild(window.get());
+  EXPECT_EQ(1, model_->item_count());
+
+  // Check |window|'s item is added when it is re-parented to
+  // |default_container|.
+  default_container->AddChild(window.get());
+  EXPECT_EQ(2, model_->item_count());
+}
+
+// Check |window|'s item is not changed during the dragging.
+// TODO(simonhong): Add a test for removing a Window during the dragging.
+TEST_F(ShelfWindowWatcherTest, DragWindow) {
+  // ShelfModel only has an APP_LIST item.
+  EXPECT_EQ(1, model_->item_count());
+
+  scoped_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
+
+  // Create a LauncherItem for |window|.
+  LauncherID id = CreateShelfItem(window.get());
+  EXPECT_EQ(2, model_->item_count());
+
+  int index = model_->ItemIndexByID(id);
+  EXPECT_EQ(STATUS_RUNNING, model_->items()[index].status);
+
+  // Simulate dragging of |window| and check its item is not changed.
+  scoped_ptr<WindowResizer> resizer(
+      CreateWindowResizer(window.get(),
+                          gfx::Point(),
+                          HTCAPTION,
+                          aura::client::WINDOW_MOVE_SOURCE_MOUSE));
+  ASSERT_TRUE(resizer.get());
+  resizer->Drag(gfx::Point(50, 50), 0);
+  resizer->CompleteDrag();
+
+  //Index and id are not changed after dragging a |window|.
+  EXPECT_EQ(index, model_->ItemIndexByID(id));
+  EXPECT_EQ(id, model_->items()[index].id);
+}
+
+// Check |window|'s item is removed when it is re-parented not to default
+// container during the dragging.
+TEST_F(ShelfWindowWatcherTest, ReparentWindowDuringTheDragging) {
+  // ShelfModel only has an APP_LIST item.
+  EXPECT_EQ(1, model_->item_count());
+
+  scoped_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
+  window->set_owned_by_parent(false);
+
+  // Create a LauncherItem for |window|.
+  LauncherID id = CreateShelfItem(window.get());
+  EXPECT_EQ(2, model_->item_count());
+  int index = model_->ItemIndexByID(id);
+  EXPECT_EQ(STATUS_RUNNING, model_->items()[index].status);
+
+  aura::Window* root_window = window->GetRootWindow();
+  aura::Window* default_container = Shell::GetContainer(
+      root_window,
+      kShellWindowId_DefaultContainer);
+  EXPECT_EQ(default_container, window->parent());
+
+  aura::Window* new_parent = Shell::GetContainer(
+      root_window,
+      kShellWindowId_PanelContainer);
+
+  // Simulate re-parenting to |new_parent| during the dragging.
+  {
+    scoped_ptr<WindowResizer> resizer(
+        CreateWindowResizer(window.get(),
+                            gfx::Point(),
+                            HTCAPTION,
+                            aura::client::WINDOW_MOVE_SOURCE_MOUSE));
+    ASSERT_TRUE(resizer.get());
+    resizer->Drag(gfx::Point(50, 50), 0);
+    resizer->CompleteDrag();
+    EXPECT_EQ(2, model_->item_count());
+
+    // Item should be removed when |window| is re-parented not to default
+    // container before fininshing the dragging.
+    EXPECT_TRUE(wm::GetWindowState(window.get())->is_dragged());
+    new_parent->AddChild(window.get());
+    EXPECT_EQ(1, model_->item_count());
+  }
+  EXPECT_FALSE(wm::GetWindowState(window.get())->is_dragged());
+  EXPECT_EQ(1, model_->item_count());
+}
+
 }  // namespace internal
 }  // namespace ash