Sync removal of Default apps.

This CL implements TYPE_REMOVE_DEFAULT_APP. It also includes a fair bit
of cleanup of AppListSyncableService, which on the one hand I apologize
for, but on the other hand I think the result is more straightforward.

BUG=258758

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

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

chrome/app/generated_resources.grd

@@ -6514,6 +6514,12 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_FLAGS_ENABLE_SYNCED_NOTIFICATIONS_DESCRIPTION" desc="Description for the flag to enable synced notifications.">
         Enable experimental Synchronized Notifications.
       </message>
+      <message name="IDS_FLAGS_ENABLE_SYNC_APP_LIST_NAME" desc="Name of the flag to enable syncing the app list.">
+        Enable App Launcher Sync
+      </message>
+      <message name="IDS_FLAGS_ENABLE_SYNC_APP_LIST_DESCRIPTION" desc="Description for the flag to enable syncing the app list.">
+        Enable experimental App Launcher Sync.
+      </message>
       <message name="IDS_FLAGS_ENABLE_SCREEN_CAPTURE_NAME" desc="Name of the flag to enable screen capture support.">
         Enable screen capture support in getUserMedia().
       </message>

chrome/browser/about_flags.cc

@@ -1400,6 +1400,15 @@ const Experiment kExperiments[] = {
     ENABLE_DISABLE_VALUE_TYPE(switches::kEnableSyncSyncedNotifications,
                               switches::kDisableSyncSyncedNotifications)
   },
