Protect AppListItemList Add/Remove and fix sync bugs

This should fixe the issues seen in the bug, and includes some cleanup
in prep for syncing folders.

BUG=338520
TBR=jamescook@chromium.org for ash_shell.cc

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

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

17 files changed, 285 insertions, 261 deletions

diff --git a/ui/app_list/app_list_folder_item.cc b/ui/app_list/app_list_folder_item.cc
index 1aa14af..a20fbcd 100644
--- a/ui/app_list/app_list_folder_item.cc
+++ b/ui/app_list/app_list_folder_item.cc
@@ -4,6 +4,7 @@
 
 #include "ui/app_list/app_list_folder_item.h"
 
+#include "base/guid.h"
 #include "ui/app_list/app_list_constants.h"
 #include "ui/app_list/app_list_item_list.h"
 #include "ui/gfx/canvas.h"
@@ -144,6 +145,10 @@ Rects AppListFolderItem::GetTopIconsBounds(
   return top_icon_bounds;
 }
 
+std::string AppListFolderItem::GenerateId() {
+  return base::GenerateGUID();
+}
+
 const char* AppListFolderItem::GetItemType() const {
   return AppListFolderItem::kItemType;
 }
diff --git a/ui/app_list/app_list_folder_item.h b/ui/app_list/app_list_folder_item.h
index 18c890b..02d54ce 100644
--- a/ui/app_list/app_list_folder_item.h
+++ b/ui/app_list/app_list_folder_item.h
@@ -43,6 +43,9 @@ class APP_LIST_EXPORT AppListFolderItem : public AppListItem,
   // bottom left, bottom right.
   static Rects GetTopIconsBounds(const gfx::Rect& folder_icon_bounds);
 
+  // Returns an id for a new folder.
+  static std::string GenerateId();
+
  private:
   // AppListItem
   virtual void Activate(int event_flags) OVERRIDE;
