AppListModel / AppsGridView: Better error handling for corner cases.

ui/app_list/app_list_model.cc

Index: ui/app_list/app_list_model.cc
diff --git a/ui/app_list/app_list_model.cc b/ui/app_list/app_list_model.cc
index 22ac8a25634973496035b81e118e10ef1e14ad89..b3512c8b79f3e453c77d12aaa63224a6c1ccbf3f 100644
--- a/ui/app_list/app_list_model.cc
+++ b/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 ("

[calamity, 2014/09/26 03:55:18]

These can probably stand to be DCHECKs.

[Matt Giuca, 2014/09/29 03:05:03]

Hmm. If this is a DCHECK and the one in apps_grid_view is also a DCHECK, it
means that in a Release build we will hit the same crash as before (RemoveItem)
if it is ever possible to duplicate an item in the launcher.

I think we should either keep it as-is (as a compile-time checked warning and
return ""), or make one of those a CHECK so that stack traces show the true
cause rather than an undiagnosable "RemoveItem: Item not found".

Which of these should we do?

[Matt Giuca, 2014/09/29 03:34:34]

As discussed, it's reasonable to leave the model's semantics as "it's legal to
try to do this; you get back an error code" as opposed to crashing.

And safer, because we don't know if these other errors like the OEM one are
possible to trigger in the UI.

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