Extension syncing: Introduce a NeedsSync pref

chrome/browser/extensions/extension_sync_service.cc

Index: chrome/browser/extensions/extension_sync_service.cc
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 33613fe76b5be2c31fb9b20f4842f3e408538206..e65f1ec18d5ff82862afecd4a30d28739977e76c 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -35,7 +35,6 @@ using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::ExtensionSyncData;
-using extensions::PendingEnables;
 using extensions::SyncBundle;
 
 namespace {
@@ -79,6 +78,15 @@ bool IsCorrectSyncType(const Extension& extension, syncer::ModelType type) {
          (type == syncer::APPS && extension.is_app());
 }
 
+syncer::SyncDataList ToSyncerSyncDataList(
+    const std::vector<ExtensionSyncData>& data) {
+  syncer::SyncDataList result;
+  result.reserve(data.size());
+  for (const ExtensionSyncData& item : data)
+    result.push_back(item.GetSyncData());
+  return result;
+}
+
 }  // namespace
 
 ExtensionSyncService::ExtensionSyncService(Profile* profile,
@@ -88,15 +96,7 @@ ExtensionSyncService::ExtensionSyncService(Profile* profile,
       extension_prefs_(extension_prefs),
       extension_service_(extension_service),
       app_sync_bundle_(this),
-      extension_sync_bundle_(this),
-      pending_app_enables_(make_scoped_ptr(new sync_driver::SyncPrefs(
-                               extension_prefs_->pref_service())),
-                           &app_sync_bundle_,
-                           syncer::APPS),
-      pending_extension_enables_(make_scoped_ptr(new sync_driver::SyncPrefs(
-                                     extension_prefs_->pref_service())),
-                                 &extension_sync_bundle_,
-                                 syncer::EXTENSIONS) {
+      extension_sync_bundle_(this) {
   SetSyncStartFlare(sync_start_util::GetFlareForSyncableService(
       profile_->GetPath()));
 
@@ -129,25 +129,8 @@ void ExtensionSyncService::SyncUninstallExtension(
       flare_.Run(type);  // Tell sync to start ASAP.
     return;
   }
-  const std::string& id = extension.id();
-  if (bundle->HasExtensionId(id))
-    bundle->PushSyncDeletion(id, CreateSyncData(extension).GetSyncData());
-}
-
-void ExtensionSyncService::SyncEnableExtension(const Extension& extension) {
-  // Syncing may not have started yet, so handle pending enables.
-  if (extensions::util::ShouldSync(&extension, profile_))
-    GetPendingEnables(extension.is_app())->Add(extension.id());
-
-  SyncExtensionChangeIfNeeded(extension);
-}
-
-void ExtensionSyncService::SyncDisableExtension(const Extension& extension) {
-  // Syncing may not have started yet, so handle pending enables.
-  if (extensions::util::ShouldSync(&extension, profile_))
-    GetPendingEnables(extension.is_app())->Remove(extension.id());
-
-  SyncExtensionChangeIfNeeded(extension);
+  bundle->PushSyncDeletion(extension.id(),
+                           CreateSyncData(extension).GetSyncData());
 }
 
 void ExtensionSyncService::SyncExtensionChangeIfNeeded(
@@ -158,10 +141,15 @@ void ExtensionSyncService::SyncExtensionChangeIfNeeded(
   syncer::ModelType type =
       extension.is_app() ? syncer::APPS : syncer::EXTENSIONS;
   SyncBundle* bundle = GetSyncBundle(type);
-  if (bundle->IsSyncing())
-    bundle->PushSyncAddOrUpdate(extension);
-  else if (extension_service_->is_ready() && !flare_.is_null())
-    flare_.Run(type);
+  if (bundle->IsSyncing()) {
+    bundle->PushSyncAddOrUpdate(extension.id(),
+                                CreateSyncData(extension).GetSyncData());
+    DCHECK(!ExtensionPrefs::Get(profile_)->NeedsSync(extension.id()));
+  } else {
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), true);
+    if (extension_service_->is_ready() && !flare_.is_null())
+      flare_.Run(type);
+  }
 }
 
 syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
@@ -176,13 +164,15 @@ syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
   SyncBundle* bundle = GetSyncBundle(type);
   bool is_apps = (type == syncer::APPS);
 
+  // Forward to the bundle which will apply the initial sync data, filtering
+  // out any items where we have more recent local changes.
   bundle->MergeDataAndStartSyncing(initial_sync_data, sync_processor.Pass());
-  GetPendingEnables(is_apps)->OnSyncStarted(extension_service_);
 
-  // Process local extensions.
-  // TODO(yoz): Determine whether pending extensions should be considered too.
-  //            See crbug.com/104399.
-  bundle->PushSyncDataList(GetAllSyncData(type));
+  // Now push those local changes to sync.
+  std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true);
+  bundle->PushSyncDataList(ToSyncerSyncDataList(data_list));
+  for (const ExtensionSyncData& data : data_list)
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false);
 
   if (is_apps)
     extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
