Protect AppListItemList Add/Remove and fix sync bugs

ui/app_list/views/apps_grid_view.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/views/apps_grid_view.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 
@@ skipping 164 matching lines @@
 int GetDistanceBetweenRects(gfx::Rect rect_1,
                             gfx::Rect rect_2) {
   return (rect_1.CenterPoint() - rect_2.CenterPoint()).Length();
 }
 
 // Returns true if the |item| is an folder item.
 bool IsFolderItem(AppListItem* item) {
   return (item->GetItemType() == AppListFolderItem::kItemType);
 }
 
-// Merges |source_item| into the folder containing the target item specified
-// by |target_item_id|. Both |source_item| and target item belongs to
-// |item_list|.
-// Returns the index of the target folder.
-size_t MergeItems(AppListItemList* item_list,
-                  const std::string& target_item_id,
-                  AppListItem* source_item) {
-  scoped_ptr<AppListItem> source_item_ptr =
-      item_list->RemoveItem(source_item->id());
-  DCHECK_EQ(source_item, source_item_ptr.get());
-  size_t target_index;
-  bool found_target_item = item_list->FindItemIndex(target_item_id,
-                                                    &target_index);
-  DCHECK(found_target_item);
-  AppListItem* target_item = item_list->item_at(target_index);
-  if (IsFolderItem(target_item)) {
-    AppListFolderItem* target_folder =
-        static_cast<AppListFolderItem*>(target_item);
-    target_folder->item_list()->AddItem(source_item_ptr.release());
-  } else {
-    scoped_ptr<AppListItem> target_item_ptr =
-        item_list->RemoveItemAt(target_index);
-    DCHECK_EQ(target_item, target_item_ptr.get());
-    AppListFolderItem* new_folder =
-        new AppListFolderItem(base::GenerateGUID());
-    new_folder->item_list()->AddItem(target_item_ptr.release());
-    new_folder->item_list()->AddItem(source_item_ptr.release());
-    item_list->InsertItemAt(new_folder, target_index);
-  }
-
-  return target_index;
-}
-
 }  // namespace
 
 #if defined(OS_WIN)
 // Interprets drag events sent from Windows via the drag/drop API and forwards
 // them to AppsGridView.
 // On Windows, in order to have the OS perform the drag properly we need to
 // provide it with a shortcut file which may or may not exist at the time the
 // drag is started. Therefore while waiting for that shortcut to be located we
 // just do a regular "internal" drag and transition into the synchronous drag
 // when the shortcut is found/created. Hence a synchronous drag is an optional
@@ skipping 1132 matching lines @@
   item_list_->MoveItem(current_model_index, target_model_index);
   view_model_.Move(current_model_index, target_model_index);
   item_list_->AddObserver(this);
 
   if (pagination_model_->selected_page() != target.page)
     pagination_model_->SelectPage(target.page, false);
 }
 
 void AppsGridView::MoveItemToFolder(views::View* item_view,
                                     const Index& target) {
-  AppListItem* source_item =
-      static_cast<AppListItemView*>(item_view)->item();
+  std::string source_id =

[xiyuan, 2014/01/28 05:58:29]

nit: const std::string&

[stevenjb, 2014/01/28 18:28:08]

Done.

+      static_cast<AppListItemView*>(item_view)->item()->id();
   AppListItemView* target_view =
       static_cast<AppListItemView*>(GetViewAtSlotOnCurrentPage(target.slot));
-  AppListItem* target_item = target_view->item();
-  bool target_is_folder = IsFolderItem(target_item);
+  std::string target_id = target_view->item()->id();

[xiyuan, 2014/01/28 05:58:29]

nit: const std::string&

[stevenjb, 2014/01/28 18:28:08]

Done.

 
   // Make change to data model.
   item_list_->RemoveObserver(this);
-  int folder_index = MergeItems(item_list_, target_item->id(), source_item);
+  std::string folder_id = model_->MergeItems(target_id, source_id);
   item_list_->AddObserver(this);
 
-  if (!target_is_folder) {
-    // Change view_model_ to replace the old target view with new folder
-    // item view.
-    int target_index = view_model_.GetIndexOfView(target_view);
-    view_model_.Remove(target_index);
-    delete target_view;
-
-    views::View* target_folder_view = CreateViewForItemAtIndex(folder_index);
-    view_model_.Add(target_folder_view, target_index);
-    AddChildView(target_folder_view);
+  if (folder_id != target_id) {
+    // New folder was created, change the view model to replace the old target
+    // view with the new folder item view.
+    size_t folder_index;
+    if (item_list_->FindItemIndex(folder_id, &folder_index)) {
+      int target_index = view_model_.GetIndexOfView(target_view);
+      view_model_.Remove(target_index);
+      delete target_view;
+      views::View* target_folder_view = CreateViewForItemAtIndex(folder_index);
+      view_model_.Add(target_folder_view, target_index);
+      AddChildView(target_folder_view);
+    } else {
+      LOG(ERROR) << "Folder no longer in item_list: " << folder_id;
+    }
   }
 
   // Fade out the drag_view_ and delete it when animation ends.
   int drag_view_index = view_model_.GetIndexOfView(drag_view_);
   view_model_.Remove(drag_view_index);
   bounds_animator_.AnimateViewTo(drag_view_, drag_view_->bounds());
   bounds_animator_.SetAnimationDelegate(
       drag_view_, new ItemRemoveAnimationDelegate(drag_view_), true);
 
   UpdatePaging();
@@ skipping 291 matching lines @@
 void AppsGridView::SetAsFolderDroppingTarget(const Index& target_index,
                                              bool is_target_folder) {
   AppListItemView* target_view =
       static_cast<AppListItemView*>(
           GetViewAtSlotOnCurrentPage(target_index.slot));
   if (target_view)
     target_view->SetAsAttemptedFolderTarget(is_target_folder);
 }
 
 }  // namespace app_list