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

ash/shell/app_list.cc

@@ -198,19 +198,19 @@ class ExampleAppListViewDelegate : public app_list::AppListViewDelegate {
  public:
   ExampleAppListViewDelegate()
       : model_(new app_list::AppListModel) {
-    PopulateApps(model_->item_list());
+    PopulateApps();
     DecorateSearchBox(model_->search_box());
   }
 
  private:
-  void PopulateApps(app_list::AppListItemList* item_list) {
+  void PopulateApps() {
     for (int i = 0;
          i < static_cast<int>(WindowTypeLauncherItem::LAST_TYPE);
          ++i) {
       WindowTypeLauncherItem::Type type =
           static_cast<WindowTypeLauncherItem::Type>(i);
       std::string id = base::StringPrintf("%d", i);
-      item_list->AddItem(new WindowTypeLauncherItem(id, type));
+      model_->AddItem(new WindowTypeLauncherItem(id, type));
     }
   }
 

chrome/browser/sync/test/integration/sync_app_list_helper.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/sync/test/integration/sync_app_list_helper.h"
 
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
@@ -12,11 +13,13 @@
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/common/extensions/sync_helper.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.h"
 
-using app_list::AppListItemList;
+using app_list::AppListFolderItem;
 using app_list::AppListItem;
+using app_list::AppListItemList;
 using app_list::AppListSyncableService;
 using app_list::AppListSyncableServiceFactory;
 
@@ -98,11 +101,15 @@ bool SyncAppListHelper::AllProfilesHaveSameAppListAsVerifier() {
     }
   }
   if (!res) {
-    VLOG(1) << "Verifier:";
+    Profile* verifier = test_->verifier();
+    VLOG(1) << "Verifier: "
+            << AppListSyncableServiceFactory::GetForProfile(verifier);
     PrintAppList(test_->verifier());
     for (int i = 0; i < test_->num_clients(); ++i) {
-      VLOG(1) << "Profile: " << i;
-      PrintAppList(test_->GetProfile(i));
+      Profile* profile = test_->GetProfile(i);
+      VLOG(1) << "Profile: " << i << ": "
+              << AppListSyncableServiceFactory::GetForProfile(profile);
+      PrintAppList(profile);
     }
   }
   return res;
@@ -130,13 +137,32 @@ void SyncAppListHelper::PrintAppList(Profile* profile) {
       AppListSyncableServiceFactory::GetForProfile(profile);
   for (size_t i = 0; i < service->model()->item_list()->item_count(); ++i) {
     AppListItem* item = service->model()->item_list()->item_at(i);
+    std::string label =
+        base::StringPrintf("Item(%d): ", static_cast<int>(i));
+    PrintItem(profile, item, label);
+  }
+}
+
+void SyncAppListHelper::PrintItem(Profile* profile,
+                                  AppListItem* item,
+                                  const std::string& label) {
   extensions::AppSorting* s =
       extensions::ExtensionSystem::Get(profile)->extension_service()->
       extension_prefs()->app_sorting();
   std::string id = item->id();
+  if (item->GetItemType() == AppListFolderItem::kItemType) {
+    VLOG(1) << label << item->ToDebugString();
+    AppListFolderItem* folder = static_cast<AppListFolderItem*>(item);
+    for (size_t i = 0; i < folder->item_list()->item_count(); ++i) {
+      AppListItem* child = folder->item_list()->item_at(i);
+      std::string child_label =
+          base::StringPrintf(" Folder Item(%d): ", static_cast<int>(i));
+      PrintItem(profile, child, child_label);
+    }
+    return;
+  }
   VLOG(1)
-        << "Item(" << i << "): " << item->ToDebugString()
+      << label << item->ToDebugString()
       << " Page: " << s->GetPageOrdinal(id).ToDebugString().substr(0, 8)
       << " Item: " << s->GetAppLaunchOrdinal(id).ToDebugString().substr(0, 8);
-  }
 }

chrome/browser/sync/test/integration/sync_app_list_helper.h

@@ -14,6 +14,10 @@
 class Profile;
 class SyncTest;
 