+#if defined(ENABLE_APP_LIST)
+  {
+    "enable-sync-app-list",
+    IDS_FLAGS_ENABLE_SYNC_APP_LIST_NAME,
+    IDS_FLAGS_ENABLE_SYNC_APP_LIST_DESCRIPTION,
+    kOsDesktop,
+    SINGLE_VALUE_TYPE(switches::kEnableSyncAppList)
+  },
+#endif
   {
     "disable-full-history-sync",
     IDS_FLAGS_FULL_HISTORY_SYNC_NAME,

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

@@ -54,13 +54,8 @@ bool SyncAppListHelper::AppListMatchesVerifier(Profile* profile) {
       AppListSyncableServiceFactory::GetForProfile(profile);
   AppListSyncableService* verifier =
       AppListSyncableServiceFactory::GetForProfile(test_->verifier());
-  if (service->GetNumSyncItemsForTest() !=
-      verifier->GetNumSyncItemsForTest()) {
-    LOG(ERROR) << "Sync item count: "
-               << service->GetNumSyncItemsForTest()
-               << " != " << verifier->GetNumSyncItemsForTest();
-    return false;
-  }
+  // Note: sync item entries may not exist in verifier, but item lists should
+  // match.
   if (service->model()->item_list()->item_count() !=
       verifier->model()->item_list()->item_count()) {
     LOG(ERROR) << "Model item count: "

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

@@ -35,6 +35,14 @@ bool AllProfilesHaveSameAppListAsVerifier() {
       AllProfilesHaveSameAppListAsVerifier();
 }
 
+const app_list::AppListSyncableService::SyncItem* GetSyncItem(
+    Profile* profile,
+    const std::string& app_id) {
+  app_list::AppListSyncableService* service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile);
+  return service->GetSyncItem(app_id);
+}
+
 }  // namespace
 
 class TwoClientAppListSyncTest : public SyncTest {
@@ -398,3 +406,66 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppListSyncTest, Move) {
   ASSERT_TRUE(AwaitQuiescence());
   ASSERT_TRUE(AllProfilesHaveSameAppListAsVerifier());
 }
+
+// Install a Default App on both clients, then sync. Remove the app on one
+// client and sync. Ensure that the app is removed on the other client and
+// that a REMOVE_DEFAULT_APP entry exists.
+IN_PROC_BROWSER_TEST_F(TwoClientAppListSyncTest, RemoveDefault) {
+  ASSERT_TRUE(SetupClients());
+  ASSERT_TRUE(SetupSync());
+
+  // Install a non-default app.
+  InstallApp(GetProfile(0), 0);
+  InstallApp(GetProfile(1), 0);
+  InstallApp(verifier(), 0);
+
+  // Install a default app in Profile 0 only.
+  const int default_app_index = 1;
+  std::string default_app_id = InstallApp(GetProfile(0), default_app_index);
+  InstallApp(verifier(), default_app_index);
+  SyncAppListHelper::GetInstance()->CopyOrdinalsToVerifier(
+      GetProfile(0), default_app_id);
+
+  ASSERT_TRUE(AwaitQuiescence());
+  InstallAppsPendingForSync(GetProfile(0));
+  InstallAppsPendingForSync(GetProfile(1));
+  ASSERT_TRUE(AllProfilesHaveSameAppListAsVerifier());
+
+  // Flag Default app in Profile 1.
+  extensions::ExtensionSystem::Get(GetProfile(1))->
+      extension_service()->extension_prefs()->
+      UpdateExtensionPref(default_app_id, "was_installed_by_default",
+                          new base::FundamentalValue(true));
+
+  // Remove the default app in Profile 0 and verifier, ensure it was removed
+  // in Profile 1.
+  UninstallApp(GetProfile(0), default_app_index);
+  UninstallApp(verifier(), default_app_index);
+  ASSERT_TRUE(AwaitQuiescence());
+  ASSERT_TRUE(AllProfilesHaveSameAppListAsVerifier());
+
+  // Ensure that a REMOVE_DEFAULT_APP SyncItem entry exists in Profile 1.
+  const app_list::AppListSyncableService::SyncItem* sync_item =
+      GetSyncItem(GetProfile(1), default_app_id);
+  ASSERT_TRUE(sync_item);
+  ASSERT_EQ(sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP,
+            sync_item->item_type);
+
+  // Re-Install the same app in Profile 0.
+  std::string app_id2 = InstallApp(GetProfile(0), default_app_index);
+  EXPECT_EQ(default_app_id, app_id2);
+  InstallApp(verifier(), default_app_index);
+  sync_item = GetSyncItem(GetProfile(0), app_id2);
+  EXPECT_EQ(sync_pb::AppListSpecifics::TYPE_APP, sync_item->item_type);
+
+  ASSERT_TRUE(AwaitQuiescence());
+  InstallAppsPendingForSync(GetProfile(0));
+  InstallAppsPendingForSync(GetProfile(1));
+  ASSERT_TRUE(AllProfilesHaveSameAppListAsVerifier());
+
+  // Ensure that the REMOVE_DEFAULT_APP SyncItem entry in Profile 1 is replaced
+  // with an APP entry after an install.
+  sync_item = GetSyncItem(GetProfile(1), app_id2);
+  ASSERT_TRUE(sync_item);
+  EXPECT_EQ(sync_pb::AppListSpecifics::TYPE_APP, sync_item->item_type);
+}

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -19,11 +19,13 @@
 #include "sync/api/syncable_service.h"
 #include "sync/protocol/app_list_specifics.pb.h"
 
-class ExtensionAppItem;
 class ExtensionAppModelBuilder;
-class ExtensionService;
 class Profile;
 
+namespace extensions {
+class ExtensionSystem;
+}
+
 namespace sync_pb {
 class AppListSpecifics;
 }
@@ -33,8 +35,7 @@ namespace app_list {
 class AppListModel;
 class AppListItem;
 
-// Keyed Service that owns, stores, and syncs an AppListModel for an
-// ExtensionSystem and corresponding profile.
+// Keyed Service that owns, stores, and syncs an AppListModel for a profile.
 class AppListSyncableService : public syncer::SyncableService,
                                public BrowserContextKeyedService,
                                public content::NotificationObserver {
@@ -53,18 +54,18 @@ class AppListSyncableService : public syncer::SyncableService,
     std::string ToString() const;
   };
 
-  // Create an empty model. Then, if |extension_service| is non-NULL and ready,
-  // populate it. Otherwise populate the model once extensions become ready.
-  AppListSyncableService(Profile* profile, ExtensionService* extension_service);
+  // Populates the model when |extension_system| is ready.
+  AppListSyncableService(Profile* profile,
+                         extensions::ExtensionSystem* extension_system);
 
   virtual ~AppListSyncableService();
 
-  // Adds |item| to |sync_items_| and |model_|. Does noting if a sync item
+  // Adds |item| to |sync_items_| and |model_|. Does nothing if a sync item
   // already exists.
-  void AddExtensionAppItem(ExtensionAppItem* item);
+  void AddItem(AppListItem* item);
 
   // Updates existing entry in |sync_items_| from |item|.
-  void UpdateExtensionAppItem(ExtensionAppItem* item);
+  void UpdateItem(AppListItem* item);
 
   // Removes sync item matching |id|.
   void RemoveItem(const std::string& id);
@@ -100,15 +101,19 @@ class AppListSyncableService : public syncer::SyncableService,
   // Builds the model once ExtensionService is ready.
   void BuildModel();
 
-  // Creates a new Appitem from |sync_item| and adds it to the model.
-  void CreateAppItemFromSyncItem(SyncItem* sync_item);
-
   // Returns true if sync has restarted, otherwise runs |flare_|.
   bool SyncStarted();
 
-  // Creates a SyncItem entry and adds |item| to the model.
-  SyncItem* AddItem(sync_pb::AppListSpecifics::AppListItemType type,
-                    AppListItem* item);
+  // Creates or updates a SyncItem from |specifics|. Returns true if a new item
+  // was created.
+  bool ProcessSyncItem(const sync_pb::AppListSpecifics& specifics);
+
+  // 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).
+  void ProcessExistingSyncItem(SyncItem* sync_item);
 
   // Sends ADD or CHANGED for sync item.
   void SendSyncChange(SyncItem* sync_item,
@@ -117,21 +122,16 @@ class AppListSyncableService : public syncer::SyncableService,
   // Returns an existing SyncItem corresponding to |item_id| or NULL.
   SyncItem* FindSyncItem(const std::string& item_id);
 
-  // Returns a SyncItem corresponding to |item_id|. Sets |new_item| if an item
-  // was created.
-  SyncItem* FindOrCreateSyncItem(
+  // Creates a new sync item for |item_id|.
+  SyncItem* CreateSyncItem(
       const std::string& item_id,
-      sync_pb::AppListSpecifics::AppListItemType type,
-      bool* new_item);
-
-  // Creates or updates a SyncItem from |specifics|. Returns true if an item
-  // was created.
-  bool CreateOrUpdateSyncItem(const sync_pb::AppListSpecifics& specifics);
+      sync_pb::AppListSpecifics::AppListItemType item_type);
 
   // Deletes a SyncItem matching |specifics|.
   void DeleteSyncItem(const sync_pb::AppListSpecifics& specifics);
 
   Profile* profile_;
+  extensions::ExtensionSystem* extension_system_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<AppListModel> model_;
   scoped_ptr<ExtensionAppModelBuilder> apps_builder_;

chrome/browser/ui/app_list/app_list_syncable_service_factory.cc

@@ -40,9 +40,8 @@ BrowserContextKeyedService*
 AppListSyncableServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* browser_context) const {
   Profile* profile = static_cast<Profile*>(browser_context);
-  ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(profile)->extension_service();
-  return new AppListSyncableService(profile, extension_service);
+  return new AppListSyncableService(profile,
+                                    extensions::ExtensionSystem::Get(profile));
 }
 
 void AppListSyncableServiceFactory::RegisterProfilePrefs(

chrome/browser/ui/app_list/extension_app_model_builder.cc

@@ -220,7 +220,7 @@ void ExtensionAppModelBuilder::PopulateApps() {
 
 void ExtensionAppModelBuilder::InsertApp(ExtensionAppItem* app) {
   if (service_) {
-    service_->AddExtensionAppItem(app);
+    service_->AddItem(app);
     return;
   }
   model_->item_list()->AddItem(app);
@@ -271,9 +271,8 @@ void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index,
   if (item->GetAppType() != ExtensionAppItem::kAppType)
     return;
 
-  ExtensionAppItem* extension_item = static_cast<ExtensionAppItem*>(item);
   if (service_) {
-    service_->UpdateExtensionAppItem(extension_item);
+    service_->UpdateItem(item);
     return;
   }
 
@@ -295,5 +294,5 @@ void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index,
   }
   // item->Move will call set_position, overriding the item's position.
   if (prev || next)
-    extension_item->Move(prev, next);
+    static_cast<ExtensionAppItem*>(item)->Move(prev, next);
 }