AppListModel / AppsGridView: Better error handling for corner cases.

Fixes a potential crash if an app gets duplicated in the app list, and
the user tries to drop it onto itself. (There is no known way to repro
this currently, but there was recently; see http://crbug.com/415530.)
That situation will now result in a very descriptive DCHECK, rather than
a confusing low-level CHECK fail.

AppListModel::MergeItems:
- returns "" instead of crashing if the source and target IDs are equal.
- returns "" instead of crashing if the target ID is inside a folder (no
  longer needs to explicitly CHECK whether it is in a folder.)

Added plenty of new tests for the various corner cases in
AppListModel::MergeItems which would have caught these crashes.

AppsGridView::MoveItemToFolder:
- DCHECKs for a NULL result of GetViewAtSlotOnCurrentPage.
- DCHECKs that you aren't dropping an item onto itself (this should
  never happen).

BUG=417482

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

Cr-Commit-Position: refs/heads/master@{#297356}

ui/app_list/app_list_model.cc

@@ -91,13 +91,19 @@ const std::string AppListModel::MergeItems(const std::string& target_item_id,
     return "";
   }
   DVLOG(2) << "MergeItems: " << source_item_id << " -> " << target_item_id;
+
+  if (target_item_id == source_item_id) {
+    LOG(WARNING) << "MergeItems tried to drop item onto itself ("
+                 << source_item_id << " -> " << target_item_id << ").";
+    return "";
+  }
+
   // Find the target item.
-  AppListItem* target_item = FindItem(target_item_id);
+  AppListItem* target_item = top_level_item_list_->FindItem(target_item_id);
   if (!target_item) {
     LOG(ERROR) << "MergeItems: Target no longer exists.";
     return "";
   }
-  CHECK(target_item->folder_id().empty());
 
   AppListItem* source_item = FindItem(source_item_id);
   if (!source_item) {
@@ -125,6 +131,8 @@ const std::string AppListModel::MergeItems(const std::string& target_item_id,
   // location, they will become owned by the new folder.
   scoped_ptr<AppListItem> source_item_ptr = RemoveItem(source_item);
   CHECK(source_item_ptr);
+  // Note: This would fail if |target_item_id == source_item_id|, except we
+  // checked that they are distinct at the top of this method.
   scoped_ptr<AppListItem> target_item_ptr =
       top_level_item_list_->RemoveItem(target_item_id);
   CHECK(target_item_ptr);

ui/app_list/app_list_model_unittest.cc

@@ -292,6 +292,20 @@ TEST_F(AppListModelFolderTest, MergeItems) {
   AppListItem* item1 = model_.top_level_item_list()->item_at(1);
   AppListItem* item2 = model_.top_level_item_list()->item_at(2);
 
+  // Merge an item onto a non-existent target.
+  EXPECT_EQ(std::string(), model_.MergeItems("nonexistent", item0->id()));
+  ASSERT_EQ(3u, model_.top_level_item_list()->item_count());
+
+  // Merge a non-existent item onto a target.
+  EXPECT_EQ(std::string(), model_.MergeItems(item0->id(), "nonexistent"));
+  ASSERT_EQ(3u, model_.top_level_item_list()->item_count());
+
+  // Merge an item onto itself (should have no effect). This should not be
+  // possible, but there have been bugs in the past that made it possible (see
+  // http://crbug.com/415530), so it should be handled correctly.
+  EXPECT_EQ(std::string(), model_.MergeItems(item0->id(), item0->id()));
+  ASSERT_EQ(3u, model_.top_level_item_list()->item_count());
+
   // Merge two items.
   std::string folder1_id = model_.MergeItems(item0->id(), item1->id());
   ASSERT_EQ(2u, model_.top_level_item_list()->item_count());  // Folder + 1 item
@@ -299,6 +313,13 @@ TEST_F(AppListModelFolderTest, MergeItems) {
   ASSERT_TRUE(folder1_item);
   EXPECT_EQ("Item 0,Item 1", GetItemListContents(folder1_item->item_list()));
 
+  // Merge an item onto an item that is already in a folder (should have no
+  // effect). This should not be possible, but it should be handled correctly
+  // if it does happen.
+  EXPECT_EQ(std::string(), model_.MergeItems(item1->id(), item2->id()));
+  ASSERT_EQ(2u, model_.top_level_item_list()->item_count());  // Folder + 1 item
+  EXPECT_EQ("Item 0,Item 1", GetItemListContents(folder1_item->item_list()));
+
   // Merge an item from the new folder into the third item.
   std::string folder2_id = model_.MergeItems(item2->id(), item1->id());
   ASSERT_EQ(2u, model_.top_level_item_list()->item_count());  // 2 folders
@@ -315,6 +336,7 @@ TEST_F(AppListModelFolderTest, MergeItems) {
   // The empty folder should be deleted.
   folder1_item = model_.FindFolderItem(folder1_id);
   EXPECT_FALSE(folder1_item);
+  EXPECT_EQ(1u, model_.top_level_item_list()->item_count());  // 1 folder
 }
 
 TEST_F(AppListModelFolderTest, AddItemToFolder) {

ui/app_list/views/apps_grid_view.cc

@@ -1741,8 +1741,14 @@ void AppsGridView::MoveItemToFolder(AppListItemView* item_view,
   const std::string& source_item_id = item_view->item()->id();
   AppListItemView* target_view =
       GetViewDisplayedAtSlotOnCurrentPage(target.slot);
+  DCHECK(target_view);
   const std::string& target_view_item_id = target_view->item()->id();
 
+  // Check that the item is not being dropped onto itself; this should not
+  // happen, but it can if something allows multiple views to share an
+  // item (e.g., if a folder drop does not clean up properly).
+  DCHECK_NE(source_item_id, target_view_item_id);
+
   // Make change to data model.
   item_list_->RemoveObserver(this);
   std::string folder_item_id =