app_list: Fix possible out of bounds index.

- Fix a problem that could not drop at the last position when dragging
  an item out of a folder;
- Fix a case view_model_.Move is called with an out of range target index
  when dragging the last item out of folder;
- Add a test for dragging last item and drop it at last slot;
- Speculative fix to protect using an index from data model to modify
   view_model_;
- A few more DCHECKs to catch the out-of-bound case; 

BUG=370064
TEST=AppListMainViewTest.DragLastItemFromFolderAndDropAtLastSlot

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

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

ui/app_list/cocoa/apps_grid_controller_unittest.mm

@@ -454,6 +454,7 @@ TEST_F(AppsGridControllerTest, ModelUpdate) {
   EXPECT_NSEQ(@"UpdatedItem", [button title]);
 
   // Test icon updates through the model observer by ensuring the icon changes.
+  item_model->SetIcon(gfx::ImageSkia(), false);
   NSSize icon_size = [[button image] size];
   EXPECT_EQ(0, icon_size.width);
   EXPECT_EQ(0, icon_size.height);

ui/app_list/test/app_list_test_model.cc

@@ -7,11 +7,22 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "grit/ui_resources.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/app_list/app_list_constants.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image_skia.h"
 
 namespace app_list {
 namespace test {
 
+gfx::ImageSkia CreateImageSkia(int width, int height) {
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height);
+  bitmap.allocPixels();
+  bitmap.eraseARGB(255, 0, 255, 0);
+  return gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
+}
+
 // static
 const char AppListTestModel::kItemType[] = "TestItem";
 
@@ -22,6 +33,8 @@ AppListTestModel::AppListTestItem::AppListTestItem(
     AppListTestModel* model)
     : AppListItem(id),
       model_(model) {
+  SetIcon(CreateImageSkia(kPreferredIconDimension, kPreferredIconDimension),
+          false /* has_shadow */);
 }
 
 AppListTestModel::AppListTestItem::~AppListTestItem() {
@@ -93,6 +106,16 @@ AppListFolderItem* AppListTestModel::CreateAndAddOemFolder(
   return static_cast<AppListFolderItem*>(AddItem(folder));
 }
 
+AppListFolderItem* AppListTestModel::CreateSingleItemFolder(
+    const std::string& folder_id,
+    const std::string& item_id) {
+  AppListTestItem* item = CreateItem(item_id);
+  AddItemToFolder(item, folder_id);
+  AppListItem* folder_item = FindItem(folder_id);
+  DCHECK(folder_item->GetItemType() == AppListFolderItem::kItemType);
+  return static_cast<AppListFolderItem*>(folder_item);
+}
+
 void AppListTestModel::PopulateAppWithId(int id) {
   CreateAndAddItem(GetItemName(id));
 }

ui/app_list/test/app_list_test_model.h

@@ -53,6 +53,9 @@ class AppListTestModel : public AppListModel {
 
   AppListFolderItem* CreateAndAddOemFolder(const std::string& id);
 
+  AppListFolderItem* CreateSingleItemFolder(const std::string& folder_id,
+                                            const std::string& item_id);
+
   // Populate the model with an item titled "Item |id|".
   void PopulateAppWithId(int id);
 

ui/app_list/views/app_list_main_view_unittest.cc

@@ -4,13 +4,20 @@
 
 #include "ui/app_list/views/app_list_main_view.h"
 
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/app_list/pagination_model.h"
 #include "ui/app_list/test/app_list_test_model.h"
 #include "ui/app_list/test/app_list_test_view_delegate.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"
 #include "ui/app_list/views/contents_view.h"
+#include "ui/app_list/views/test/apps_grid_view_test_api.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/view_model.h"
 #include "ui/views/widget/widget.h"
@@ -22,6 +29,38 @@ namespace {
 
 const int kInitialItems = 2;
 
+class GridViewVisibleWaiter {
+ public:
+  explicit GridViewVisibleWaiter(AppsGridView* grid_view)
+      : grid_view_(grid_view) {}
+  ~GridViewVisibleWaiter() {}
+
+  void Wait() {
+    if (grid_view_->visible())
+      return;
+
+    check_timer_.Start(FROM_HERE,
+                       base::TimeDelta::FromMilliseconds(50),
+                       base::Bind(&GridViewVisibleWaiter::OnTimerCheck,
+                                  base::Unretained(this)));
+    run_loop_.reset(new base::RunLoop);
+    run_loop_->Run();
+    check_timer_.Stop();
+  }
+
+ private:
+  void OnTimerCheck() {
+    if (grid_view_->visible())
+      run_loop_->Quit();
+  }
+
+  AppsGridView* grid_view_;
+  scoped_ptr<base::RunLoop> run_loop_;
+  base::RepeatingTimer<GridViewVisibleWaiter> check_timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(GridViewVisibleWaiter);
+};
+
 class AppListMainViewTest : public views::ViewsTestBase {
  public:
   AppListMainViewTest()
@@ -34,7 +73,6 @@ class AppListMainViewTest : public views::ViewsTestBase {
   virtual void SetUp() OVERRIDE {
     views::ViewsTestBase::SetUp();
     delegate_.reset(new AppListTestViewDelegate);
-    delegate_->GetTestModel()->PopulateApps(kInitialItems);
 
     main_view_ =
         new AppListMainView(delegate_.get(), &pagination_model_, GetContext());
@@ -55,9 +93,78 @@ class AppListMainViewTest : public views::ViewsTestBase {
     delegate_.reset();
   }
 
-  const views::ViewModel* ViewModel() {
-    return main_view_->contents_view()->apps_container_view()->apps_grid_view()
-        ->view_model_for_test();
+  // |point| is in |grid_view|'s coordinates.
+  AppListItemView* GetItemViewAtPointInGrid(AppsGridView* grid_view,
+                                            const gfx::Point& point) {
+    const views::ViewModel* view_model = grid_view->view_model_for_test();
+    for (int i = 0; i < view_model->view_size(); ++i) {
+      views::View* view = view_model->view_at(i);
+      if (view->bounds().Contains(point)) {
+        return static_cast<AppListItemView*>(view);
+      }
+    }
+
+    return NULL;
+  }
+
+  void SimulateClick(views::View* view) {
+    gfx::Point center = view->GetLocalBounds().CenterPoint();
+    view->OnMousePressed(ui::MouseEvent(ui::ET_MOUSE_PRESSED,
+                                        center,
+                                        center,
+                                        ui::EF_LEFT_MOUSE_BUTTON,
+                                        ui::EF_LEFT_MOUSE_BUTTON));
+    view->OnMouseReleased(ui::MouseEvent(ui::ET_MOUSE_RELEASED,
+                                         center,
+                                         center,
+                                         ui::EF_LEFT_MOUSE_BUTTON,
+                                         ui::EF_LEFT_MOUSE_BUTTON));
+  }
+
+  // |point| is in |grid_view|'s coordinates.
+  AppListItemView* SimulateInitiateDrag(AppsGridView* grid_view,
+                                        AppsGridView::Pointer pointer,
+                                        const gfx::Point& point) {
+    AppListItemView* view = GetItemViewAtPointInGrid(grid_view, point);
+    DCHECK(view);
+
+    gfx::Point translated =
+        gfx::PointAtOffsetFromOrigin(point - view->bounds().origin());
+    ui::MouseEvent pressed_event(ui::ET_MOUSE_PRESSED, translated, point, 0, 0);
+    grid_view->InitiateDrag(view, pointer, pressed_event);
+    return view;
+  }
+
+  // |point| is in |grid_view|'s coordinates.
+  void SimulateUpdateDrag(AppsGridView* grid_view,
+                          AppsGridView::Pointer pointer,
+                          AppListItemView* drag_view,
+                          const gfx::Point& point) {
+    DCHECK(drag_view);
+    gfx::Point translated =
+        gfx::PointAtOffsetFromOrigin(point - drag_view->bounds().origin());
+    ui::MouseEvent drag_event(ui::ET_MOUSE_DRAGGED, translated, point, 0, 0);
+    grid_view->UpdateDragFromItem(pointer, drag_event);
+  }
+
+  AppsGridView* RootGridView() {
+    return main_view_->contents_view()->apps_container_view()->apps_grid_view();
+  }
+
+  AppListFolderView* FolderView() {
+    return main_view_->contents_view()
+        ->apps_container_view()
+        ->app_list_folder_view();
+  }
+
+  AppsGridView* FolderGridView() { return FolderView()->items_grid_view(); }
+
+  const views::ViewModel* RootViewModel() {
+    return RootGridView()->view_model_for_test();
+  }
+
+  const views::ViewModel* FolderViewModel() {
+    return FolderGridView()->view_model_for_test();
   }
 
  protected:
@@ -74,7 +181,8 @@ class AppListMainViewTest : public views::ViewsTestBase {
 
 // Tests changing the AppListModel when switching profiles.
 TEST_F(AppListMainViewTest, ModelChanged) {
-  EXPECT_EQ(kInitialItems, ViewModel()->view_size());
+  delegate_->GetTestModel()->PopulateApps(kInitialItems);
+  EXPECT_EQ(kInitialItems, RootViewModel()->view_size());
 
   // The model is owned by a profile keyed service, which is never destroyed
   // until after profile switching.
@@ -83,7 +191,86 @@ TEST_F(AppListMainViewTest, ModelChanged) {
   const int kReplacementItems = 5;
   delegate_->ReplaceTestModel(kReplacementItems);
   main_view_->ModelChanged();
-  EXPECT_EQ(kReplacementItems, ViewModel()->view_size());
+  EXPECT_EQ(kReplacementItems, RootViewModel()->view_size());
+}
+
+// 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);
+  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());
+
+  // 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();
+  SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
+  SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point);
+  base::RunLoop().RunUntilIdle();
+
+  // Drop it.
+  FolderGridView()->EndDrag(false);
+  base::RunLoop().RunUntilIdle();
+
+  // Folder icon view should be gone and there is only one item view.
+  EXPECT_EQ(1, RootViewModel()->view_size());
+  EXPECT_EQ(AppListItemView::kViewClassName,
+            RootViewModel()->view_at(0)->GetClassName());
+
+  // The item view should be in slot 1 instead of slot 2 where it is dropped.
+  AppsGridViewTestApi root_grid_view_test_api(RootGridView());
+  root_grid_view_test_api.LayoutToIdealBounds();
+  EXPECT_EQ(first_slot_tile, RootViewModel()->view_at(0)->bounds());
+
+  // Single item folder should be auto removed.
+  EXPECT_EQ(NULL,
+            delegate_->GetTestModel()->FindFolderItem("single_item_folder"));
 }
 
 }  // namespace test

ui/app_list/views/apps_grid_view.cc

@@ -348,6 +348,9 @@ AppsGridView::AppsGridView(AppsGridViewDelegate* delegate,
       selected_view_(NULL),
       drag_view_(NULL),
       drag_start_page_(-1),
+#if defined(OS_WIN)
+      use_synchronous_drag_(true),
+#endif
       drag_pointer_(NONE),
       drop_attempt_(DROP_FOR_NONE),
       drag_and_drop_host_(NULL),
@@ -402,6 +405,8 @@ void AppsGridView::ResetForShowApps() {
   for (int i = 0; i < view_model_.view_size(); ++i) {
     view_model_.view_at(i)->SetVisible(true);
   }
+  CHECK_EQ(item_list_->item_count(),
+           static_cast<size_t>(view_model_.view_size()));
 }
 
 void AppsGridView::SetModel(AppListModel* model) {
@@ -477,7 +482,7 @@ void AppsGridView::InitiateDrag(AppListItemView* view,
 
 void AppsGridView::StartSettingUpSynchronousDrag() {
 #if defined(OS_WIN)
-  if (!delegate_)
+  if (!delegate_ || !use_synchronous_drag_)
     return;
 
   // Folders can't be integrated with the OS.
@@ -812,7 +817,9 @@ void AppsGridView::ClearDragState() {
   if (drag_view_) {
     drag_view_->OnDragEnded();
     if (IsDraggingForReparentInRootLevelGridView()) {
-      DeleteItemViewAtIndex(view_model_.GetIndexOfView(drag_view_));
+      const int drag_view_index = view_model_.GetIndexOfView(drag_view_);
+      CHECK_EQ(view_model_.view_size() - 1, drag_view_index);
+      DeleteItemViewAtIndex(drag_view_index);
     }
   }
   drag_view_ = NULL;
@@ -939,6 +946,9 @@ bool AppsGridView::OnKeyReleased(const ui::KeyEvent& event) {
 void AppsGridView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   if (!details.is_add && details.parent == this) {
+    // The view being delete should not have reference in |view_model_|.
+    CHECK_EQ(-1, view_model_.GetIndexOfView(details.child));
+
     if (selected_view_ == details.child)
       selected_view_ = NULL;
     if (activated_folder_item_view_ == details.child)
@@ -1693,16 +1703,23 @@ void AppsGridView::ReparentItemForReorder(views::View* item_view,
   AppListFolderItem* source_folder =
       static_cast<AppListFolderItem*>(item_list_->FindItem(source_folder_id));
 
+  int target_model_index = GetModelIndexFromIndex(target);
+
   // Remove the source folder view if there is only 1 item in it, since the
   // source folder will be deleted after its only child item removed from it.
-  if (source_folder->ChildItemCount() == 1u)
-    DeleteItemViewAtIndex(
-        view_model_.GetIndexOfView(activated_folder_item_view()));
+  if (source_folder->ChildItemCount() == 1u) {
+    const int deleted_folder_index =
+        view_model_.GetIndexOfView(activated_folder_item_view());
+    DeleteItemViewAtIndex(deleted_folder_index);
+
+    // Adjust |target_model_index| if it is beyond the deleted folder index.
+    if (target_model_index > deleted_folder_index)
+      --target_model_index;
+  }
 
   // Move the item from its parent folder to top level item list.
   // Must move to target_model_index, the location we expect the target item
   // to be, not the item location we want to insert before.
-  int target_model_index = GetModelIndexFromIndex(target);
   int current_model_index = view_model_.GetIndexOfView(item_view);
   syncer::StringOrdinal target_position;
   if (target_model_index < static_cast<int>(item_list_->item_count()))
@@ -1804,7 +1821,11 @@ void AppsGridView::RemoveLastItemFromReparentItemFolderIfNecessary(
 
   // Create a new item view for the last item in folder.
   size_t last_item_index;
-  item_list_->FindItemIndex(last_item->id(), &last_item_index);
+  if (!item_list_->FindItemIndex(last_item->id(), &last_item_index) ||
+      last_item_index > static_cast<size_t>(view_model_.view_size())) {
+    NOTREACHED();
+    return;
+  }
   views::View* last_item_view = CreateViewForItemAtIndex(last_item_index);
   view_model_.Add(last_item_view, last_item_index);
   AddChildView(last_item_view);
@@ -2016,7 +2037,7 @@ AppsGridView::Index AppsGridView::GetNearestTileForDragView() {
 
   // If user drags an item across pages to the last page, and targets it
   // to the last empty slot on it, push the last item for re-ordering.
-  if (IsFirstEmptySlot(nearest_tile) && d_min < d_reorder) {
+  if (IsLastPossibleDropTarget(nearest_tile) && d_min < d_reorder) {
     drop_attempt_ = DROP_FOR_REORDER;
     nearest_tile.slot = nearest_tile.slot - 1;
     return nearest_tile;
@@ -2119,14 +2140,8 @@ gfx::Rect AppsGridView::GetTileBounds(int row, int col) const {
   return tile_rect;
 }
 
-bool AppsGridView::IsFirstEmptySlot(const Index& index) const {
-  // Don't count the hidden drag_view_ created from the item_dragged out of a
-  // folder during re-parenting into the total number of views that are visible
-  // on all grid view pages.
-  int total_views = IsDraggingForReparentInRootLevelGridView()
-                        ? view_model_.view_size() - 1
-                        : view_model_.view_size();
-  int last_possible_slot = (total_views - 1) % tiles_per_page();
+bool AppsGridView::IsLastPossibleDropTarget(const Index& index) const {
+  int last_possible_slot = view_model_.view_size() % tiles_per_page();
   return (index.page == pagination_model_->total_pages() - 1 &&
           index.slot == last_possible_slot + 1);
 }

ui/app_list/views/apps_grid_view.h

@@ -403,9 +403,9 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
   // Gets the bounds of the tile located at |row| and |col| on current page.
   gfx::Rect GetTileBounds(int row, int col) const;
 
-  // Returns true if the slot of |index| is the first empty slot next to the
-  // last item on the last page.
-  bool IsFirstEmptySlot(const Index& index) const;
+  // Returns true if the slot of |index| is the last possible slot to drop
+  // an item, i.e. first empty slot next to the last item on the last page.
+  bool IsLastPossibleDropTarget(const Index& index) const;
 
   // Gets the item view located at |slot| on the current page. If there is
   // no item located at |slot|, returns NULL.
@@ -497,6 +497,9 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
   // Created when a drag is started (ie: drag exceeds the drag threshold), but
   // not Run() until supplied with a shortcut path.
   scoped_refptr<SynchronousDrag> synchronous_drag_;
+
+  // Whether to use SynchronousDrag to support dropping to task bar etc.
+  bool use_synchronous_drag_;
 #endif
 
   Pointer drag_pointer_;

ui/app_list/views/test/apps_grid_view_test_api.cc

@@ -36,5 +36,14 @@ void AppsGridViewTestApi::PressItemAt(int index) {
       ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, 0, false));
 }
 
+void AppsGridViewTestApi::DisableSynchronousDrag() {
+#if defined(OS_WIN)
+  DCHECK(view_->synchronous_drag_ == NULL) << "DisableSynchronousDrag needs to "
+                                              "be called before "
+                                              "synchronous_drag_ is set up.";
+  view_->use_synchronous_drag_ = false;
+#endif
+}
+
 }  // namespace test
 }  // namespace app_list

ui/app_list/views/test/apps_grid_view_test_api.h

@@ -30,6 +30,8 @@ class AppsGridViewTestApi {
 
   void PressItemAt(int index);
 
+  void DisableSynchronousDrag();
+
  private:
   AppsGridView* view_;
 

ui/views/view_model.cc

@@ -33,6 +33,11 @@ void ViewModel::Remove(int index) {
 }
 
 void ViewModel::Move(int index, int target_index) {
+  DCHECK_LT(index, static_cast<int>(entries_.size()));
+  DCHECK_GE(index, 0);
+  DCHECK_LT(target_index, static_cast<int>(entries_.size()));
+  DCHECK_GE(target_index, 0);
+
   if (index == target_index)
     return;
   Entry entry(entries_[index]);