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 c01cd7a08f0228b385e4a677fd9ca5a6bc7fa8e9..cd86bb2d846eea18d50b3c97dc61f7c45b4c356c 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());

[not at google - send to devlin, 2015/07/15 20:27:57]

You should be able to do this in the constructor; syncer::SyncDataList
result(data.size(), ExtensionSyncData()); (or maybe the arguments are reversed).

[Marc Treib, 2015/07/16 09:22:34]

Actually no, that would do something different: It'd create |data.size()| empty
ExtensionSyncDatas (which wouldn't work because the default constructor is
private).
reserve() OTOH just allocates memory, but doesn't construct the elements.
Calling it at all isn't strictly necessary, it's just a small optimization to
avoid some re-allocations.

+  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()));
 
@@ -134,22 +134,6 @@ void ExtensionSyncService::SyncUninstallExtension(
     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);
-}
-
 void ExtensionSyncService::SyncExtensionChangeIfNeeded(
     const Extension& extension) {
   if (!extensions::util::ShouldSync(&extension, profile_))
@@ -158,10 +142,14 @@ void ExtensionSyncService::SyncExtensionChangeIfNeeded(
   syncer::ModelType type =
       extension.is_app() ? syncer::APPS : syncer::EXTENSIONS;
   SyncBundle* bundle = GetSyncBundle(type);
-  if (bundle->IsSyncing())
-    bundle->PushSyncChangeIfNeeded(extension);
-  else if (extension_service_->is_ready() && !flare_.is_null())
+  if (bundle->IsSyncing()) {
+    bundle->PushSyncChange(extension.id(),
+                           CreateSyncData(extension).GetSyncData());
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), false);

[not at google - send to devlin, 2015/07/15 20:27:57]

I think this only needs to go at the point when the extensions that need syncing
are pushed out, not here?

[Marc Treib, 2015/07/16 09:22:34]

I think you're right. By the time we get here, the pref should already have been
cleared. I've put in a DCHECK instead.

+  } else if (extension_service_->is_ready() && !flare_.is_null()) {

[not at google - send to devlin, 2015/07/15 20:27:57]

It seems like these 2 cases should be separated.

1. if the bundle is syncing, push the sync change. If not, mark it as needing
syncing in prefs.

2. if the sync flare hasn't been run yet, and the extension system is ready, run
the flare.

[Marc Treib, 2015/07/16 09:22:34]

Done.
I think this should still go in the "else - if the bundle is syncing already,
there's no point in running the flare.
Also, the extension_service_->is_ready() check seems redundant - we're being
called by the extension service here, so it must be ready, right?

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

Ok good point. I still think it shouldn't be in an else because it's not
logically related, and that can be a source of bugs. As for the ready thing, I
never really understood that, but it's not a matter of correlating between the
existence of an extensionservice and whether it's ready.

Though generally speaking we probably check ready() where we don't need to, and
don't check it when we should. Eh.

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

Hm, turns out there's a test (ExtensionServiceTest.DeferredSyncStartupOnInstall)
that makes sure the flare isn't run after sync has started. So maybe it actually
matters? I've left it in the "!IsSyncing" branch for now...

+    ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), true);
     flare_.Run(type);
+  }
 }
 
 syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
@@ -177,12 +165,14 @@ syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
   bool is_apps = (type == syncer::APPS);
 
   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));
+  std::vector<ExtensionSyncData> data_list = GetSyncDataList(type);
+  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,7 @@ 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;
+  return ToSyncerSyncDataList(GetSyncDataList(type));
 }
 
 syncer::SyncError ExtensionSyncService::ProcessSyncChanges(
@@ -245,6 +230,12 @@ bool ExtensionSyncService::ApplySyncData(
     const ExtensionSyncData& extension_sync_data) {
   const std::string& id = extension_sync_data.id();
 
+  // If the extension has local state that needs to be synced, ignore this
+  // change (we assume the local state is more recent). This should only ever
+  // happen during MergeDataAndStartSyncing.
+  if (ExtensionPrefs::Get(profile_)->NeedsSync(id))

[not at google - send to devlin, 2015/07/15 20:27:57]

I don't think this is the right place for this. Instead we should be filtering
changes to push back out to sync based on whether the pref is set... sort of a
reversal of the logic. Basically up on line 173 the call
"bundle->PushSyncDataList" should only contain the extensions that have changed.

[Marc Treib, 2015/07/16 09:22:34]

That's two different things. What MergeDataAndStartSyncing does is:
1. Apply the initial sync data.
2. Push out all the local state.
You're proposing that in 2. we should only push what's actually changed (which
makes sense, but is just an optimization).
But what this code here does is during 1., filter out those sync changes for
which we have more recent local changes. That's actually required to make all
this work.
I do agree that here isn't really the right place for it though. I've moved the
check into SyncBundle::MergeDataAndStartSyncing.

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

In a way it's an optimisation, but only because code which pushes out "changes"
for everything is grossly inefficient and the wrong contract from a sync
interface perspective. The point of sync is that you only say what changed.
Sure, sync will say "hey wait a minute you said this thing changed but it
didn't, I'm going to ignore it" but it's still *wrong*.

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

Still "just" an optimization ;P
Anyway, done in PS4 already :)

+    return true;
+
   if (extension_sync_data.is_app()) {
     if (extension_sync_data.app_launch_ordinal().IsValid() &&
         extension_sync_data.page_ordinal().IsValid()) {
@@ -345,12 +336,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,13 +346,10 @@ 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 {
+  // 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);
@@ -375,6 +357,8 @@ std::vector<ExtensionSyncData> ExtensionSyncService::GetSyncDataList(
   FillSyncDataList(
       registry->terminated_extensions(), type, &extension_sync_list);
 
+  // Add pending data (where the local extension is either not installed yet or
+  // outdated).
   std::vector<ExtensionSyncData> pending_extensions =
       GetSyncBundle(type)->GetPendingData();
   extension_sync_list.insert(extension_sync_list.begin(),
@@ -392,7 +376,8 @@ void ExtensionSyncService::FillSyncDataList(
   for (const scoped_refptr<const Extension>& extension : extensions) {
     if (IsCorrectSyncType(*extension, type) &&
         extensions::util::ShouldSync(extension.get(), profile_) &&
-        bundle->ShouldIncludeInLocalSyncDataList(*extension)) {
+        bundle->IsSyncing() &&

[not at google - send to devlin, 2015/07/15 20:27:57]

Should this be a (D)CHECK?

[Marc Treib, 2015/07/16 09:22:34]

Not sure - can GetAllSyncData (from syncer::SyncableService) be called before
MergeDataAndStartSyncing?
Anyway, I don't see the point in the IsSyncing check. Maybe we should just
remove it?

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

Removing SGTM.

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

Done.

+        !bundle->HasPendingExtensionId(extension->id())) {
       sync_data_list->push_back(CreateSyncData(*extension));
     }
   }
@@ -448,7 +433,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)) {