Fix app list DCHECKs getting hit when model updates occur during dragging

The DCHECK from AppsGridView::UpdateDragFromItem() is changed
to an early return. This is a valid codepath, because drag events
can still arrive from the AppListItemView after a drag is cancelled,
e.g., via Sync.

AppsGridViewTest.MouseDragWithFolderDisabled is updated to
include coverage for this codepath.

A DCHECK was also hit in UpdateDragFromReparentItem(). This was not
a valid code path. The problem was that the root grid view tried
to cancel only its "part" of the drag, rather than forwarding the
cancel request to the folder managing the drag.

AppListMainViewTest.MouseDragItemOutOfFolderWithCancel is added to
include coverage for this codepath.

BUG=402784
TEST=app_list_unittests

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

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

6 files changed, 118 insertions, 49 deletions

diff --git a/ui/app_list/views/app_list_main_view.cc b/ui/app_list/views/app_list_main_view.cc
index 2cbe4f6..c0f7a83 100644
--- a/ui/app_list/views/app_list_main_view.cc
+++ b/ui/app_list/views/app_list_main_view.cc
@@ -19,6 +19,7 @@
 #include "ui/app_list/app_list_view_delegate.h"
 #include "ui/app_list/pagination_model.h"
 #include "ui/app_list/search_box_model.h"
+#include "ui/app_list/views/app_list_folder_view.h"
 #include "ui/app_list/views/app_list_item_view.h"
 #include "ui/app_list/views/apps_container_view.h"
 #include "ui/app_list/views/apps_grid_view.h"
@@ -302,6 +303,13 @@ void AppListMainView::GetShortcutPathForApp(
   delegate_->GetShortcutPathForApp(app_id, callback);
 }
 
+void AppListMainView::CancelDragInActiveFolder() {
+  contents_view_->apps_container_view()
+      ->app_list_folder_view()
+      ->items_grid_view()
+      ->EndDrag(true);
+}
+
 void AppListMainView::QueryChanged(SearchBoxView* sender) {
   base::string16 query;
   base::TrimWhitespace(model_->search_box()->text(), base::TRIM_ALL, &query);
diff --git a/ui/app_list/views/app_list_main_view.h b/ui/app_list/views/app_list_main_view.h
index 15fdf06..b45559d 100644
--- a/ui/app_list/views/app_list_main_view.h
+++ b/ui/app_list/views/app_list_main_view.h
@@ -101,6 +101,7 @@ class APP_LIST_EXPORT AppListMainView : public views::View,
   virtual void GetShortcutPathForApp(
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) OVERRIDE;
+  virtual void CancelDragInActiveFolder() OVERRIDE;
 
   // Overridden from SearchBoxViewDelegate:
   virtual void QueryChanged(SearchBoxView* sender) OVERRIDE;
diff --git a/ui/app_list/views/app_list_main_view_unittest.cc b/ui/app_list/views/app_list_main_view_unittest.cc
index 625a86c..d59e45f 100644
--- a/ui/app_list/views/app_list_main_view_unittest.cc
+++ b/ui/app_list/views/app_list_main_view_unittest.cc
@@ -169,6 +169,60 @@ class AppListMainViewTest : public views::ViewsTestBase {
     return FolderGridView()->view_model_for_test();
   }
 
+  AppListItemView* CreateAndOpenSingleItemFolder() {
+    // Prepare single folder with a single item in it.
+    AppListFolderItem* folder_item =
+        delegate_->GetTestModel()->CreateSingleItemFolder("single_item_folder",
+                                                          "single");
+    EXPECT_EQ(folder_item,
+              delegate_->GetTestModel()->FindFolderItem("single_item_folder"));
+    EXPECT_EQ(AppListFolderItem::kItemType, folder_item->GetItemType());
+
+    EXPECT_EQ(1, RootViewModel()->view_size());
+    AppListItemView* folder_item_view =
+        static_cast<AppListItemView*>(RootViewModel()->view_at(0));
+    EXPECT_EQ(folder_item_view->item(), folder_item);
+
+    // Click on the folder to open it.
+    EXPECT_FALSE(FolderView()->visible());
+    SimulateClick(folder_item_view);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FolderView()->visible());
+
+#if defined(OS_WIN)
+    AppsGridViewTestApi folder_grid_view_test_api(FolderGridView());
+    folder_grid_view_test_api.DisableSynchronousDrag();
+#endif
+    return folder_item_view;
+  }
+
+  AppListItemView* StartDragForReparent(int index_in_folder) {
+    // Start to drag the item in folder.
+    views::View* item_view = FolderViewModel()->view_at(index_in_folder);
+    gfx::Point point = item_view->bounds().CenterPoint();
+    AppListItemView* dragged =
+        SimulateInitiateDrag(FolderGridView(), AppsGridView::MOUSE, point);
+    EXPECT_EQ(item_view, dragged);
+    EXPECT_FALSE(RootGridView()->visible());
+    EXPECT_TRUE(FolderView()->visible());
+
+    // Drag it to top left corner.
+    point = gfx::Point(0, 0);
+    // Two update drags needed to actually drag the view. The first changes
+    // state and the 2nd one actually moves the view. The 2nd call can be
+    // removed when UpdateDrag is fixed.
+    SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
+    SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
+    base::RunLoop().RunUntilIdle();
+
+    // Wait until the folder view is invisible and root grid view shows up.
+    GridViewVisibleWaiter(RootGridView()).Wait();
+    EXPECT_TRUE(RootGridView()->visible());
+    EXPECT_EQ(0, FolderView()->layer()->opacity());
+
+    return dragged;
+  }
+
  protected:
   views::Widget* widget_;  // Owned by native window.
   AppListMainView* main_view_;  // Owned by |widget_|.
