AppListModel / AppsGridView: Better error handling for corner cases.

ui/app_list/app_list_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/app_list/app_list_model.h"
 
 #include <string>
 
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
@@ skipping 73 matching lines @@
   return AddItemToFolderItemAndNotify(dest_folder, item.Pass());
 }
 
 const std::string AppListModel::MergeItems(const std::string& target_item_id,
                                            const std::string& source_item_id) {
   if (!folders_enabled()) {
     LOG(ERROR) << "MergeItems called with folders disabled.";
     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) {
     LOG(ERROR) << "MergeItems: Source no longer exists.";
     return "";
   }
 
   // If the target item is a folder, just add the source item to it.
   if (target_item->GetItemType() == AppListFolderItem::kItemType) {
     AppListFolderItem* target_folder =
         static_cast<AppListFolderItem*>(target_item);
     if (target_folder->folder_type() == AppListFolderItem::FOLDER_TYPE_OEM) {
       LOG(WARNING) << "MergeItems called with OEM folder as target";
       return "";
     }
     scoped_ptr<AppListItem> source_item_ptr = RemoveItem(source_item);
     source_item_ptr->set_position(
         target_folder->item_list()->CreatePositionBefore(
             syncer::StringOrdinal()));
     AddItemToFolderItemAndNotify(target_folder, source_item_ptr.Pass());
     return target_folder->id();
   }
 
   // Otherwise remove the source item and target item from their current
   // 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);
 
   // Create a new folder in the same location as the target item.
   std::string new_folder_id = AppListFolderItem::GenerateId();
   DVLOG(2) << "Creating folder for merge: " << new_folder_id;
   scoped_ptr<AppListItem> new_folder_ptr(new AppListFolderItem(
       new_folder_id, AppListFolderItem::FOLDER_TYPE_NORMAL));
   new_folder_ptr->set_position(target_item_ptr->position());
@@ skipping 246 matching lines @@
   scoped_ptr<AppListItem> result = folder->item_list()->RemoveItem(item->id());
   result->set_folder_id("");
   if (folder->item_list()->item_count() == 0) {
     DVLOG(2) << "Deleting empty folder: " << folder->ToDebugString();
     DeleteItem(folder_id);
   }
   return result.Pass();
 }
 
 }  // namespace app_list