@@ -196,12 +186,18 @@ void ExtensionSyncService::StopSyncing(syncer::ModelType type) {
 
 syncer::SyncDataList ExtensionSyncService::GetAllSyncData(
     syncer::ModelType type) const {
-  std::vector<ExtensionSyncData> data = GetSyncDataList(type);
-  syncer::SyncDataList result;
-  result.reserve(data.size());
-  for (const ExtensionSyncData& item : data)
-    result.push_back(item.GetSyncData());
-  return result;
+  std::vector<extensions::ExtensionSyncData> sync_data_list =
+      GetLocalSyncDataList(type, false);
+
+  // Add pending data (where the local extension is either not installed yet or
+  // outdated).
+  std::vector<ExtensionSyncData> pending_extensions =
+      GetSyncBundle(type)->GetPendingData();
+  sync_data_list.insert(sync_data_list.begin(),
+                        pending_extensions.begin(),
+                        pending_extensions.end());
+
+  return ToSyncerSyncDataList(sync_data_list);
 }
 
 syncer::SyncError ExtensionSyncService::ProcessSyncChanges(
@@ -280,6 +276,11 @@ bool ExtensionSyncService::ApplySyncData(
   return true;
 }
 
+bool ExtensionSyncService::ExtensionNeedsSync(
+    const std::string& extension_id) const {
+  return ExtensionPrefs::Get(profile_)->NeedsSync(extension_id);
+}
+
 void ExtensionSyncService::ApplyBookmarkAppSyncData(
     const extensions::ExtensionSyncData& extension_sync_data) {
   DCHECK(extension_sync_data.is_app());
@@ -345,12 +346,6 @@ void ExtensionSyncService::SetSyncStartFlare(
   flare_ = flare;
 }
 
-bool ExtensionSyncService::IsPendingEnable(
-    const std::string& extension_id) const {
-  return pending_app_enables_.Contains(extension_id) ||
-      pending_extension_enables_.Contains(extension_id);
-}
-
 SyncBundle* ExtensionSyncService::GetSyncBundle(syncer::ModelType type) {
   return const_cast<SyncBundle*>(
       const_cast<const ExtensionSyncService&>(*this).GetSyncBundle(type));
@@ -361,38 +356,36 @@ const SyncBundle* ExtensionSyncService::GetSyncBundle(
   return (type == syncer::APPS) ? &app_sync_bundle_ : &extension_sync_bundle_;
 }
 
-extensions::PendingEnables* ExtensionSyncService::GetPendingEnables(
-    bool is_apps) {
-  return is_apps ? &pending_app_enables_ : &pending_extension_enables_;
-}
-
-std::vector<ExtensionSyncData> ExtensionSyncService::GetSyncDataList(
-    syncer::ModelType type) const {
+std::vector<ExtensionSyncData> ExtensionSyncService::GetLocalSyncDataList(
+    syncer::ModelType type,
+    bool only_with_needs_sync) const {

[not at google - send to devlin, 2015/07/16 18:25:47]

Sorry for nit but could we reverse this boolean? Make true mean to include all
sync data, false to mean only include things that have changed. Something about
that makes more sense to me. Maybe it's that true > false and all sync data >
only needs sync.

[Marc Treib, 2015/07/17 10:24:06]

Sure, done. Agreed that this makes it easier to read, somehow.

+  // Collect the local state. FillSyncDataList will filter out extensions for
+  // which we have pending data that should be used instead.
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
-  std::vector<ExtensionSyncData> extension_sync_list;
-  FillSyncDataList(registry->enabled_extensions(), type, &extension_sync_list);
-  FillSyncDataList(registry->disabled_extensions(), type, &extension_sync_list);
+  std::vector<ExtensionSyncData> data;
   FillSyncDataList(
-      registry->terminated_extensions(), type, &extension_sync_list);
-
-  std::vector<ExtensionSyncData> pending_extensions =
-      GetSyncBundle(type)->GetPendingData();
-  extension_sync_list.insert(extension_sync_list.begin(),
-                             pending_extensions.begin(),
-                             pending_extensions.end());
-
-  return extension_sync_list;
+      registry->enabled_extensions(), type, only_with_needs_sync, &data);
+  FillSyncDataList(
+      registry->disabled_extensions(), type, only_with_needs_sync, &data);
+  FillSyncDataList(
+      registry->terminated_extensions(), type, only_with_needs_sync, &data);
+  return data;
 }
 
 void ExtensionSyncService::FillSyncDataList(
     const ExtensionSet& extensions,
     syncer::ModelType type,
+    bool only_with_needs_sync,
     std::vector<ExtensionSyncData>* sync_data_list) const {
   const SyncBundle* bundle = GetSyncBundle(type);
+  // TODO(treib,kalman): This seems weird. Remove the IsSyncing check?
+  if (!bundle->IsSyncing())
+    return;
   for (const scoped_refptr<const Extension>& extension : extensions) {
     if (IsCorrectSyncType(*extension, type) &&
         extensions::util::ShouldSync(extension.get(), profile_) &&
-        bundle->ShouldIncludeInLocalSyncDataList(*extension)) {
+        !bundle->HasPendingExtensionId(extension->id()) &&
+        (!only_with_needs_sync || ExtensionNeedsSync(extension->id()))) {
       sync_data_list->push_back(CreateSyncData(*extension));
     }
   }
@@ -448,7 +441,7 @@ bool ExtensionSyncService::ApplyExtensionSyncDataHelper(
       extension_service_->GrantPermissionsAndEnableExtension(extension);
     else
       extension_service_->EnableExtension(id);
-  } else if (!IsPendingEnable(id)) {
+  } else {
     int disable_reasons = extension_sync_data.disable_reasons();
     if (extension_sync_data.remote_install()) {
       if (!(disable_reasons & Extension::DISABLE_REMOTE_INSTALL)) {