+namespace app_list {
+class AppListItem;
+}
+
 class SyncAppListHelper {
  public:
   // Singleton implementation.
@@ -46,6 +50,11 @@ class SyncAppListHelper {
   // and the app list entries all have the same state.
   bool AppListMatchesVerifier(Profile* profile);
 
+  // Helper function for debugging, logs info for an item.
+  void PrintItem(Profile* profile,
+                 app_list::AppListItem* item,
+                 const std::string& label);
+
   SyncTest* test_;
   bool setup_completed_;
 

chrome/browser/ui/app_list/app_list_syncable_service.cc

@@ -23,6 +23,8 @@
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
 #include "ui/app_list/app_list_model.h"
+#include "ui/app_list/app_list_model_observer.h"
+#include "ui/app_list/app_list_switches.h"
 
 using syncer::SyncChange;
 
@@ -32,7 +34,7 @@ namespace {
 
 bool SyncAppListEnabled() {
   return CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableSyncAppList);
+      ::switches::kEnableSyncAppList);
 }
 
 void UpdateSyncItemFromSync(const sync_pb::AppListSpecifics& specifics,
@@ -123,38 +125,41 @@ AppListSyncableService::SyncItem::SyncItem(
 AppListSyncableService::SyncItem::~SyncItem() {
 }
 
-// AppListSyncableService::ItemListObserver
+// AppListSyncableService::ModelObserver
 
-class AppListSyncableService::ItemListObserver
-    : public AppListItemListObserver {
+class AppListSyncableService::ModelObserver : public AppListModelObserver {
  public:
-  explicit ItemListObserver(AppListSyncableService* owner) : owner_(owner) {
-    owner_->model()->item_list()->AddObserver(this);
+  explicit ModelObserver(AppListSyncableService* owner)
+      : owner_(owner) {
+    DVLOG(2) << owner_ << ": ModelObserver Added";
+    owner_->model()->AddObserver(this);
   }
 
-  virtual ~ItemListObserver() {
-    owner_->model()->item_list()->RemoveObserver(this);
+  virtual ~ModelObserver() {
+    owner_->model()->RemoveObserver(this);
+    DVLOG(2) << owner_ << ": ModelObserver Removed";
   }
 
  private:
-  // AppListItemListObserver
-  virtual void OnListItemAdded(size_t index, AppListItem* item) OVERRIDE {
+  // AppListModelObserver
+  virtual void OnAppListItemAdded(AppListItem* item) OVERRIDE {
+    DVLOG(2) << owner_ << " OnAppListItemAdded: " << item->ToDebugString();
     owner_->AddOrUpdateFromSyncItem(item);
   }
 
-  virtual void OnListItemRemoved(size_t index, AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemWillBeDeleted(AppListItem* item) OVERRIDE {
+    DVLOG(2) << owner_ << " OnAppListItemDeleted: " << item->ToDebugString();
     owner_->RemoveSyncItem(item->id());
   }
 
-  virtual void OnListItemMoved(size_t from_index,
-                               size_t to_index,
-                               AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemUpdated(AppListItem* item) OVERRIDE {
+    DVLOG(2) << owner_ << " OnAppListItemUpdated: " << item->ToDebugString();
     owner_->UpdateSyncItem(item);
   }
 
   AppListSyncableService* owner_;
 
-  DISALLOW_COPY_AND_ASSIGN(ItemListObserver);
+  DISALLOW_COPY_AND_ASSIGN(ModelObserver);
 };
 
 // AppListSyncableService
@@ -170,9 +175,9 @@ AppListSyncableService::AppListSyncableService(
     return;
   }
 
-  if (SyncAppListEnabled())
-    item_list_observer_.reset(new ItemListObserver(this));
-
+  // Note: model_observer_ is constructed after the initial sync changes are
+  // received in MergeDataAndStartSyncing(). Changes to the model before that
+  // will be synced after the initial sync occurs.
   if (extension_system->extension_service()->is_ready()) {
     BuildModel();
     return;
@@ -185,7 +190,7 @@ AppListSyncableService::AppListSyncableService(
 
 AppListSyncableService::~AppListSyncableService() {
   // Remove observers.
-  item_list_observer_.reset();
+  model_observer_.reset();
 
   STLDeleteContainerPairSecondPointers(sync_items_.begin(), sync_items_.end());
 }
@@ -232,20 +237,15 @@ AppListSyncableService::GetSyncItem(const std::string& id) const {
 }
 
 void AppListSyncableService::AddItem(AppListItem* app_item) {
-  SyncItem* sync_item = AddOrUpdateSyncItem(app_item);
+  SyncItem* sync_item = FindOrAddSyncItem(app_item);
   if (!sync_item)
     return;  // Item is not valid.
 
   DVLOG(1) << this << ": AddItem: " << sync_item->ToString();
-
-  // Add the item to the model if necessary.
-  if (!model_->item_list()->FindItem(app_item->id()))
-    model_->item_list()->AddItem(app_item);
-  else
-    model_->item_list()->SetItemPosition(app_item, sync_item->item_ordinal);
+  model_->AddItem(app_item);
 }
 
-AppListSyncableService::SyncItem* AppListSyncableService::AddOrUpdateSyncItem(
+AppListSyncableService::SyncItem* AppListSyncableService::FindOrAddSyncItem(
     AppListItem* app_item) {
   const std::string& item_id = app_item->id();
   if (item_id.empty()) {
@@ -254,11 +254,10 @@ AppListSyncableService::SyncItem* AppListSyncableService::AddOrUpdateSyncItem(
   }
   SyncItem* sync_item = FindSyncItem(item_id);
   if (sync_item) {
-    // If there is an existing, non-REMOVE_DEFAULT entry, update it.
+    // If there is an existing, non-REMOVE_DEFAULT entry, return it.
     if (sync_item->item_type !=
         sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
       DVLOG(2) << this << ": AddItem already exists: " << sync_item->ToString();
-      UpdateSyncItem(app_item);
       return sync_item;
     }
 
@@ -343,7 +342,7 @@ void AppListSyncableService::UpdateSyncItem(AppListItem* app_item) {
 
 void AppListSyncableService::RemoveItem(const std::string& id) {
   RemoveSyncItem(id);
-  model_->item_list()->DeleteItem(id);
+  model_->DeleteItem(id);
 }
 
 void AppListSyncableService::RemoveSyncItem(const std::string& id) {
@@ -435,6 +434,9 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
   }
   sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
 
+  // Start observing app list model changes.
+  model_observer_.reset(new ModelObserver(this));
+
   return result;
 }
 
@@ -469,6 +471,9 @@ syncer::SyncError AppListSyncableService::ProcessSyncChanges(
                              syncer::APP_LIST);
   }
 
+  // Don't observe the model while processing incoming sync changes.
+  model_observer_.reset();
+
   DVLOG(1) << this << ": ProcessSyncChanges: " << change_list.size();
   for (syncer::SyncChangeList::const_iterator iter = change_list.begin();
        iter != change_list.end(); ++iter) {
@@ -485,6 +490,10 @@ syncer::SyncError AppListSyncableService::ProcessSyncChanges(
       LOG(ERROR) << "Invalid sync change";
     }
   }
+
+  // Continue observing app list model changes.
+  model_observer_.reset(new ModelObserver(this));
+
   return syncer::SyncError();
 }
 
@@ -514,7 +523,7 @@ bool AppListSyncableService::ProcessSyncItemSpecifics(
       LOG(ERROR) << "Synced item type: " << specifics.item_type()
                  << " != existing sync item type: " << sync_item->item_type
                  << " Deleting item from model!";
-      model_->item_list()->DeleteItem(item_id);
+      model_->DeleteItem(item_id);
     }
     DVLOG(2) << this << " - ProcessSyncItem: Delete existing entry: "
              << sync_item->ToString();
@@ -555,7 +564,7 @@ void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
       return;
     }
   }
-  NOTREACHED() << "Unrecoginized sync item type: " << sync_item->ToString();
+  NOTREACHED() << "Unrecognized sync item type: " << sync_item->ToString();
 }
 
 void AppListSyncableService::ProcessExistingSyncItem(SyncItem* sync_item) {
@@ -564,7 +573,8 @@ void AppListSyncableService::ProcessExistingSyncItem(SyncItem* sync_item) {
     return;
   }
   DVLOG(2) << "ProcessExistingSyncItem: " << sync_item->ToString();
-  AppListItem* app_item = model_->item_list()->FindItem(sync_item->item_id);
+  AppListItem* app_item = model_->FindItem(sync_item->item_id);
+  DVLOG(2) << " AppItem: " << app_item->ToDebugString();
   if (!app_item) {
     LOG(ERROR) << "Item not found in model: " << sync_item->ToString();
     return;
@@ -576,7 +586,7 @@ void AppListSyncableService::UpdateAppItemFromSyncItem(
     const AppListSyncableService::SyncItem* sync_item,
     AppListItem* app_item) {
   if (!app_item->position().Equals(sync_item->item_ordinal))
-    model_->item_list()->SetItemPosition(app_item, sync_item->item_ordinal);
+    model_->SetItemPosition(app_item, sync_item->item_ordinal);
 }
 
 bool AppListSyncableService::SyncStarted() {
@@ -643,7 +653,7 @@ void AppListSyncableService::DeleteSyncItemSpecifics(
   delete iter->second;
   sync_items_.erase(iter);
   if (item_type != sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP)
-    model_->item_list()->DeleteItem(item_id);
+    model_->DeleteItem(item_id);
 }
 
 std::string AppListSyncableService::SyncItem::ToString() const {
@@ -653,6 +663,8 @@ std::string AppListSyncableService::SyncItem::ToString() const {
   } else {
     res += " { " + item_name + " }";
     res += " [" + item_ordinal.ToDebugString() + "]";
+    if (!parent_id.empty())
+      res += " <" + parent_id.substr(0, 8) + ">";
   }
   return res;
 }

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -88,7 +88,7 @@ class AppListSyncableService : public syncer::SyncableService,
       const syncer::SyncChangeList& change_list) OVERRIDE;
 
  private:
-  class ItemListObserver;
+  class ModelObserver;
   typedef std::map<std::string, SyncItem*> SyncItemMap;
 
   // content::NotificationObserver
@@ -102,10 +102,10 @@ class AppListSyncableService : public syncer::SyncableService,
   // Returns true if sync has restarted, otherwise runs |flare_|.
   bool SyncStarted();
 
-  // If |app_item| matches an existing sync item, updates the sync item and
-  // returns it. Otherwise adds |app_item| to |sync_items_| and returns the new
-  // item. If |app_item| is invalid returns NULL.
-  SyncItem* AddOrUpdateSyncItem(AppListItem* app_item);
+  // If |app_item| matches an existing sync item, returns it. Otherwise adds
+  // |app_item| to |sync_items_| and returns the new item. If |app_item| is
+  // invalid returns NULL.
+  SyncItem* FindOrAddSyncItem(AppListItem* app_item);
 
   // Creates a sync item for |app_item| and sends an ADD SyncChange event.
   SyncItem* CreateSyncItemFromAppItem(AppListItem* app_item);
@@ -162,7 +162,7 @@ class AppListSyncableService : public syncer::SyncableService,
   extensions::ExtensionSystem* extension_system_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<AppListModel> model_;
-  scoped_ptr<ItemListObserver> item_list_observer_;
+  scoped_ptr<ModelObserver> model_observer_;
   scoped_ptr<ExtensionAppModelBuilder> apps_builder_;
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
   scoped_ptr<syncer::SyncErrorFactory> sync_error_handler_;

chrome/browser/ui/app_list/extension_app_model_builder.cc

@@ -59,6 +59,7 @@ ExtensionAppModelBuilder::~ExtensionAppModelBuilder() {
 
 void ExtensionAppModelBuilder::InitializeWithService(
     app_list::AppListSyncableService* service) {
+  DCHECK(!service_ && !profile_);
   model_ = service->model();
   model_->item_list()->AddObserver(this);
   service_ = service;
@@ -69,6 +70,7 @@ void ExtensionAppModelBuilder::InitializeWithService(
 void ExtensionAppModelBuilder::InitializeWithProfile(
     Profile* profile,
     app_list::AppListModel* model) {
+  DCHECK(!service_ && !profile_);
   model_ = model;
   model_->item_list()->AddObserver(this);
   profile_ = profile;
@@ -105,7 +107,7 @@ void ExtensionAppModelBuilder::OnDownloadProgress(
 
 void ExtensionAppModelBuilder::OnInstallFailure(
     const std::string& extension_id) {
-  model_->item_list()->DeleteItem(extension_id);
+  model_->DeleteItem(extension_id);
 }
 
 void ExtensionAppModelBuilder::OnExtensionLoaded(const Extension* extension) {
@@ -142,7 +144,7 @@ void ExtensionAppModelBuilder::OnExtensionUninstalled(
     service_->RemoveItem(extension->id());
     return;
   }
-  model_->item_list()->DeleteItem(extension->id());
+  model_->DeleteItem(extension->id());
 }
 
 void ExtensionAppModelBuilder::OnAppsReordered() {
@@ -191,12 +193,7 @@ void ExtensionAppModelBuilder::AddApps(
 }
 
 void ExtensionAppModelBuilder::BuildModel() {
-  // Delete any extension apps.
-  model_->item_list()->DeleteItemsByType(ExtensionAppItem::kItemType);
-
-  if (tracker_)
-    tracker_->RemoveObserver(this);
-
+  DCHECK(!tracker_);
   tracker_ = controller_->GetInstallTrackerFor(profile_);
 
   PopulateApps();
@@ -225,7 +222,7 @@ void ExtensionAppModelBuilder::InsertApp(ExtensionAppItem* app) {
     service_->AddItem(app);
     return;
   }
-  model_->item_list()->AddItem(app);
+  model_->AddItem(app);
 }
 
 void ExtensionAppModelBuilder::SetHighlightedApp(

chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc

@@ -122,19 +122,30 @@ class ExtensionAppModelBuilderTest : public ExtensionServiceTestBase {
     const extensions::ExtensionSet* extensions = service_->extensions();
     ASSERT_EQ(static_cast<size_t>(4),  extensions->size());
 
+    CreateBuilder();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    ResetBuilder();
+  }
+
+ protected:
+  // Creates a new builder, destroying any existing one.
+  void CreateBuilder() {
+    ResetBuilder();  // Destroy any existing builder in the correct order.
+
     model_.reset(new app_list::AppListModel);
     controller_.reset(new TestAppListControllerDelegate);
     builder_.reset(new ExtensionAppModelBuilder(controller_.get()));
     builder_->InitializeWithProfile(profile_.get(), model_.get());
   }
 
-  virtual void TearDown() OVERRIDE {
+  void ResetBuilder() {
     builder_.reset();
     controller_.reset();
     model_.reset();
   }
 
- protected:
   scoped_ptr<app_list::AppListModel> model_;
   scoped_ptr<TestAppListControllerDelegate> controller_;
   scoped_ptr<ExtensionAppModelBuilder> builder_;
@@ -281,7 +292,7 @@ TEST_F(ExtensionAppModelBuilderTest, InvalidOrdinal) {
   extensions::AppSorting* sorting = service_->extension_prefs()->app_sorting();
   sorting->ClearOrdinals(kPackagedApp1Id);
 
-  // Creates an corrupted ordinal case.
+  // Creates a corrupted ordinal case.
   extensions::ExtensionScopedPrefs* scoped_prefs = service_->extension_prefs();
   scoped_prefs->UpdateExtensionPref(
       kHostedAppId,
@@ -289,8 +300,7 @@ TEST_F(ExtensionAppModelBuilderTest, InvalidOrdinal) {
       base::Value::CreateStringValue("a corrupted ordinal"));
 
   // This should not assert or crash.
-  ExtensionAppModelBuilder builder1(controller_.get());
-  builder1.InitializeWithProfile(profile_.get(), model_.get());
+  CreateBuilder();
 }
 
 TEST_F(ExtensionAppModelBuilderTest, OrdinalConfilicts) {
@@ -309,8 +319,7 @@ TEST_F(ExtensionAppModelBuilderTest, OrdinalConfilicts) {
   sorting->SetAppLaunchOrdinal(kPackagedApp2Id, conflict_ordinal);
 
   // This should not assert or crash.
-  ExtensionAppModelBuilder builder1(controller_.get());
-  builder1.InitializeWithProfile(profile_.get(), model_.get());
+  CreateBuilder();
 
   // By default, conflicted items are sorted by their app ids (= order added).
   EXPECT_EQ(std::string("Hosted App,Packaged App 1,Packaged App 2"),

chrome/browser/ui/app_list/fast_show_pickler.cc

@@ -208,12 +208,12 @@ scoped_ptr<Pickle> FastShowPickler::PickleAppListModelForFastShow(
 }
 
 void FastShowPickler::CopyOver(AppListModel* src, AppListModel* dest) {
-  dest->item_list()->DeleteItemsByType(NULL /* all items */);
+  DCHECK_EQ(0u, dest->item_list()->item_count());
   for (size_t i = 0; i < src->item_list()->item_count(); i++) {
     AppListItem* src_item = src->item_list()->item_at(i);
     AppListItem* dest_item = new AppListItem(src_item->id());
     CopyOverItem(src_item, dest_item);
-    dest->item_list()->AddItem(dest_item);
+    dest->AddItem(dest_item);
   }
 }
 
@@ -234,7 +234,7 @@ FastShowPickler::UnpickleAppListModelForFastShow(Pickle* pickle) {
     scoped_ptr<AppListItem> item(UnpickleAppListItem(&it).Pass());
     if (!item)
       return scoped_ptr<AppListModel>();
-    model->item_list()->AddItem(item.release());
+    model->AddItem(item.release());
   }
 
   return model.Pass();

chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc

@@ -78,7 +78,7 @@ TEST_F(AppListModelPicklerUnitTest, OneItem) {
   AppListModel model;
   AppListItem* app1 = new AppListItem("abc");
   app1->SetTitleAndFullName("ht", "hello, there");
-  model.item_list()->AddItem(app1);
+  model.AddItem(app1);
 
   DoConsistencyChecks(&model);
 }
@@ -87,11 +87,11 @@ TEST_F(AppListModelPicklerUnitTest, TwoItems) {
   AppListModel model;
   AppListItem* app1 = new AppListItem("abc");
   app1->SetTitleAndFullName("ht", "hello, there");
-  model.item_list()->AddItem(app1);
+  model.AddItem(app1);
 
   AppListItem* app2 = new AppListItem("abc2");
   app2->SetTitleAndFullName("ht2", "hello, there 2");
-  model.item_list()->AddItem(app2);
+  model.AddItem(app2);
 
   DoConsistencyChecks(&model);
 }
@@ -101,11 +101,11 @@ TEST_F(AppListModelPicklerUnitTest, Images) {
   AppListItem* app1 = new AppListItem("abc");
   app1->SetTitleAndFullName("ht", "hello, there");
   app1->SetIcon(MakeImage(), true);
-  model.item_list()->AddItem(app1);
+  model.AddItem(app1);
 
   AppListItem* app2 = new AppListItem("abc2");
   app2->SetTitleAndFullName("ht2", "hello, there 2");
-  model.item_list()->AddItem(app2);
+  model.AddItem(app2);
 
   DoConsistencyChecks(&model);
 }
@@ -115,7 +115,7 @@ TEST_F(AppListModelPicklerUnitTest, EmptyImage) {
   AppListItem* app1 = new AppListItem("abc");
   app1->SetTitleAndFullName("ht", "hello, there");
   app1->SetIcon(gfx::ImageSkia(), true);
-  model.item_list()->AddItem(app1);
+  model.AddItem(app1);
 
   DoConsistencyChecks(&model);
 }

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;
 }

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;

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_);
 }
 

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

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) {

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

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 {
 

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

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

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_;

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() {}
 };

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(

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

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];

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) {

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.
+  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.

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