diff --git a/ui/app_list/app_list_item.cc b/ui/app_list/app_list_item.cc
index 9d35f87..70fe6bb 100644
--- a/ui/app_list/app_list_item.cc
+++ b/ui/app_list/app_list_item.cc
@@ -88,6 +88,7 @@ ui::MenuModel* AppListItem::GetContextMenuModel() {
 bool AppListItem::CompareForTest(const AppListItem* other) const {
   return id_ == other->id_ &&
       title_ == other->title_ &&
+      GetItemType() == other->GetItemType() &&
       position_.Equals(other->position_);
 }
 
diff --git a/ui/app_list/app_list_item.h b/ui/app_list/app_list_item.h
index 69139eb..de67a79 100644
--- a/ui/app_list/app_list_item.h
+++ b/ui/app_list/app_list_item.h
@@ -22,6 +22,7 @@ namespace app_list {
 class AppListItemList;
 class AppListItemListTest;
 class AppListItemObserver;
+class AppListModel;
 
 // AppListItem provides icon and title to be shown in a AppListItemView
 // and action to be executed when the AppListItemView is activated.
@@ -73,6 +74,7 @@ class APP_LIST_EXPORT AppListItem {
  protected:
   friend class AppListItemList;
   friend class AppListItemListTest;
+  friend class AppListModel;
 
   void set_position(const syncer::StringOrdinal& new_position) {
     DCHECK(new_position.IsValid());
diff --git a/ui/app_list/app_list_item_list.cc b/ui/app_list/app_list_item_list.cc
index ffcab0f..21db2bf 100644
--- a/ui/app_list/app_list_item_list.cc
+++ b/ui/app_list/app_list_item_list.cc
@@ -42,80 +42,6 @@ bool AppListItemList::FindItemIndex(const std::string& id, size_t* index) {
   return false;
 }
 
-size_t AppListItemList::AddItem(AppListItem* item) {
-  CHECK(std::find(app_list_items_.begin(), app_list_items_.end(), item)
-        == app_list_items_.end());
-  EnsureValidItemPosition(item);
-  size_t index = GetItemSortOrderIndex(item->position(), item->id());
-  app_list_items_.insert(app_list_items_.begin() + index, item);
-  FOR_EACH_OBSERVER(AppListItemListObserver,
-                    observers_,
-                    OnListItemAdded(index, item));
-  return index;
-}
-
-void AppListItemList::InsertItemAt(AppListItem* item, size_t index) {
-  DCHECK_LE(index, item_count());
-  if (item_count() == 0) {
-    AddItem(item);
-    return;
-  }
-
-  AppListItem* prev = index > 0 ? app_list_items_[index - 1] : NULL;
-  AppListItem* next = index <= app_list_items_.size() - 1 ?
-      app_list_items_[index] : NULL;
-  CHECK_NE(prev, next);
-
-  if (prev && next && prev->position().Equals(next->position()))
-    prev = NULL;
-
-  if (!prev)
-    item->set_position(next->position().CreateBefore());
-  else if (!next)
-    item->set_position(prev->position().CreateAfter());
-  else
-    item->set_position(prev->position().CreateBetween(next->position()));
-
-  app_list_items_.insert(app_list_items_.begin() + index, item);
-
-  FOR_EACH_OBSERVER(AppListItemListObserver,
-                    observers_,
-                    OnListItemAdded(index, item));
-}
-
-void AppListItemList::DeleteItem(const std::string& id) {
-  scoped_ptr<AppListItem> item = RemoveItem(id);
-  // |item| will be deleted on destruction.
-}
-
-void AppListItemList::DeleteItemsByType(const char* type) {
-  for (int i = static_cast<int>(app_list_items_.size()) - 1;
-       i >= 0; --i) {
-    AppListItem* item = app_list_items_[i];
-    if (!type || item->GetItemType() == type)
-      DeleteItemAt(i);
-  }
-}
-
-scoped_ptr<AppListItem> AppListItemList::RemoveItem(
-    const std::string& id) {
-  size_t index;
-  if (FindItemIndex(id, &index))
-    return RemoveItemAt(index);
-
-  return scoped_ptr<AppListItem>();
-}
-
-scoped_ptr<AppListItem> AppListItemList::RemoveItemAt(size_t index) {
-  DCHECK_LT(index, item_count());
-  AppListItem* item = app_list_items_[index];
-  app_list_items_.weak_erase(app_list_items_.begin() + index);
-  FOR_EACH_OBSERVER(AppListItemListObserver,
-                    observers_,
-                    OnListItemRemoved(index, item));
-  return make_scoped_ptr<AppListItem>(item);
-}
-
 void AppListItemList::MoveItem(size_t from_index, size_t to_index) {
   DCHECK_LT(from_index, item_count());
   DCHECK_LT(to_index, item_count());
@@ -186,6 +112,44 @@ void AppListItemList::SetItemPosition(
                     OnListItemMoved(from_index, to_index, item));
 }
 
+// AppListItemList protected
+
+size_t AppListItemList::AddItem(AppListItem* item) {
+  CHECK(std::find(app_list_items_.begin(), app_list_items_.end(), item)
+        == app_list_items_.end());
+  EnsureValidItemPosition(item);
+  size_t index = GetItemSortOrderIndex(item->position(), item->id());
+  app_list_items_.insert(app_list_items_.begin() + index, item);
+  FOR_EACH_OBSERVER(AppListItemListObserver,
+                    observers_,
+                    OnListItemAdded(index, item));
+  return index;
+}
+
+void AppListItemList::DeleteItem(const std::string& id) {
+  scoped_ptr<AppListItem> item = RemoveItem(id);
+  // |item| will be deleted on destruction.
+}
+
+scoped_ptr<AppListItem> AppListItemList::RemoveItem(
+    const std::string& id) {
+  size_t index;
+  if (FindItemIndex(id, &index))
+    return RemoveItemAt(index);
+
+  return scoped_ptr<AppListItem>();
+}
+
+scoped_ptr<AppListItem> AppListItemList::RemoveItemAt(size_t index) {
+  DCHECK_LT(index, item_count());
+  AppListItem* item = app_list_items_[index];
+  app_list_items_.weak_erase(app_list_items_.begin() + index);
+  FOR_EACH_OBSERVER(AppListItemListObserver,
+                    observers_,
+                    OnListItemRemoved(index, item));
+  return make_scoped_ptr<AppListItem>(item);
+}
+
 // AppListItemList private
 
 void AppListItemList::DeleteItemAt(size_t index) {
diff --git a/ui/app_list/app_list_item_list.h b/ui/app_list/app_list_item_list.h
index d52acf2..a5fa152 100644
--- a/ui/app_list/app_list_item_list.h
+++ b/ui/app_list/app_list_item_list.h
@@ -37,25 +37,39 @@ class APP_LIST_EXPORT AppListItemList {
   // Note: Requires a linear search.
   bool FindItemIndex(const std::string& id, size_t* index);
 
+  // Moves item at |from_index| to |to_index|.
+  // Triggers observers_.OnListItemMoved().
+  void MoveItem(size_t from_index, size_t to_index);
+
+  // Sets the position of |item| which is expected to be a member of
+  // |app_list_items_| and sorts the list accordingly.
+  void SetItemPosition(AppListItem* item,
+                       const syncer::StringOrdinal& new_position);
+
+  AppListItem* item_at(size_t index) {
+    DCHECK_LT(index, app_list_items_.size());
+    return app_list_items_[index];
+  }
+  const AppListItem* item_at(size_t index) const {
+    DCHECK_LT(index, app_list_items_.size());
+    return app_list_items_[index];
+  }
+  size_t item_count() const { return app_list_items_.size(); }
+
+ private:
+  friend class AppListItemListTest;
+  friend class AppListModel;
+  friend class AppListModelTest;  // TODO(stevenjb): Remove dependency
+
   // Adds |item| to the end of |app_list_items_|. Takes ownership of |item|.
   // Triggers observers_.OnListItemAdded(). Returns the index of the added item.
   size_t AddItem(AppListItem* item);
 
-  // Inserts |item| at the |index| into |app_list_items_|. Takes ownership of
-  // |item|. Triggers observers_.OnListItemAdded().
-  void InsertItemAt(AppListItem* item, size_t index);
-
   // Finds item matching |id| in |app_list_items_| (linear search) and deletes
   // it. Triggers observers_.OnListItemRemoved() after removing the item from
   // the list and before deleting it.
   void DeleteItem(const std::string& id);
 
-  // Deletes all items matching |type| which must be a statically defined
-  // type descriptor, e.g. AppListFolderItem::kItemType. If |type| is NULL,
-  // deletes all items. Triggers observers_.OnListItemRemoved() for each item
-  // as for DeleteItem.
-  void DeleteItemsByType(const char* type);
-
   // Removes the item with matching |id| in |app_list_items_| without deleting
   // it. Returns a scoped pointer containing the removed item.
   scoped_ptr<AppListItem> RemoveItem(const std::string& id);
@@ -64,22 +78,6 @@ class APP_LIST_EXPORT AppListItemList {
   // Returns a scoped pointer containing the removed item.
   scoped_ptr<AppListItem> RemoveItemAt(size_t index);
 
-  // Moves item at |from_index| to |to_index|.
-  // Triggers observers_.OnListItemMoved().
-  void MoveItem(size_t from_index, size_t to_index);
-
-  // Sets the position of |item| which is expected to be a member of
-  // |app_list_items_| and sorts the list accordingly.
-  void SetItemPosition(AppListItem* item,
-                       const syncer::StringOrdinal& new_position);
-
-  AppListItem* item_at(size_t index) {
-    DCHECK_LT(index, app_list_items_.size());
-    return app_list_items_[index];
-  }
-  size_t item_count() const { return app_list_items_.size(); }
-
- private:
   // Deletes item at |index| and signals observers.
   void DeleteItemAt(size_t index);
 
diff --git a/ui/app_list/app_list_item_list_observer.h b/ui/app_list/app_list_item_list_observer.h
index a6debcb..fe8aa24 100644
--- a/ui/app_list/app_list_item_list_observer.h
+++ b/ui/app_list/app_list_item_list_observer.h
@@ -6,6 +6,7 @@
 #define UI_APP_LIST_APP_LIST_ITEM_LIST_OBSERVER_H_
 
 #include "base/basictypes.h"
+#include "ui/app_list/app_list_export.h"
 
 namespace app_list {
 
diff --git a/ui/app_list/app_list_item_list_unittest.cc b/ui/app_list/app_list_item_list_unittest.cc
index b21edba..9dcf065 100644
--- a/ui/app_list/app_list_item_list_unittest.cc
+++ b/ui/app_list/app_list_item_list_unittest.cc
@@ -86,6 +86,14 @@ class AppListItemListTest : public testing::Test {
     return item;
   }
 
+  scoped_ptr<AppListItem> RemoveItem(const std::string& id) {
+    return item_list_.RemoveItem(id);
+  }
+
+  scoped_ptr<AppListItem> RemoveItemAt(size_t index) {
+    return item_list_.RemoveItemAt(index);
+  }
+
   bool VerifyItemListOrdinals() {
     bool res = true;
     for (size_t i = 1; i < item_list_.item_count(); ++i) {
@@ -155,7 +163,7 @@ TEST_F(AppListItemListTest, RemoveItemAt) {
   EXPECT_EQ(index, 1u);
   EXPECT_TRUE(VerifyItemListOrdinals());
 
-  scoped_ptr<AppListItem> item_removed = item_list_.RemoveItemAt(1);
+  scoped_ptr<AppListItem> item_removed = RemoveItemAt(1);
   EXPECT_EQ(item_removed, item_1);
   EXPECT_FALSE(item_list_.FindItem(item_1->id()));
   EXPECT_EQ(item_list_.item_count(), 2u);
@@ -180,71 +188,17 @@ TEST_F(AppListItemListTest, RemoveItem) {
   EXPECT_TRUE(item_list_.FindItemIndex(item_1->id(), &index));
   EXPECT_EQ(index, 1u);
 
-  scoped_ptr<AppListItem> item_removed =
-      item_list_.RemoveItem(item_1->id());
+  scoped_ptr<AppListItem> item_removed = RemoveItem(item_1->id());
   EXPECT_EQ(item_removed, item_1);
   EXPECT_FALSE(item_list_.FindItem(item_1->id()));
   EXPECT_EQ(item_list_.item_count(), 2u);
   EXPECT_EQ(observer_.items_removed(), 1u);
   EXPECT_TRUE(VerifyItemListOrdinals());
 
-  scoped_ptr<AppListItem> not_found_item = item_list_.RemoveItem("Bogus");
+  scoped_ptr<AppListItem> not_found_item = RemoveItem("Bogus");
   EXPECT_FALSE(not_found_item.get());
 }
 
-TEST_F(AppListItemListTest, InsertItemAt) {
-  AppListItem* item_0 = CreateAndAddItem(GetItemName(0), GetItemName(0));
-  AppListItem* item_1 = CreateAndAddItem(GetItemName(1), GetItemName(1));
-  EXPECT_EQ(item_list_.item_count(), 2u);
-  EXPECT_EQ(observer_.items_added(), 2u);
-  EXPECT_EQ(item_list_.item_at(0), item_0);
-  EXPECT_EQ(item_list_.item_at(1), item_1);
-  EXPECT_TRUE(VerifyItemListOrdinals());
-
-  // Insert an item at the beginning of the item_list_.
-  AppListItem* item_2 = CreateItem(GetItemName(2), GetItemName(2));
-  item_list_.InsertItemAt(item_2, 0);
-  EXPECT_EQ(item_list_.item_count(), 3u);
-  EXPECT_EQ(observer_.items_added(), 3u);
-  EXPECT_EQ(item_list_.item_at(0), item_2);
-  EXPECT_EQ(item_list_.item_at(1), item_0);
-  EXPECT_EQ(item_list_.item_at(2), item_1);
-  EXPECT_TRUE(VerifyItemListOrdinals());
-
-  // Insert an item at the end of the item_list_.
-  AppListItem* item_3 = CreateItem(GetItemName(3), GetItemName(3));
-  item_list_.InsertItemAt(item_3, item_list_.item_count());
-  EXPECT_EQ(item_list_.item_count(), 4u);
-  EXPECT_EQ(observer_.items_added(), 4u);
-  EXPECT_EQ(item_list_.item_at(0), item_2);
-  EXPECT_EQ(item_list_.item_at(1), item_0);
-  EXPECT_EQ(item_list_.item_at(2), item_1);
-  EXPECT_EQ(item_list_.item_at(3), item_3);
-  EXPECT_TRUE(VerifyItemListOrdinals());
-
-  // Insert an item at the 2nd item of the item_list_.
-  AppListItem* item_4 = CreateItem(GetItemName(4), GetItemName(4));
-  item_list_.InsertItemAt(item_4, 1);
-  EXPECT_EQ(item_list_.item_count(), 5u);
-  EXPECT_EQ(observer_.items_added(), 5u);
-  EXPECT_EQ(item_list_.item_at(0), item_2);
-  EXPECT_EQ(item_list_.item_at(1), item_4);
-  EXPECT_EQ(item_list_.item_at(2), item_0);
-  EXPECT_EQ(item_list_.item_at(3), item_1);
-  EXPECT_EQ(item_list_.item_at(4), item_3);
-  EXPECT_TRUE(VerifyItemListOrdinals());
-}
-
-TEST_F(AppListItemListTest, InsertItemAtEmptyList) {
-  AppListItem* item_0 = CreateItem(GetItemName(0), GetItemName(0));
-  EXPECT_EQ(item_list_.item_count(), 0u);
-  item_list_.InsertItemAt(item_0, 0);
-  EXPECT_EQ(item_list_.item_count(), 1u);
-  EXPECT_EQ(observer_.items_added(), 1u);
-  EXPECT_EQ(item_list_.item_at(0), item_0);
-  EXPECT_TRUE(VerifyItemListOrdinals());
-}
-
 TEST_F(AppListItemListTest, MoveItem) {
   CreateAndAddItem(GetItemName(0), GetItemName(0));
   CreateAndAddItem(GetItemName(1), GetItemName(1));
diff --git a/ui/app_list/app_list_model.cc b/ui/app_list/app_list_model.cc
index c5515033..246ed8b 100644
--- a/ui/app_list/app_list_model.cc
+++ b/ui/app_list/app_list_model.cc
@@ -4,6 +4,7 @@
 
 #include "ui/app_list/app_list_model.h"
 
+#include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
 #include "ui/app_list/app_list_model_observer.h"
 #include "ui/app_list/search_box_model.h"
@@ -16,9 +17,11 @@ AppListModel::AppListModel()
       search_box_(new SearchBoxModel),
       results_(new SearchResults),
       status_(STATUS_NORMAL) {
+  item_list_->AddObserver(this);
 }
 
 AppListModel::~AppListModel() {
+  item_list_->RemoveObserver(this);
 }
 
 void AppListModel::AddObserver(AppListModelObserver* observer) {
@@ -39,4 +42,96 @@ void AppListModel::SetStatus(Status status) {
                     OnAppListModelStatusChanged());
 }
 
+AppListItem* AppListModel::FindItem(const std::string& id) {
+  return item_list_->FindItem(id);
+}
+
+void AppListModel::AddItem(AppListItem* item) {
+  DCHECK(!item_list()->FindItem(item->id()));
+  AddItemToItemList(item);
+}
+
+const std::string& AppListModel::MergeItems(const std::string& target_item_id,
+                                            const std::string& source_item_id) {
+  // First, remove the source item from |item_list_|.
+  scoped_ptr<AppListItem> source_item_ptr =
+      item_list_->RemoveItem(source_item_id);
+
+  // Next, find the target item.
+  AppListItem* target_item = item_list_->FindItem(target_item_id);
+  DCHECK(target_item);
+
+  // If the target item is a folder, just add |source_item| to it.
+  if (target_item->GetItemType() == AppListFolderItem::kItemType) {
+    AppListFolderItem* target_folder =
+        static_cast<AppListFolderItem*>(target_item);
+    AddItemToFolder(target_folder, source_item_ptr.release());
+    return target_folder->id();
+  }
+
+  // Otherwise, remove the target item from |item_list_|, it will become owned
+  // by the new folder.
+  scoped_ptr<AppListItem> target_item_ptr =
+      item_list_->RemoveItem(target_item_id);
+
+  // Create a new folder in the same location as the target item.
+  AppListFolderItem* new_folder =
+      new AppListFolderItem(AppListFolderItem::GenerateId());
+  new_folder->set_position(target_item->position());
+  AddItemToItemList(new_folder);
+
+  // Add the items to the new folder.
+  AddItemToFolder(new_folder, target_item_ptr.release());
+  AddItemToFolder(new_folder, source_item_ptr.release());
+
+  return new_folder->id();
+}
+
+void AppListModel::SetItemPosition(AppListItem* item,
+                                   const syncer::StringOrdinal& new_position) {
+  item_list_->SetItemPosition(item, new_position);
+  // Note: this will trigger OnListItemMoved which will signal observers.
+  // (This is done this way because some View code still moves items within
+  // the item list directly).
+}
+
+void AppListModel::DeleteItem(const std::string& id) {
+  AppListItem* item = FindItem(id);
+  if (!item)
+    return;
+  FOR_EACH_OBSERVER(AppListModelObserver,
+                    observers_,
+                    OnAppListItemWillBeDeleted(item));
+  item_list_->DeleteItem(id);
+}
+
+// Private methods
+
+void AppListModel::OnListItemMoved(size_t from_index,
+                                   size_t to_index,
+                                   AppListItem* item) {
+  FOR_EACH_OBSERVER(AppListModelObserver,
+                    observers_,
+                    OnAppListItemUpdated(item));
+}
+
+syncer::StringOrdinal AppListModel::GetLastItemPosition() {
+  size_t count = item_list_->item_count();
+  if (count == 0)
+    return syncer::StringOrdinal::CreateInitialOrdinal();
+  return item_list_->item_at(count - 1)->position();
+}
+
+void AppListModel::AddItemToItemList(AppListItem* item) {
+  item_list_->AddItem(item);
+  FOR_EACH_OBSERVER(AppListModelObserver,
+                    observers_,
+                    OnAppListItemAdded(item));
+}
+
+void AppListModel::AddItemToFolder(AppListFolderItem* folder,
+                                   AppListItem* item) {
+  folder->item_list()->AddItem(item);
+}
+
 }  // namespace app_list
diff --git a/ui/app_list/app_list_model.h b/ui/app_list/app_list_model.h
index 76619ec..38b1e85 100644
--- a/ui/app_list/app_list_model.h
+++ b/ui/app_list/app_list_model.h
@@ -10,10 +10,12 @@
 #include "base/observer_list.h"
 #include "ui/app_list/app_list_export.h"
 #include "ui/app_list/app_list_item_list.h"
+#include "ui/app_list/app_list_item_list_observer.h"
 #include "ui/base/models/list_model.h"
 
 namespace app_list {
 
+class AppListFolderItem;
 class AppListItem;
 class AppListItemList;
 class AppListModelObserver;
@@ -24,7 +26,11 @@ class SearchResult;
 // SearchBoxModel and SearchResults. The AppListItemList sub model owns a list
 // of AppListItems and is displayed in the grid view. SearchBoxModel is
 // the model for SearchBoxView. SearchResults owns a list of SearchResult.
-class APP_LIST_EXPORT AppListModel {
+// NOTE: Currently this class observes |item_list_|. The View code may
+// move entries in the item list directly (but can not add or remove them) and
+// the model needs to notify its observers when this occurs.
+
+class APP_LIST_EXPORT AppListModel : public AppListItemListObserver {
  public:
   enum Status {
     STATUS_NORMAL,
@@ -34,19 +40,52 @@ class APP_LIST_EXPORT AppListModel {
   typedef ui::ListModel<SearchResult> SearchResults;
 
   AppListModel();
-  ~AppListModel();
+  virtual ~AppListModel();
 
   void AddObserver(AppListModelObserver* observer);
   void RemoveObserver(AppListModelObserver* observer);
 
   void SetStatus(Status status);
 
+  // Finds the item matching |id|.
+  AppListItem* FindItem(const std::string& id);
+
+  // Adds |item| to the model. The model takes ownership of the item.
+  void AddItem(AppListItem* item);
+
+  // Merges two items. If the target item is a folder, the source item is added
+  // to that folder, otherwise a new folder is created in the same position as
+  // the target item.  Returns the id of the target folder.
+  const std::string& MergeItems(const std::string& target_item_id,
+                                const std::string& source_item_id);
+
+  // Sets the position of |item| in |item_list_|.
+  void SetItemPosition(AppListItem* item,
+                       const syncer::StringOrdinal& new_position);
+
+  // Deletes the item matching |id| from |item_list_|.
+  void DeleteItem(const std::string& id);
+
   AppListItemList* item_list() { return item_list_.get(); }
   SearchBoxModel* search_box() { return search_box_.get(); }
   SearchResults* results() { return results_.get(); }
   Status status() const { return status_; }
 
  private:
+  // AppListItemListObserver
+  virtual void OnListItemMoved(size_t from_index,
+                               size_t to_index,
+                               AppListItem* item) OVERRIDE;
+
+  // Returns the position of the last item in |item_list_| or an initial one.
+  syncer::StringOrdinal GetLastItemPosition();
+
+  // Adds |item| to |item_list_| and notifies observers.
+  void AddItemToItemList(AppListItem* item);
+
+  // Adds |item| to |folder|.
+  void AddItemToFolder(AppListFolderItem* folder, AppListItem* item);
+
   scoped_ptr<AppListItemList> item_list_;
   scoped_ptr<SearchBoxModel> search_box_;
   scoped_ptr<SearchResults> results_;
diff --git a/ui/app_list/app_list_model_observer.h b/ui/app_list/app_list_model_observer.h
index 8810c82..cc3d08e 100644
--- a/ui/app_list/app_list_model_observer.h
+++ b/ui/app_list/app_list_model_observer.h
@@ -9,11 +9,22 @@
 
 namespace app_list {
 
+class AppListItem;
+
 class APP_LIST_EXPORT AppListModelObserver {
  public:
   // Invoked when AppListModel's status has changed.
   virtual void OnAppListModelStatusChanged() {}
 
+  // Triggered after |item| has been added to the model.
+  virtual void OnAppListItemAdded(AppListItem* item) {}
+
+  // Triggered just before an item is deleted from the model.
+  virtual void OnAppListItemWillBeDeleted(AppListItem* item) {}
+
+  // Triggered after |item| has moved or changed folders.
+  virtual void OnAppListItemUpdated(AppListItem* item) {}
+
  protected:
   virtual ~AppListModelObserver() {}
 };
diff --git a/ui/app_list/app_list_model_unittest.cc b/ui/app_list/app_list_model_unittest.cc
index 339a4ab7..3c2df89 100644
--- a/ui/app_list/app_list_model_unittest.cc
+++ b/ui/app_list/app_list_model_unittest.cc
@@ -7,11 +7,13 @@
 #include <map>
 #include <string>
 
+#include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
 #include "ui/app_list/app_list_model_observer.h"
+#include "ui/app_list/app_list_switches.h"
 #include "ui/app_list/test/app_list_test_model.h"
 #include "ui/base/models/list_model_observer.h"
 
@@ -19,8 +21,7 @@ namespace app_list {
 
 namespace {
 
-class TestObserver : public AppListModelObserver,
-                     public AppListItemListObserver {
+class TestObserver : public AppListModelObserver {
  public:
   TestObserver()
       : status_changed_count_(0),
@@ -36,30 +37,25 @@ class TestObserver : public AppListModelObserver,
     ++status_changed_count_;
   }
 
-  // AppListItemListObserver
-  virtual void OnListItemAdded(size_t index, AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemAdded(AppListItem* item) OVERRIDE {
     items_added_++;
   }
 
-  virtual void OnListItemRemoved(size_t index, AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemWillBeDeleted(AppListItem* item) OVERRIDE {
     items_removed_++;
   }
 
-  virtual void OnListItemMoved(size_t from_index,
-                               size_t to_index,
-                               AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemUpdated(AppListItem* item) OVERRIDE {
     items_moved_++;
   }
 
   int status_changed_count() const { return status_changed_count_; }
-  int signin_changed_count() const { return signin_changed_count_; }
   size_t items_added() { return items_added_; }
   size_t items_removed() { return items_removed_; }
   size_t items_moved() { return items_moved_; }
 
   void ResetCounts() {
     status_changed_count_ = 0;
-    signin_changed_count_ = 0;
     items_added_ = 0;
     items_removed_ = 0;
     items_moved_ = 0;
@@ -67,7 +63,6 @@ class TestObserver : public AppListModelObserver,
 
  private:
   int status_changed_count_;
-  int signin_changed_count_;
   size_t items_added_;
   size_t items_removed_;
   size_t items_moved_;
@@ -85,11 +80,9 @@ class AppListModelTest : public testing::Test {
   // testing::Test overrides:
   virtual void SetUp() OVERRIDE {
     model_.AddObserver(&observer_);
-    model_.item_list()->AddObserver(&observer_);
   }
   virtual void TearDown() OVERRIDE {
     model_.RemoveObserver(&observer_);
-    model_.item_list()->RemoveObserver(&observer_);
   }
 
  protected:
@@ -98,6 +91,11 @@ class AppListModelTest : public testing::Test {
     return item->observers_.HasObserver(folder);
   }
 
+  void AddFolderItem(AppListFolderItem* folder,
+                     const std::string& name) {
+    folder->item_list()->AddItem(model_.CreateItem(name, name));
+  }
+
   test::AppListTestModel model_;
   TestObserver observer_;
 
@@ -138,11 +136,11 @@ TEST_F(AppListModelTest, ModelFindItem) {
   const size_t num_apps = 2;
   model_.PopulateApps(num_apps);
   std::string item_name0 = model_.GetItemName(0);
-  AppListItem* item0 = model_.item_list()->FindItem(item_name0);
+  AppListItem* item0 = model_.FindItem(item_name0);
   ASSERT_TRUE(item0);
   EXPECT_EQ(item_name0, item0->id());
   std::string item_name1 = model_.GetItemName(1);
-  AppListItem* item1 = model_.item_list()->FindItem(item_name1);
+  AppListItem* item1 = model_.FindItem(item_name1);
   ASSERT_TRUE(item1);
   EXPECT_EQ(item_name1, item1->id());
 }
@@ -160,10 +158,11 @@ TEST_F(AppListModelTest, ModelAddItem) {
   AppListItem* item1 = model_.item_list()->item_at(1);
   ASSERT_TRUE(item1);
   AppListItem* item2 = model_.CreateItem("Added Item 2", "Added Item 2");
-  model_.item_list()->AddItem(item2);
-  model_.item_list()->SetItemPosition(
+  model_.AddItem(item2);
+  model_.SetItemPosition(
       item2, item0->position().CreateBetween(item1->position()));
   EXPECT_EQ(num_apps + 2, model_.item_list()->item_count());
+  EXPECT_EQ(num_apps + 2, observer_.items_added());
   EXPECT_EQ("Added Item 2", model_.item_list()->item_at(1)->id());
 }
 
@@ -175,6 +174,7 @@ TEST_F(AppListModelTest, ModelMoveItem) {
   ASSERT_EQ(num_apps + 1, model_.item_list()->item_count());
   // Move it to the position 1.
   model_.item_list()->MoveItem(num_apps, 1);
+  EXPECT_EQ(1u, observer_.items_moved());
   AppListItem* item = model_.item_list()->item_at(1);
   ASSERT_TRUE(item);
   EXPECT_EQ("Inserted Item", item->id());
@@ -184,15 +184,15 @@ TEST_F(AppListModelTest, ModelRemoveItem) {
   const size_t num_apps = 4;
   model_.PopulateApps(num_apps);
   // Remove an item in the middle.
-  model_.item_list()->DeleteItem(model_.GetItemName(1));
+  model_.DeleteItem(model_.GetItemName(1));
   EXPECT_EQ(num_apps - 1, model_.item_list()->item_count());
   EXPECT_EQ(1u, observer_.items_removed());
   // Remove the first item in the list.
-  model_.item_list()->DeleteItem(model_.GetItemName(0));
+  model_.DeleteItem(model_.GetItemName(0));
   EXPECT_EQ(num_apps - 2, model_.item_list()->item_count());
   EXPECT_EQ(2u, observer_.items_removed());
   // Remove the last item in the list.
-  model_.item_list()->DeleteItem(model_.GetItemName(num_apps - 1));
+  model_.DeleteItem(model_.GetItemName(num_apps - 1));
   EXPECT_EQ(num_apps - 3, model_.item_list()->item_count());
   EXPECT_EQ(3u, observer_.items_removed());
   // Ensure that the first item is the expected one
@@ -201,27 +201,6 @@ TEST_F(AppListModelTest, ModelRemoveItem) {
   EXPECT_EQ(model_.GetItemName(2), item0->id());
 }
 
-TEST_F(AppListModelTest, ModelRemoveItemByType) {
-  const size_t num_apps = 4;
-  model_.PopulateApps(num_apps);
-  model_.item_list()->AddItem(new AppListFolderItem("folder1"));
-  model_.item_list()->AddItem(new AppListFolderItem("folder2"));
-  model_.item_list()->DeleteItemsByType(test::AppListTestModel::kItemType);
-  EXPECT_EQ(num_apps, observer_.items_removed());
-  EXPECT_EQ(2u, model_.item_list()->item_count());
-  model_.item_list()->DeleteItemsByType(AppListFolderItem::kItemType);
-  EXPECT_EQ(num_apps + 2, observer_.items_removed());
-  EXPECT_EQ(0u, model_.item_list()->item_count());
-  // Delete all items
-  observer_.ResetCounts();
-  model_.PopulateApps(num_apps);
-  model_.item_list()->AddItem(new AppListFolderItem("folder1"));
-  model_.item_list()->AddItem(new AppListFolderItem("folder2"));
-  model_.item_list()->DeleteItemsByType(NULL /* all items */);
-  EXPECT_EQ(num_apps + 2, observer_.items_removed());
-  EXPECT_EQ(0u, model_.item_list()->item_count());
-}
-
 TEST_F(AppListModelTest, AppOrder) {
   const size_t num_apps = 5;
   model_.PopulateApps(num_apps);
@@ -245,7 +224,7 @@ TEST_F(AppListModelTest, FolderItem) {
   const size_t num_observed_apps = 4;
   for (int i = 0; static_cast<size_t>(i) < num_folder_apps; ++i) {
     std::string name = model_.GetItemName(i);
-    folder->item_list()->AddItem(model_.CreateItem(name, name));
+    AddFolderItem(folder.get(), name);
   }
   // Check that items 0 and 3 are observed.
   EXPECT_TRUE(ItemObservedByFolder(
diff --git a/ui/app_list/app_list_switches.h b/ui/app_list/app_list_switches.h
index c3d759e..22216be 100644
--- a/ui/app_list/app_list_switches.h
+++ b/ui/app_list/app_list_switches.h
@@ -13,7 +13,7 @@ namespace switches {
 APP_LIST_EXPORT extern const char kEnableFolderUI[];
 APP_LIST_EXPORT extern const char kDisableVoiceSearch[];
 
-bool IsFolderUIEnabled();
+bool APP_LIST_EXPORT IsFolderUIEnabled();
 
 bool APP_LIST_EXPORT IsVoiceSearchEnabled();
 
diff --git a/ui/app_list/cocoa/apps_grid_controller_unittest.mm b/ui/app_list/cocoa/apps_grid_controller_unittest.mm
index a015246..e12c80a 100644
--- a/ui/app_list/cocoa/apps_grid_controller_unittest.mm
+++ b/ui/app_list/cocoa/apps_grid_controller_unittest.mm
@@ -486,7 +486,7 @@ TEST_F(AppsGridControllerTest, ModelAdd) {
   app_list::AppListItem* item1 = item_list->item_at(1);
   app_list::AppListItem* item3 =
       model()->CreateItem("Item Three", "Item Three");
-  item_list->AddItem(item3);
+  model()->AddItem(item3);
   item_list->SetItemPosition(
       item3, item0->position().CreateBetween(item1->position()));
   EXPECT_EQ(4u, [apps_grid_controller_ itemCount]);
@@ -509,7 +509,7 @@ TEST_F(AppsGridControllerTest, ModelRemove) {
   EXPECT_EQ(std::string("|Item 0,Item 1,Item 2|"), GetViewContent());
 
   // Test removing an item at the end.
-  model()->item_list()->DeleteItem("Item 2");
+  model()->DeleteItem("Item 2");
   EXPECT_EQ(2u, [apps_grid_controller_ itemCount]);
   EXPECT_EQ(std::string("|Item 0,Item 1|"), GetViewContent());
 
@@ -517,18 +517,20 @@ TEST_F(AppsGridControllerTest, ModelRemove) {
   model()->CreateAndAddItem("Item 2");
   EXPECT_EQ(3u, [apps_grid_controller_ itemCount]);
   EXPECT_EQ(std::string("|Item 0,Item 1,Item 2|"), GetViewContent());
-  model()->item_list()->DeleteItem("Item 1");
+  model()->DeleteItem("Item 1");
   EXPECT_EQ(2u, [apps_grid_controller_ itemCount]);
   EXPECT_EQ(std::string("|Item 0,Item 2|"), GetViewContent());
 }
 
-TEST_F(AppsGridControllerTest, ModelRemoveAlll) {
+TEST_F(AppsGridControllerTest, ModelRemoveSeveral) {
   model()->PopulateApps(3);
   EXPECT_EQ(3u, [[GetPageAt(0) content] count]);
   EXPECT_EQ(std::string("|Item 0,Item 1,Item 2|"), GetViewContent());
 
   // Test removing multiple items via the model.
-  model()->item_list()->DeleteItemsByType(NULL /* all items */);
+  model()->DeleteItem("Item 0");
+  model()->DeleteItem("Item 1");
+  model()->DeleteItem("Item 2");
   EXPECT_EQ(0u, [apps_grid_controller_ itemCount]);
   EXPECT_EQ(std::string("||"), GetViewContent());
 }
@@ -543,7 +545,7 @@ TEST_F(AppsGridControllerTest, ModelRemovePage) {
 
   // Test removing the last item when there is one item on the second page.
   app_list::AppListItem* last_item = item_list->item_at(kItemsPerPage);
-  item_list->DeleteItem(last_item->id());
+  model()->DeleteItem(last_item->id());
   EXPECT_EQ(kItemsPerPage, item_list->item_count());
   EXPECT_EQ(kItemsPerPage, [apps_grid_controller_ itemCount]);
   EXPECT_EQ(1u, [apps_grid_controller_ pageCount]);
@@ -962,8 +964,8 @@ TEST_F(AppsGridControllerTest, ScrollingWhileDragging) {
 
 TEST_F(AppsGridControllerTest, ContextMenus) {
   AppListItemWithMenu* item_two_model = new AppListItemWithMenu("Item Two");
-  model()->item_list()->AddItem(new AppListItemWithMenu("Item One"));
-  model()->item_list()->AddItem(item_two_model);
+  model()->AddItem(new AppListItemWithMenu("Item One"));
+  model()->AddItem(item_two_model);
   EXPECT_EQ(2u, [apps_grid_controller_ itemCount]);
 
   NSCollectionView* page = [apps_grid_controller_ collectionViewAtPageIndex:0];
diff --git a/ui/app_list/test/app_list_test_model.cc b/ui/app_list/test/app_list_test_model.cc
index d75f5c6..d29fc25 100644
--- a/ui/app_list/test/app_list_test_model.cc
+++ b/ui/app_list/test/app_list_test_model.cc
@@ -84,7 +84,7 @@ AppListTestModel::AppListTestItemModel* AppListTestModel::CreateItem(
 
 void AppListTestModel::CreateAndAddItem(const std::string& title,
                                         const std::string& full_name) {
-  item_list()->AddItem(CreateItem(title, full_name));
+  AddItem(CreateItem(title, full_name));
 }
 
 void AppListTestModel::CreateAndAddItem(const std::string& title) {
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index 3a9285e..a4ac656 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -182,39 +182,6 @@ 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)
@@ -1367,28 +1334,31 @@ void AppsGridView::MoveItemInModel(views::View* item_view,
 
 void AppsGridView::MoveItemToFolder(views::View* item_view,
                                     const Index& target) {
-  AppListItem* source_item =
-      static_cast<AppListItemView*>(item_view)->item();
+  const std::string& source_id =
+      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);
+  const std::string&  target_id = target_view->item()->id();
 
   // 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.
diff --git a/ui/app_list/views/apps_grid_view_unittest.cc b/ui/app_list/views/apps_grid_view_unittest.cc
index 2e43d21..fc7c470 100644
--- a/ui/app_list/views/apps_grid_view_unittest.cc
+++ b/ui/app_list/views/apps_grid_view_unittest.cc
@@ -218,7 +218,7 @@ TEST_F(AppsGridViewTest, RemoveSelectedLastApp) {
 
   AppListItemView* last_view = GetItemViewAt(kLastItemIndex);
   apps_grid_view_->SetSelectedView(last_view);
-  model_->item_list()->DeleteItem(model_->GetItemName(kLastItemIndex));
+  model_->DeleteItem(model_->GetItemName(kLastItemIndex));
 
   EXPECT_FALSE(apps_grid_view_->IsSelectedView(last_view));
 
@@ -253,7 +253,7 @@ TEST_F(AppsGridViewTest, MouseDrag) {
 
   // Deleting an item keeps remaining intact.
   SimulateDrag(AppsGridView::MOUSE, from, to);
-  model_->item_list()->DeleteItem(model_->GetItemName(0));
+  model_->DeleteItem(model_->GetItemName(0));
   apps_grid_view_->EndDrag(false);
   EXPECT_EQ(std::string("Item 1,Item 2,Item 3"),
             model_->GetModelContent());