Cleanup AppListSyncableService

This is just some code cleanup that I did while adding support for
folder sync. There are no major functional changes, but it does fix some
minor bugs exposed while testing AppList sync.

The cleanup includes:
* Rename GetAppType() -> GetItemType() which is more descriptive
* Introduce AppListSyncableService::ItemListObserver so that any changes
  to AppListModel.item_list_ are reflected in sync items
* Eliminates SyncItem update calls from ExtensionAppModelBuilder
* Breaks up AppListSyncableService::AddItem into smaller pieces

BUG=335761

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

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

16 files changed, 254 insertions, 140 deletions

diff --git a/chrome/browser/ui/app_list/app_list_syncable_service.cc b/chrome/browser/ui/app_list/app_list_syncable_service.cc
index 0d0baee..68c6522 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.cc
@@ -30,6 +30,11 @@ namespace app_list {
 
 namespace {
 
+bool SyncAppListEnabled() {
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableSyncAppList);
+}
+
 void UpdateSyncItemFromSync(const sync_pb::AppListSpecifics& specifics,
                             AppListSyncableService::SyncItem* item) {
   DCHECK_EQ(item->item_id, specifics.item_id());
@@ -85,19 +90,25 @@ bool AppIsDefault(ExtensionService* service, const std::string& id) {
   return service && service->extension_prefs()->WasInstalledByDefault(id);
 }
 
-bool AppIsPlatformApp(ExtensionService* service, const std::string& id) {
-  if (!service)
-    return false;
-  const extensions::Extension* app = service->GetInstalledExtension(id);
-  DVLOG_IF(1, !app) << "No App for ID: " << id;
-  return app ? app->is_platform_app() : false;
-}
-
 void UninstallExtension(ExtensionService* service, const std::string& id) {
   if (service && service->GetInstalledExtension(id))
     service->UninstallExtension(id, false, NULL);
 }
 
+bool GetAppListItemType(AppListItem* item,
+                        sync_pb::AppListSpecifics::AppListItemType* type) {
+  const char* item_type = item->GetItemType();
+  if (item_type == ExtensionAppItem::kItemType) {
+    *type = sync_pb::AppListSpecifics::TYPE_APP;
+  } else if (item_type == AppListFolderItem::kItemType) {
+    *type = sync_pb::AppListSpecifics::TYPE_FOLDER;
+  } else {
+    LOG(ERROR) << "Unrecognized model type: " << item_type;
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 // AppListSyncableService::SyncItem
@@ -112,6 +123,40 @@ AppListSyncableService::SyncItem::SyncItem(
 AppListSyncableService::SyncItem::~SyncItem() {
 }
 
+// AppListSyncableService::ItemListObserver
+
+class AppListSyncableService::ItemListObserver
+    : public AppListItemListObserver {
+ public:
+  explicit ItemListObserver(AppListSyncableService* owner) : owner_(owner) {
+    owner_->model()->item_list()->AddObserver(this);
+  }
+
+  virtual ~ItemListObserver() {
+    owner_->model()->item_list()->RemoveObserver(this);
+  }
+
+ private:
+  // AppListItemListObserver
+  virtual void OnListItemAdded(size_t index, AppListItem* item) OVERRIDE {
+    owner_->AddOrUpdateFromSyncItem(item);
+  }
+
+  virtual void OnListItemRemoved(size_t index, AppListItem* item) OVERRIDE {
+    owner_->RemoveSyncItem(item->id());
+  }
+
+  virtual void OnListItemMoved(size_t from_index,
+                               size_t to_index,
+                               AppListItem* item) OVERRIDE {
+    owner_->UpdateSyncItem(item);
+  }
+
+  AppListSyncableService* owner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ItemListObserver);
+};
+
 // AppListSyncableService
 
 AppListSyncableService::AppListSyncableService(
@@ -125,6 +170,9 @@ AppListSyncableService::AppListSyncableService(
     return;
   }
 
+  if (SyncAppListEnabled())
+    item_list_observer_.reset(new ItemListObserver(this));
+
   if (extension_system->extension_service()->is_ready()) {
     BuildModel();
     return;
@@ -136,6 +184,9 @@ AppListSyncableService::AppListSyncableService(
 }
 
 AppListSyncableService::~AppListSyncableService() {
+  // Remove observers.
+  item_list_observer_.reset();
+
   STLDeleteContainerPairSecondPointers(sync_items_.begin(), sync_items_.end());
 }
 
@@ -152,9 +203,7 @@ void AppListSyncableService::BuildModel() {
   apps_builder_.reset(new ExtensionAppModelBuilder(controller));
   DCHECK(profile_);
   // TODO(stevenjb): Correctly handle OTR profiles for Guest mode.
-  if (!profile_->IsOffTheRecord() &&
-      CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableSyncAppList)) {
+  if (!profile_->IsOffTheRecord() && SyncAppListEnabled()) {
     DVLOG(1) << this << ": AppListSyncableService: InitializeWithService.";
     SyncStarted();
     apps_builder_->InitializeWithService(this);
@@ -182,71 +231,109 @@ AppListSyncableService::GetSyncItem(const std::string& id) const {
   return NULL;
 }
 
-void AppListSyncableService::AddItem(AppListItem* item) {
-  const std::string& item_id = item->id();
+void AppListSyncableService::AddItem(AppListItem* app_item) {
+  SyncItem* sync_item = AddOrUpdateSyncItem(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);
+}
+
+AppListSyncableService::SyncItem* AppListSyncableService::AddOrUpdateSyncItem(
+    AppListItem* app_item) {
+  const std::string& item_id = app_item->id();
   if (item_id.empty()) {
     LOG(ERROR) << "AppListItem item with empty ID";
-    return;
-  }
-  sync_pb::AppListSpecifics::AppListItemType type;
-  const char* item_type = item->GetAppType();
-  if (item_type == ExtensionAppItem::kAppType) {
-    type = sync_pb::AppListSpecifics_AppListItemType_TYPE_APP;
-  } else if (item_type == AppListFolderItem::kAppType) {
-    type = sync_pb::AppListSpecifics_AppListItemType_TYPE_FOLDER;
-  } else {
-    LOG(ERROR) << "Unrecognized model type: " << item_type;
-    return;
+    return NULL;
   }
   SyncItem* sync_item = FindSyncItem(item_id);
   if (sync_item) {
     // If there is an existing, non-REMOVE_DEFAULT entry, update it.
     if (sync_item->item_type !=
         sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
-      DCHECK_EQ(sync_item->item_type, type);
       DVLOG(2) << this << ": AddItem already exists: " << sync_item->ToString();
-      UpdateItem(item);
-      return;
-    }
-    // If there is an existing REMOVE_DEFAULT_APP entry, and the app is
-    // installed as a Default app, uninstall the app instead of adding it.
-    if (type == sync_pb::AppListSpecifics_AppListItemType_TYPE_APP &&
-        AppIsDefault(extension_system_->extension_service(), item_id)) {
-      DVLOG(1) << this << ": AddItem: Uninstall: " << sync_item->ToString();
-      UninstallExtension(extension_system_->extension_service(), item_id);
-      return;
-    }
-    // Otherwise, we are adding the app as a non-default app (i.e. an app that
-    // was installed by Default and removed is getting installed explicitly by
-    // the user), so delete the REMOVE_DEFAULT_APP.
-    if (SyncStarted()) {
-      DVLOG(2) << this << " -> SYNC DELETE: " << sync_item->ToString();
-      SyncChange sync_change(FROM_HERE, SyncChange::ACTION_DELETE,
-                             GetSyncDataFromSyncItem(sync_item));
-      sync_processor_->ProcessSyncChanges(
-          FROM_HERE, syncer::SyncChangeList(1, sync_change));
+      UpdateSyncItem(app_item);
+      return sync_item;
     }
-    delete sync_item;
-    sync_items_.erase(item_id);
-    // Fall through. The REMOVE_DEFAULT_APP entry has been deleted, now an App
-    // entry can be added as usual.
+
+    if (RemoveDefaultApp(app_item, sync_item))
+      return NULL;
+
+    // Fall through. The REMOVE_DEFAULT_APP entry has been deleted, now a new
+    // App entry can be added.
   }
 
-  sync_item = CreateSyncItem(item_id, type);
-  UpdateSyncItemFromAppItem(item, sync_item);
-  model_->item_list()->AddItem(item);
-  DVLOG(1) << this << ": AddItem: " << sync_item->ToString() << " Default: "
-           << AppIsDefault(extension_system_->extension_service(), item->id());
+  return CreateSyncItemFromAppItem(app_item);
+}
+
+AppListSyncableService::SyncItem*
+AppListSyncableService::CreateSyncItemFromAppItem(AppListItem* app_item) {
+  sync_pb::AppListSpecifics::AppListItemType type;
+  if (!GetAppListItemType(app_item, &type))
+    return NULL;
+  SyncItem* sync_item = CreateSyncItem(app_item->id(), type);
+  UpdateSyncItemFromAppItem(app_item, sync_item);
   SendSyncChange(sync_item, SyncChange::ACTION_ADD);
+  return sync_item;
+}
+
+void AppListSyncableService::AddOrUpdateFromSyncItem(AppListItem* app_item) {
+  SyncItem* sync_item = FindSyncItem(app_item->id());
+  if (sync_item) {
+    UpdateAppItemFromSyncItem(sync_item, app_item);
+    return;
+  }
+  CreateSyncItemFromAppItem(app_item);
+}
+
+bool AppListSyncableService::RemoveDefaultApp(AppListItem* item,
+                                              SyncItem* sync_item) {
+  CHECK_EQ(sync_item->item_type,
+           sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP);
+
+  // If there is an existing REMOVE_DEFAULT_APP entry, and the app is
+  // installed as a Default app, uninstall the app instead of adding it.
+  if (sync_item->item_type == sync_pb::AppListSpecifics::TYPE_APP &&
+      AppIsDefault(extension_system_->extension_service(), item->id())) {
+    DVLOG(1) << this << ": HandleDefaultApp: Uninstall: "
+             << sync_item->ToString();
+    UninstallExtension(extension_system_->extension_service(), item->id());
+    return true;
+  }
+
+  // Otherwise, we are adding the app as a non-default app (i.e. an app that
+  // was installed by Default and removed is getting installed explicitly by
+  // the user), so delete the REMOVE_DEFAULT_APP.
+  DeleteSyncItem(sync_item);
+  return false;
+}
+
+void AppListSyncableService::DeleteSyncItem(SyncItem* sync_item) {
+  if (SyncStarted()) {
+    DVLOG(2) << this << " -> SYNC DELETE: " << sync_item->ToString();
+    SyncChange sync_change(FROM_HERE, SyncChange::ACTION_DELETE,
+                           GetSyncDataFromSyncItem(sync_item));
+    sync_processor_->ProcessSyncChanges(
+        FROM_HERE, syncer::SyncChangeList(1, sync_change));
+  }
+  std::string item_id = sync_item->item_id;
+  delete sync_item;
+  sync_items_.erase(item_id);
 }
 
-void AppListSyncableService::UpdateItem(AppListItem* item) {
-  SyncItem* sync_item = FindSyncItem(item->id());
+void AppListSyncableService::UpdateSyncItem(AppListItem* app_item) {
+  SyncItem* sync_item = FindSyncItem(app_item->id());
   if (!sync_item) {
-    LOG(ERROR) << "UpdateItem: no sync item: " << item->id();
+    LOG(ERROR) << "UpdateItem: no sync item: " << app_item->id();
     return;
   }
-  bool changed = UpdateSyncItemFromAppItem(item, sync_item);
+  bool changed = UpdateSyncItemFromAppItem(app_item, sync_item);
   if (!changed) {
     DVLOG(2) << this << " - Update: SYNC NO CHANGE: " << sync_item->ToString();
     return;
@@ -255,25 +342,29 @@ void AppListSyncableService::UpdateItem(AppListItem* item) {
 }
 
 void AppListSyncableService::RemoveItem(const std::string& id) {
-  DVLOG(2) << this << ": RemoveItem: " << id.substr(0, 8);
+  RemoveSyncItem(id);
+  model_->item_list()->DeleteItem(id);
+}
+
+void AppListSyncableService::RemoveSyncItem(const std::string& id) {
+  DVLOG(2) << this << ": RemoveSyncItem: " << id.substr(0, 8);
   SyncItemMap::iterator iter = sync_items_.find(id);
   if (iter == sync_items_.end()) {
-    DVLOG(2) << this << " : No Sync Item.";
+    DVLOG(2) << this << " : RemoveSyncItem: No Item.";
     return;
   }
-  // Always delete the item from the model.
-  model_->item_list()->DeleteItem(id);
 
   // Check for existing RemoveDefault sync item.
   SyncItem* sync_item = iter->second;
-  if (sync_item->item_type ==
-      sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
+  sync_pb::AppListSpecifics::AppListItemType type = sync_item->item_type;
+  if (type == sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
     // RemoveDefault item exists, just return.
     DVLOG(2) << this << " : RemoveDefault Item exists.";
     return;
   }
 
-  if (AppIsDefault(extension_system_->extension_service(), id)) {
+  if (type == sync_pb::AppListSpecifics::TYPE_APP &&
+      AppIsDefault(extension_system_->extension_service(), id)) {
     // This is a Default app; update the entry to a REMOVE_DEFAULT entry. This
     // will overwrite any existing entry for the item.
     DVLOG(2) << this << " -> SYNC UPDATE: REMOVE_DEFAULT: "
@@ -283,17 +374,7 @@ void AppListSyncableService::RemoveItem(const std::string& id) {
     return;
   }
 
-  // Existing entry is a normal entry, send a Delete sync change and remove
-  // the entry.
-  if (SyncStarted()) {
-    DVLOG(2) << this << " -> SYNC DELETE: " << sync_item->ToString();
-    SyncChange sync_change(FROM_HERE, SyncChange::ACTION_DELETE,
-                           GetSyncDataFromSyncItem(sync_item));
-    sync_processor_->ProcessSyncChanges(
-        FROM_HERE, syncer::SyncChangeList(1, sync_change));
-  }
-  delete sync_item;
-  sync_items_.erase(iter);
+  DeleteSyncItem(sync_item);
 }
 
 // AppListSyncableService syncer::SyncableService
@@ -331,7 +412,7 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
              << data.GetSpecifics().app_list().item_id()
              << " Type: " << data.GetSpecifics().app_list().item_type();
     DCHECK_EQ(syncer::APP_LIST, data.GetDataType());
-    if (ProcessSyncItem(data.GetSpecifics().app_list()))
+    if (ProcessSyncItemSpecifics(data.GetSpecifics().app_list()))
       ++new_items;
     else
       ++updated_items;
@@ -397,9 +478,9 @@ syncer::SyncError AppListSyncableService::ProcessSyncChanges(
              << " (" << change.change_type() << ")";
     if (change.change_type() == SyncChange::ACTION_ADD ||
         change.change_type() == SyncChange::ACTION_UPDATE) {
-      ProcessSyncItem(change.sync_data().GetSpecifics().app_list());
+      ProcessSyncItemSpecifics(change.sync_data().GetSpecifics().app_list());
     } else if (change.change_type() == SyncChange::ACTION_DELETE) {
-      DeleteSyncItem(change.sync_data().GetSpecifics().app_list());
+      DeleteSyncItemSpecifics(change.sync_data().GetSpecifics().app_list());
     } else {
       LOG(ERROR) << "Invalid sync change";
     }
@@ -409,7 +490,7 @@ syncer::SyncError AppListSyncableService::ProcessSyncChanges(
 
 // AppListSyncableService private
 
-bool AppListSyncableService::ProcessSyncItem(
+bool AppListSyncableService::ProcessSyncItemSpecifics(
     const sync_pb::AppListSpecifics& specifics) {
   const std::string& item_id = specifics.item_id();
   if (item_id.empty()) {
@@ -449,20 +530,12 @@ bool AppListSyncableService::ProcessSyncItem(
 }
 
 void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
+  DVLOG(2) << "ProcessNewSyncItem: " << sync_item->ToString();
   switch (sync_item->item_type) {
     case sync_pb::AppListSpecifics::TYPE_APP: {
-      std::string extension_id = sync_item->item_id;
-      bool is_platform_app =
-          AppIsPlatformApp(extension_system_->extension_service(),
-                           extension_id);
-      ExtensionAppItem* app_item = new ExtensionAppItem(
-          profile_,
-          sync_item,
-          extension_id,
-          sync_item->item_name,
-          gfx::ImageSkia(),
-          is_platform_app);
-      model_->item_list()->AddItem(app_item);
+      // New apps are added through ExtensionAppModelBuilder.
+      // TODO(stevenjb): Determine how to handle app items in sync that
+      // are not installed (e.g. default / OEM apps).
       return;
     }
     case sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP: {
@@ -486,12 +559,24 @@ void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
 }
 
 void AppListSyncableService::ProcessExistingSyncItem(SyncItem* sync_item) {
-  if (sync_item->item_type !=
+  if (sync_item->item_type ==
       sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
-    AppListItem* item = model_->item_list()->FindItem(sync_item->item_id);
-    if (item && !item->position().Equals(sync_item->item_ordinal))
-      model_->item_list()->SetItemPosition(item, sync_item->item_ordinal);
+    return;
+  }
+  DVLOG(2) << "ProcessExistingSyncItem: " << sync_item->ToString();
+  AppListItem* app_item = model_->item_list()->FindItem(sync_item->item_id);
+  if (!app_item) {
+    LOG(ERROR) << "Item not found in model: " << sync_item->ToString();
+    return;
   }
+  UpdateAppItemFromSyncItem(sync_item, app_item);
+}
+
+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);
 }
 
 bool AppListSyncableService::SyncStarted() {
@@ -541,14 +626,14 @@ AppListSyncableService::CreateSyncItem(
   return sync_item;
 }
 
-void AppListSyncableService::DeleteSyncItem(
+void AppListSyncableService::DeleteSyncItemSpecifics(
     const sync_pb::AppListSpecifics& specifics) {
   const std::string& item_id = specifics.item_id();
   if (item_id.empty()) {
     LOG(ERROR) << "Delete AppList item with empty ID";
     return;
   }
-  DVLOG(2) << this << ": DeleteSyncItem: " << item_id.substr(0, 8);
+  DVLOG(2) << this << ": DeleteSyncItemSpecifics: " << item_id.substr(0, 8);
   SyncItemMap::iterator iter = sync_items_.find(item_id);
   if (iter == sync_items_.end())
     return;
diff --git a/chrome/browser/ui/app_list/app_list_syncable_service.h b/chrome/browser/ui/app_list/app_list_syncable_service.h
index 9d44b59..1de32a9 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.h
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.h
@@ -32,8 +32,8 @@ class AppListSpecifics;
 
 namespace app_list {
 
-class AppListModel;
 class AppListItem;
+class AppListModel;
 
 // Keyed Service that owns, stores, and syncs an AppListModel for a profile.
 class AppListSyncableService : public syncer::SyncableService,
@@ -60,13 +60,10 @@ class AppListSyncableService : public syncer::SyncableService,
 
   virtual ~AppListSyncableService();
 
-  // Adds |item| to |sync_items_| and |model_|. Does nothing if a sync item
-  // already exists.
+  // Adds |item| to |sync_items_| and |model_|. If a sync item already exists,
+  // updates the existing sync item instead.
   void AddItem(AppListItem* item);
 
-  // Updates existing entry in |sync_items_| from |item|.
-  void UpdateItem(AppListItem* item);
-
   // Removes sync item matching |id|.
   void RemoveItem(const std::string& id);
 
@@ -91,6 +88,7 @@ class AppListSyncableService : public syncer::SyncableService,
       const syncer::SyncChangeList& change_list) OVERRIDE;
 
  private:
+  class ItemListObserver;
   typedef std::map<std::string, SyncItem*> SyncItemMap;
 
   // content::NotificationObserver
@@ -104,17 +102,47 @@ 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);
+
+  // Creates a sync item for |app_item| and sends an ADD SyncChange event.
+  SyncItem* CreateSyncItemFromAppItem(AppListItem* app_item);
+
+  // If a sync item for |app_item| already exists, update |app_item| from the
+  // sync item, otherwise create a new sync item from |app_item|.
+  void AddOrUpdateFromSyncItem(AppListItem* app_item);
+
+  // Either uninstalling a default app or remove the REMOVE_DEFAULT sync item.
+  // Returns true if the app is removed. Otherwise deletes the existing sync
+  // item and returns false.
+  bool RemoveDefaultApp(AppListItem* item, SyncItem* sync_item);
+
+  // Deletes a sync item from |sync_items_| and sends a DELETE action.
+  void DeleteSyncItem(SyncItem* sync_item);
+
+  // Updates existing entry in |sync_items_| from |app_item|.
+  void UpdateSyncItem(AppListItem* app_item);
+
+  // Removes sync item matching |id|.
+  void RemoveSyncItem(const std::string& id);
+
   // Creates or updates a SyncItem from |specifics|. Returns true if a new item
   // was created.
-  bool ProcessSyncItem(const sync_pb::AppListSpecifics& specifics);
+  bool ProcessSyncItemSpecifics(const sync_pb::AppListSpecifics& specifics);
 
-  // Handles a newly created sync item (e.g. creates a new Appitem and adds it
+  // Handles a newly created sync item (e.g. creates a new AppItem and adds it
   // to the model or uninstalls a deleted default item.
   void ProcessNewSyncItem(SyncItem* sync_item);
 
-  // Handles updating an existing sync item (e.g. updates item positions).
+  // Handles an existing sync item.
   void ProcessExistingSyncItem(SyncItem* sync_item);
 
+  // Updates |app_item| from |sync_item| (e.g. updates item positions).
+  void UpdateAppItemFromSyncItem(const SyncItem* sync_item,
+                                 AppListItem* app_item);
+
   // Sends ADD or CHANGED for sync item.
   void SendSyncChange(SyncItem* sync_item,
                       syncer::SyncChange::SyncChangeType sync_change_type);
@@ -128,12 +156,13 @@ class AppListSyncableService : public syncer::SyncableService,
       sync_pb::AppListSpecifics::AppListItemType item_type);
 
   // Deletes a SyncItem matching |specifics|.
-  void DeleteSyncItem(const sync_pb::AppListSpecifics& specifics);
+  void DeleteSyncItemSpecifics(const sync_pb::AppListSpecifics& specifics);
 
   Profile* profile_;
   extensions::ExtensionSystem* extension_system_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<AppListModel> model_;
+  scoped_ptr<ItemListObserver> item_list_observer_;
   scoped_ptr<ExtensionAppModelBuilder> apps_builder_;
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
   scoped_ptr<syncer::SyncErrorFactory> sync_error_handler_;
diff --git a/chrome/browser/ui/app_list/extension_app_item.cc b/chrome/browser/ui/app_list/extension_app_item.cc
index e0bff9d..7715988 100644
--- a/chrome/browser/ui/app_list/extension_app_item.cc
+++ b/chrome/browser/ui/app_list/extension_app_item.cc
@@ -90,6 +90,8 @@ ExtensionAppItem::ExtensionAppItem(
   if (sync_item && sync_item->item_ordinal.IsValid()) {
     // An existing synced position exists, use that.
     set_position(sync_item->item_ordinal);
+    if (title().empty())
+      SetTitleAndFullName(sync_item->item_name, sync_item->item_name);
     return;
   }
   GetAppSorting(profile_)->EnsureValidOrdinals(extension_id_,
@@ -275,10 +277,10 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() {
 }
 
 // static
-const char ExtensionAppItem::kAppType[] = "ExtensionAppItem";
+const char ExtensionAppItem::kItemType[] = "ExtensionAppItem";
 
-const char* ExtensionAppItem::GetAppType() const {
-  return ExtensionAppItem::kAppType;
+const char* ExtensionAppItem::GetItemType() const {
+  return ExtensionAppItem::kItemType;
 }
 
 void ExtensionAppItem::ExecuteLaunchCommand(int event_flags) {
diff --git a/chrome/browser/ui/app_list/extension_app_item.h b/chrome/browser/ui/app_list/extension_app_item.h
index 8e54f0a..e142315 100644
--- a/chrome/browser/ui/app_list/extension_app_item.h
+++ b/chrome/browser/ui/app_list/extension_app_item.h
@@ -57,7 +57,7 @@ class ExtensionAppItem : public app_list::AppListItem,
   const std::string& extension_id() const { return extension_id_; }
   const std::string& extension_name() const { return extension_name_; }
 
-  static const char kAppType[];
+  static const char kItemType[];
 
  private:
   // Gets extension associated with this model. Returns NULL if extension
@@ -89,7 +89,7 @@ class ExtensionAppItem : public app_list::AppListItem,
   // Overridden from AppListItem:
   virtual void Activate(int event_flags) OVERRIDE;
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
-  virtual const char* GetAppType() const OVERRIDE;
+  virtual const char* GetItemType() const OVERRIDE;
 
   // Overridden from app_list::AppContextMenuDelegate:
   virtual void ExecuteLaunchCommand(int event_flags) OVERRIDE;
diff --git a/chrome/browser/ui/app_list/extension_app_model_builder.cc b/chrome/browser/ui/app_list/extension_app_model_builder.cc
index 818c03c..2db886e 100644
--- a/chrome/browser/ui/app_list/extension_app_model_builder.cc
+++ b/chrome/browser/ui/app_list/extension_app_model_builder.cc
@@ -190,7 +190,7 @@ void ExtensionAppModelBuilder::AddApps(
 
 void ExtensionAppModelBuilder::BuildModel() {
   // Delete any extension apps.
-  model_->item_list()->DeleteItemsByType(ExtensionAppItem::kAppType);
+  model_->item_list()->DeleteItemsByType(ExtensionAppItem::kItemType);
 
   if (tracker_)
     tracker_->RemoveObserver(this);
@@ -245,9 +245,9 @@ ExtensionAppItem* ExtensionAppModelBuilder::GetExtensionAppItem(
   app_list::AppListItem* item =
       model_->item_list()->FindItem(extension_id);
   LOG_IF(ERROR, item &&
-         item->GetAppType() != ExtensionAppItem::kAppType)
+         item->GetItemType() != ExtensionAppItem::kItemType)
       << "App Item matching id: " << extension_id
-      << " has incorrect type: '" << item->GetAppType() << "'";
+      << " has incorrect type: '" << item->GetItemType() << "'";
   return static_cast<ExtensionAppItem*>(item);
 }
 
@@ -268,18 +268,16 @@ void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index,
   // This will get called from AppListItemList::ListItemMoved after
   // set_position is called for the item.
   app_list::AppListItemList* item_list = model_->item_list();
-  if (item->GetAppType() != ExtensionAppItem::kAppType)
+  if (item->GetItemType() != ExtensionAppItem::kItemType)
     return;
 
-  if (service_) {
-    service_->UpdateItem(item);
+  if (service_)
     return;
-  }
 
   ExtensionAppItem* prev = NULL;
   for (size_t idx = to_index; idx > 0; --idx) {
     app_list::AppListItem* item = item_list->item_at(idx - 1);
-    if (item->GetAppType() == ExtensionAppItem::kAppType) {
+    if (item->GetItemType() == ExtensionAppItem::kItemType) {
       prev = static_cast<ExtensionAppItem*>(item);
       break;
     }
@@ -287,7 +285,7 @@ void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index,
   ExtensionAppItem* next = NULL;
   for (size_t idx = to_index; idx < item_list->item_count() - 1; ++idx) {
     app_list::AppListItem* item = item_list->item_at(idx + 1);
-    if (item->GetAppType() == ExtensionAppItem::kAppType) {
+    if (item->GetItemType() == ExtensionAppItem::kItemType) {
       next = static_cast<ExtensionAppItem*>(item);
       break;
     }
diff --git a/ui/app_list/app_list_folder_item.cc b/ui/app_list/app_list_folder_item.cc
index 55ac8d5..1aa14af 100644
--- a/ui/app_list/app_list_folder_item.cc
+++ b/ui/app_list/app_list_folder_item.cc
@@ -110,7 +110,7 @@ void AppListFolderItem::Activate(int event_flags) {
 }
 
 // static
-const char AppListFolderItem::kAppType[] = "FolderItem";
+const char AppListFolderItem::kItemType[] = "FolderItem";
 
 // static
 Rects AppListFolderItem::GetTopIconsBounds(
@@ -144,8 +144,8 @@ Rects AppListFolderItem::GetTopIconsBounds(
   return top_icon_bounds;
 }
 
-const char* AppListFolderItem::GetAppType() const {
-  return AppListFolderItem::kAppType;
+const char* AppListFolderItem::GetItemType() const {
+  return AppListFolderItem::kItemType;
 }
 
 ui::MenuModel* AppListFolderItem::GetContextMenuModel() {
diff --git a/ui/app_list/app_list_folder_item.h b/ui/app_list/app_list_folder_item.h
index debcc04..18c890b 100644
--- a/ui/app_list/app_list_folder_item.h
+++ b/ui/app_list/app_list_folder_item.h
@@ -36,7 +36,7 @@ class APP_LIST_EXPORT AppListFolderItem : public AppListItem,
 
   AppListItemList* item_list() { return item_list_.get(); }
 
-  static const char kAppType[];
+  static const char kItemType[];
 
   // Calculates the top item icons' bounds inside |folder_icon_bounds|.
   // Returns the bounds of top item icons in sequence of top left, top right,
@@ -46,7 +46,7 @@ class APP_LIST_EXPORT AppListFolderItem : public AppListItem,
  private:
   // AppListItem
   virtual void Activate(int event_flags) OVERRIDE;
-  virtual const char* GetAppType() const OVERRIDE;
+  virtual const char* GetItemType() const OVERRIDE;
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
 
   // AppListItemObserver
diff --git a/ui/app_list/app_list_item.cc b/ui/app_list/app_list_item.cc
index 8440f52..9d35f87 100644
--- a/ui/app_list/app_list_item.cc
+++ b/ui/app_list/app_list_item.cc
@@ -76,7 +76,7 @@ void AppListItem::RemoveObserver(AppListItemObserver* observer) {
 void AppListItem::Activate(int event_flags) {
 }
 
-const char* AppListItem::GetAppType() const {
+const char* AppListItem::GetItemType() const {
   static const char* app_type = "";
   return app_type;
 }
diff --git a/ui/app_list/app_list_item.h b/ui/app_list/app_list_item.h
index 6123caf..69139eb 100644
--- a/ui/app_list/app_list_item.h
+++ b/ui/app_list/app_list_item.h
@@ -59,7 +59,7 @@ class APP_LIST_EXPORT AppListItem {
 
   // Returns a static const char* identifier for the subclass (defaults to "").
   // Pointers can be compared for quick type checking.
-  virtual const char* GetAppType() const;
+  virtual const char* GetItemType() const;
 
   // Returns the context menu model for this item, or NULL if there is currently
   // no menu for the item (e.g. during install).
diff --git a/ui/app_list/app_list_item_list.cc b/ui/app_list/app_list_item_list.cc
index ea76c48..ffcab0f 100644
--- a/ui/app_list/app_list_item_list.cc
+++ b/ui/app_list/app_list_item_list.cc
@@ -92,7 +92,7 @@ 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->GetAppType() == type)
+    if (!type || item->GetItemType() == type)
       DeleteItemAt(i);
   }
 }
diff --git a/ui/app_list/app_list_item_list.h b/ui/app_list/app_list_item_list.h
index 916de70..d52acf2 100644
--- a/ui/app_list/app_list_item_list.h
+++ b/ui/app_list/app_list_item_list.h
@@ -51,7 +51,7 @@ class APP_LIST_EXPORT AppListItemList {
   void DeleteItem(const std::string& id);
 
   // Deletes all items matching |type| which must be a statically defined
-  // type descriptor, e.g. AppListFolderItem::kAppType. If |type| is NULL,
+  // 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);
diff --git a/ui/app_list/app_list_model_unittest.cc b/ui/app_list/app_list_model_unittest.cc
index 2edc2c6..339a4ab7 100644
--- a/ui/app_list/app_list_model_unittest.cc
+++ b/ui/app_list/app_list_model_unittest.cc
@@ -206,10 +206,10 @@ TEST_F(AppListModelTest, ModelRemoveItemByType) {
   model_.PopulateApps(num_apps);
   model_.item_list()->AddItem(new AppListFolderItem("folder1"));
   model_.item_list()->AddItem(new AppListFolderItem("folder2"));
-  model_.item_list()->DeleteItemsByType(test::AppListTestModel::kAppType);
+  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::kAppType);
+  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
diff --git a/ui/app_list/test/app_list_test_model.cc b/ui/app_list/test/app_list_test_model.cc
index fd733d3..d75f5c6 100644
--- a/ui/app_list/test/app_list_test_model.cc
+++ b/ui/app_list/test/app_list_test_model.cc
@@ -10,7 +10,7 @@ namespace app_list {
 namespace test {
 
 // static
-const char AppListTestModel::kAppType[] = "TestItem";
+const char AppListTestModel::kItemType[] = "TestItem";
 
 // AppListTestModel::AppListTestItemModel
 
@@ -27,8 +27,8 @@ void AppListTestModel::AppListTestItemModel::Activate(int event_flags) {
   model_->ItemActivated(this);
 }
 
-const char* AppListTestModel::AppListTestItemModel::GetAppType() const {
-  return AppListTestModel::kAppType;
+const char* AppListTestModel::AppListTestItemModel::GetItemType() const {
+  return AppListTestModel::kItemType;
 }
 
 void AppListTestModel::AppListTestItemModel::SetPosition(
diff --git a/ui/app_list/test/app_list_test_model.h b/ui/app_list/test/app_list_test_model.h
index c9593ef..a3f11f9 100644
--- a/ui/app_list/test/app_list_test_model.h
+++ b/ui/app_list/test/app_list_test_model.h
@@ -22,7 +22,7 @@ class AppListTestModel : public AppListModel {
     AppListTestItemModel(const std::string& id, AppListTestModel* model);
     virtual ~AppListTestItemModel();
     virtual void Activate(int event_flags) OVERRIDE;
-    virtual const char* GetAppType() const OVERRIDE;
+    virtual const char* GetItemType() const OVERRIDE;
 
     void SetPosition(const syncer::StringOrdinal& new_position);
 
@@ -62,7 +62,7 @@ class AppListTestModel : public AppListModel {
   int activate_count() { return activate_count_; }
   AppListItem* last_activated() { return last_activated_; }
 
-  static const char kAppType[];
+  static const char kItemType[];
 
  private:
   void ItemActivated(AppListTestItemModel* item);
diff --git a/ui/app_list/views/app_list_main_view.cc b/ui/app_list/views/app_list_main_view.cc
index 0011a2c..abe5cb2 100644
--- a/ui/app_list/views/app_list_main_view.cc
+++ b/ui/app_list/views/app_list_main_view.cc
@@ -205,7 +205,7 @@ void AppListMainView::OnItemIconLoaded(IconLoader* loader) {
 
 void AppListMainView::ActivateApp(AppListItem* item, int event_flags) {
   // TODO(jennyz): Activate the folder via AppListModel notification.
-  if (item->GetAppType() == AppListFolderItem::kAppType)
+  if (item->GetItemType() == AppListFolderItem::kItemType)
     contents_view_->ShowFolderContent(static_cast<AppListFolderItem*>(item));
   else
     item->Activate(event_flags);
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index 930ad89..ba394d1 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -179,7 +179,7 @@ int GetDistanceBetweenRects(gfx::Rect rect_1,
 
 // Returns true if the |item| is an folder item.
 bool IsFolderItem(AppListItem* item) {
-  return (item->GetAppType() == AppListFolderItem::kAppType);
+  return (item->GetItemType() == AppListFolderItem::kItemType);
 }
 
 // Merges |source_item| into the folder containing the target item specified