@@ -198,59 +252,17 @@ TEST_F(AppListMainViewTest, ModelChanged) {
 // Tests dragging an item out of a single item folder and drop it at the last
 // slot.
 TEST_F(AppListMainViewTest, DragLastItemFromFolderAndDropAtLastSlot) {
-  // Prepare single folder with a single item in it.
-  AppListFolderItem* folder_item =
-      delegate_->GetTestModel()->CreateSingleItemFolder("single_item_folder",
-                                                        "single");
-  EXPECT_EQ(folder_item,
-            delegate_->GetTestModel()->FindFolderItem("single_item_folder"));
-  EXPECT_EQ(AppListFolderItem::kItemType, folder_item->GetItemType());
-
-  EXPECT_EQ(1, RootViewModel()->view_size());
-  AppListItemView* folder_item_view =
-      static_cast<AppListItemView*>(RootViewModel()->view_at(0));
-  EXPECT_EQ(folder_item_view->item(), folder_item);
+  AppListItemView* folder_item_view = CreateAndOpenSingleItemFolder();
   const gfx::Rect first_slot_tile = folder_item_view->bounds();
 
-  // Click on the folder to open it.
-  EXPECT_FALSE(FolderView()->visible());
-  SimulateClick(folder_item_view);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(FolderView()->visible());
-
-#if defined(OS_WIN)
-  AppsGridViewTestApi folder_grid_view_test_api(FolderGridView());
-  folder_grid_view_test_api.DisableSynchronousDrag();
-#endif
-
-  // Start to drag the item in folder.
   EXPECT_EQ(1, FolderViewModel()->view_size());
-  views::View* item_view = FolderViewModel()->view_at(0);
-  gfx::Point point = item_view->bounds().CenterPoint();
-  AppListItemView* dragged =
-      SimulateInitiateDrag(FolderGridView(), AppsGridView::MOUSE, point);
-  EXPECT_EQ(item_view, dragged);
-  EXPECT_FALSE(RootGridView()->visible());
-  EXPECT_TRUE(FolderView()->visible());
-
-  // Drag it to top left corner.
-  point = gfx::Point(0, 0);
-  // Two update drags needed to actually drag the view. The first changes state
-  // and the 2nd one actually moves the view. The 2nd call can be removed when
-  // UpdateDrag is fixed.
-  SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
-  SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
-  base::RunLoop().RunUntilIdle();
 
-  // Wait until the folder view is invisible and root grid view shows up.
-  GridViewVisibleWaiter(RootGridView()).Wait();
-  EXPECT_TRUE(RootGridView()->visible());
-  EXPECT_EQ(0, FolderView()->layer()->opacity());
+  AppListItemView* dragged = StartDragForReparent(0);
 
   // Drop it to the slot on the right of first slot.
   gfx::Rect drop_target_tile(first_slot_tile);
   drop_target_tile.Offset(first_slot_tile.width(), 0);
-  point = drop_target_tile.CenterPoint();
+  gfx::Point point = drop_target_tile.CenterPoint();
   SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
   SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
   base::RunLoop().RunUntilIdle();
@@ -274,5 +286,28 @@ TEST_F(AppListMainViewTest, DragLastItemFromFolderAndDropAtLastSlot) {
             delegate_->GetTestModel()->FindFolderItem("single_item_folder"));
 }
 
+// Test that an interrupted drag while reparenting an item from a folder, when
+// canceled via the root grid, correctly forwards the cancelation to the drag
+// ocurring from the folder.
+TEST_F(AppListMainViewTest, MouseDragItemOutOfFolderWithCancel) {
+  CreateAndOpenSingleItemFolder();
+  AppListItemView* dragged = StartDragForReparent(0);
+
+  // Now add an item to the model, not in any folder, e.g., as if by Sync.
+  EXPECT_TRUE(RootGridView()->has_dragged_view());
+  EXPECT_TRUE(FolderGridView()->has_dragged_view());
+  delegate_->GetTestModel()->CreateAndAddItem("Extra");
+
+  // The drag operation should get canceled.
+  EXPECT_FALSE(RootGridView()->has_dragged_view());
+  EXPECT_FALSE(FolderGridView()->has_dragged_view());
+
+  // Additional mouse move operations should be ignored.
+  gfx::Point point(1, 1);
+  SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
+  EXPECT_FALSE(RootGridView()->has_dragged_view());
+  EXPECT_FALSE(FolderGridView()->has_dragged_view());
+}
+
 }  // namespace test
 }  // namespace app_list
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index 6516dd9..f150eb6 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -555,7 +555,8 @@ void AppsGridView::OnGotShortcutPath(
 
 bool AppsGridView::UpdateDragFromItem(Pointer pointer,
                                       const ui::LocatedEvent& event) {
-  DCHECK(drag_view_);
+  if (!drag_view_)
+    return false;  // Drag canceled.
 
   gfx::Point drag_point_in_grid_view;
   ExtractDragLocation(event, &drag_point_in_grid_view);
@@ -577,9 +578,8 @@ void AppsGridView::UpdateDrag(Pointer pointer, const gfx::Point& point) {
   if (folder_delegate_)
     UpdateDragStateInsideFolder(pointer, point);
 
-  // EndDrag was called before if |drag_view_| is NULL.
   if (!drag_view_)
-    return;
+    return;  // Drag canceled.
 
   if (RunSynchronousDrag())
     return;
@@ -673,6 +673,14 @@ void AppsGridView::EndDrag(bool cancel) {
       return;
     }
 
+    if (IsDraggingForReparentInRootLevelGridView()) {
+      // An EndDrag can be received during a reparent via a model change. This
+      // is always a cancel and needs to be forwarded to the folder.
+      DCHECK(cancel);
+      delegate_->CancelDragInActiveFolder();
+      return;
+    }
+
     if (!cancel && dragging()) {
       // Regular drag ending path, ie, not for reparenting.
       CalculateDropTarget(last_drag_point_, true);
@@ -795,6 +803,9 @@ void AppsGridView::InitiateDragFromReparentItemInRootLevelGridView(
 
 void AppsGridView::UpdateDragFromReparentItem(Pointer pointer,
                                               const gfx::Point& drag_point) {
+  // Note that if a cancel ocurrs while reparenting, the |drag_view_| in both
+  // root and folder grid views is cleared, so the check in UpdateDragFromItem()
+  // for |drag_view_| being NULL (in the folder grid) is sufficient.
   DCHECK(drag_view_);
   DCHECK(IsDraggingForReparentInRootLevelGridView());
 
diff --git a/ui/app_list/views/apps_grid_view_delegate.h b/ui/app_list/views/apps_grid_view_delegate.h
index 14b31a0..3be3eee 100644
--- a/ui/app_list/views/apps_grid_view_delegate.h
+++ b/ui/app_list/views/apps_grid_view_delegate.h
@@ -30,6 +30,11 @@ class APP_LIST_EXPORT AppsGridViewDelegate {
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) = 0;
 
+  // Called by the root grid view to cancel a drag that started inside a folder.
+  // This can occur when the root grid is visible for a reparent and its model
+  // changes, necessitating a cancel of the drag operation.
+  virtual void CancelDragInActiveFolder() = 0;
+
  protected:
   virtual ~AppsGridViewDelegate() {}
 };
diff --git a/ui/app_list/views/apps_grid_view_unittest.cc b/ui/app_list/views/apps_grid_view_unittest.cc
index 7dc272d..adee4cb 100644
--- a/ui/app_list/views/apps_grid_view_unittest.cc
+++ b/ui/app_list/views/apps_grid_view_unittest.cc
@@ -306,8 +306,17 @@ TEST_F(AppsGridViewTest, MouseDragWithFolderDisabled) {
 
   // Adding a launcher item cancels the drag and respects the order.
   SimulateDrag(AppsGridView::MOUSE, from, to);
+  EXPECT_TRUE(apps_grid_view_->has_dragged_view());
   model_->CreateAndAddItem("Extra");
-  apps_grid_view_->EndDrag(false);
+  // No need to EndDrag explicitly - adding an item should do this.
+  EXPECT_FALSE(apps_grid_view_->has_dragged_view());
+  // Even though cancelled, mouse move events can still arrive via the item
+  // view. Ensure that behaves sanely, and doesn't start a new drag.
+  ui::MouseEvent drag_event(
+      ui::ET_MOUSE_DRAGGED, gfx::Point(1, 1), gfx::Point(2, 2), 0, 0);
+  apps_grid_view_->UpdateDragFromItem(AppsGridView::MOUSE, drag_event);
+  EXPECT_FALSE(apps_grid_view_->has_dragged_view());
+
   EXPECT_EQ(std::string("Item 1,Item 2,Item 3,Extra"),
             model_->GetModelContent());
   test_api_->LayoutToIdealBounds();