Extension syncing: Introduce a NeedsSync pref

chrome/browser/extensions/extension_sync_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_sync_service.h"
 
 #include "base/basictypes.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
@@ skipping 17 matching lines @@
 #include "extensions/common/extension_set.h"
 #include "extensions/common/image_util.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
 
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::ExtensionSyncData;
-using extensions::PendingEnables;
 using extensions::SyncBundle;
 
 namespace {
 
 void OnWebApplicationInfoLoaded(
     WebApplicationInfo synced_info,
     base::WeakPtr<ExtensionService> extension_service,
     const WebApplicationInfo& loaded_info) {
   DCHECK_EQ(synced_info.app_url, loaded_info.app_url);
 
@@ skipping 23 matching lines @@
   // Otherwise, unset.
   return ExtensionSyncData::BOOLEAN_UNSET;
 }
 
 // Returns true if the sync type of |extension| matches |type|.
 bool IsCorrectSyncType(const Extension& extension, syncer::ModelType type) {
   return (type == syncer::EXTENSIONS && extension.is_extension()) ||
          (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,
                                            ExtensionPrefs* extension_prefs,
                                            ExtensionService* extension_service)
     : 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_service_(extension_service) {
   SetSyncStartFlare(sync_start_util::GetFlareForSyncableService(
       profile_->GetPath()));
 
   extension_service_->set_extension_sync_service(this);
   extension_prefs_->app_sorting()->SetExtensionSyncService(this);
 }
 
 ExtensionSyncService::~ExtensionSyncService() {}
 
 // static
 ExtensionSyncService* ExtensionSyncService::Get(
     content::BrowserContext* context) {
   return ExtensionSyncServiceFactory::GetForBrowserContext(context);
 }
 
 void ExtensionSyncService::SyncUninstallExtension(
     const extensions::Extension& extension) {
   if (!extensions::util::ShouldSync(&extension, profile_))
     return;
 
   // TODO(tim): If we get here and IsSyncing is false, this will cause
   // "back from the dead" style bugs, because sync will add-back the extension
   // that was uninstalled here when MergeDataAndStartSyncing is called.
   // See crbug.com/256795.
   syncer::ModelType type =
       extension.is_app() ? syncer::APPS : syncer::EXTENSIONS;
   SyncBundle* bundle = GetSyncBundle(type);
-  if (!bundle->IsSyncing()) {
-    if (extension_service_->is_ready() && !flare_.is_null())
-      flare_.Run(type);  // Tell sync to start ASAP.
-    return;
+  if (bundle->IsSyncing()) {
+    bundle->PushSyncDeletion(extension.id(),
+                             CreateSyncData(extension).GetSyncData());
+  } else if (extension_service_->is_ready() && !flare_.is_null()) {
+    flare_.Run(type);  // Tell sync to start ASAP.
   }
-  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);
 }
 
 void ExtensionSyncService::SyncExtensionChangeIfNeeded(
     const Extension& extension) {
   if (!extensions::util::ShouldSync(&extension, profile_))
     return;
 
   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);  // Tell sync to start ASAP.
+  }
 }
 
 syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
     scoped_ptr<syncer::SyncErrorFactory> sync_error_factory) {
   CHECK(sync_processor.get());
   LOG_IF(FATAL, type != syncer::EXTENSIONS && type != syncer::APPS)
       << "Got " << type << " ModelType";
 
   SyncBundle* bundle = GetSyncBundle(type);
-  bool is_apps = (type == syncer::APPS);
+  bundle->Init(sync_processor.Pass());

[not at google - send to devlin, 2015/07/17 15:28:50]

(See "Init" --> "StartSyncing" comment - it's more descriptive and still fixes
the "MergeData"/interface-same-naming confusion)

 
-  bundle->MergeDataAndStartSyncing(initial_sync_data, sync_processor.Pass());
-  GetPendingEnables(is_apps)->OnSyncStarted(extension_service_);
+  // Apply the initial sync data, filtering out any items where we have more
+  // recent local changes. Also tell the SyncBundle the extension IDs.
+  for (const syncer::SyncData& sync_data : initial_sync_data) {
+    scoped_ptr<ExtensionSyncData> extension_sync_data(
+        ExtensionSyncData::CreateFromSyncData(sync_data));
+    if (extension_sync_data.get()) {

[not at google - send to devlin, 2015/07/17 15:28:50]

(incidentally you don't need the .get() calls for scoped_ptr<> null checks, but
it's not necessary to change it - whichever you prefer)

[Marc Treib, 2015/07/20 09:28:22]

Done.

+      bundle->ApplySyncData(*extension_sync_data);

[not at google - send to devlin, 2015/07/17 15:28:50]

Why isn't this guarded behind the ExtensionNeedsSync(...) check?

In fact if you're not applying these sync changes (this->ApplySyncData) should
you be updating that (as we established, silly) synced_extenison_list_ structure
in SyncBundle at all?

(i.e. should bundle->ApplySyncData call move into this->ApplySyncData)

It would be even nice to change bundle->ApplySyncData into those 2 add/remove
methods because bundle->ApplySyncData doesn't actually do anything with the sync
data, which is misleading.

However as established the best solution would be to remove that
synced_extensions_ concept entirely, so you might as well hold off any further
cleanup here until then. I am really puzzled by the whole thing (in fact I don't
quite understand your response to the ExtensionRegistry comment but it's not
important right now).

[Marc Treib, 2015/07/20 09:28:22]

Yeah, that makes sense. Done.
I didn't want to make the SyncBundle::Add/RemoveSyncedExtension methods public.
Let's try to keep that crap as local as possible :)
SyncBundle::ApplySyncData isn't really a good name anymore, but oh well.
Hopefully it'll soon be gone anyway.
There's a case where an extension is uninstalled (and removed from the
ExtensionRegistry), but that change is *not* propagated to Sync.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

 
-  // Process local extensions.
-  // TODO(yoz): Determine whether pending extensions should be considered too.
-  //            See crbug.com/104399.
-  bundle->PushSyncDataList(GetAllSyncData(type));
+      // If the extension has local state that needs to be synced, ignore this
+      // change (we assume the local state is more recent).
+      if (!ExtensionNeedsSync(extension_sync_data->id()))
+        ApplySyncData(*extension_sync_data);
+    }
+  }
 
-  if (is_apps)
+  // Now push those local changes to sync.
+  std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, false);
+  bundle->PushSyncDataList(ToSyncerSyncDataList(data_list));
+  for (const ExtensionSyncData& data : data_list)
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false);

[not at google - send to devlin, 2015/07/17 15:28:50]

I find it a little bit weird that querying this bit gets its own method
(ExtensionNeedsSync) while setting it doesn't. I'd have gone with just querying
the prefs directly. Not that important though.

[Marc Treib, 2015/07/20 09:28:22]

Ah, that's from a previous patch set where SyncBundle needed that info. Now it's
really unnecessary; I've removed it.

+
+  if (type == syncer::APPS)
     extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
 
   return syncer::SyncMergeResult(type);
 }
 
 void ExtensionSyncService::StopSyncing(syncer::ModelType type) {
   GetSyncBundle(type)->Reset();
 }
 
 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;
+  const SyncBundle* bundle = GetSyncBundle(type);
+  if (!bundle->IsSyncing())
+    return syncer::SyncDataList();
+
+  std::vector<extensions::ExtensionSyncData> sync_data_list =
+      GetLocalSyncDataList(type, true);
+
+  // Add pending data (where the local extension is either not installed yet or
+  // outdated).
+  std::vector<ExtensionSyncData> pending_extensions = bundle->GetPendingData();
+  sync_data_list.insert(sync_data_list.begin(),
+                        pending_extensions.begin(),
+                        pending_extensions.end());
+
+  return ToSyncerSyncDataList(sync_data_list);
 }
 
 syncer::SyncError ExtensionSyncService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   for (const syncer::SyncChange& sync_change : change_list) {
-    syncer::ModelType type = sync_change.sync_data().GetDataType();
-    GetSyncBundle(type)->ApplySyncChange(sync_change);
+    scoped_ptr<ExtensionSyncData> extension_sync_data(
+        ExtensionSyncData::CreateFromSyncChange(sync_change));
+    if (extension_sync_data.get()) {
+      syncer::ModelType type = sync_change.sync_data().GetDataType();
+      GetSyncBundle(type)->ApplySyncData(*extension_sync_data);
+      ApplySyncData(*extension_sync_data);
+    }
   }
 
   extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
 
   return syncer::SyncError();
 }
 
 ExtensionSyncData ExtensionSyncService::CreateSyncData(
     const extensions::Extension& extension) const {
   bool enabled = extension_service_->IsExtensionEnabled(extension.id());
@@ skipping 50 matching lines @@
 
   if (!ApplyExtensionSyncDataHelper(extension_sync_data, type)) {
     GetSyncBundle(type)->AddPendingExtension(id, extension_sync_data);
     extension_service_->CheckForUpdatesSoon();
     return false;
   }
 
   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());
 
   // Process bookmark app sync if necessary.
   GURL bookmark_app_url(extension_sync_data.bookmark_app_url());
   if (!bookmark_app_url.is_valid() ||
       extension_sync_data.uninstalled()) {
     return;
   }
@@ skipping 45 matching lines @@
 
   if (ext)
     SyncExtensionChangeIfNeeded(*ext);
 }
 
 void ExtensionSyncService::SetSyncStartFlare(
     const syncer::SyncableService::StartSyncFlare& flare) {
   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));
 }
 
 const SyncBundle* ExtensionSyncService::GetSyncBundle(
     syncer::ModelType type) const {
   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 include_everything) 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);
-  FillSyncDataList(registry->disabled_extensions(), type, &extension_sync_list);
+  std::vector<ExtensionSyncData> data;
   FillSyncDataList(

[not at google - send to devlin, 2015/07/17 15:28:50]

Could you add:

// TODO(trieb): Should we be including blacklisted/blocked extensions here?
// I.e. just calling registry->GeneratedInstalledExtensionsSet()?
// It would be more consistent, but the danger is that the black/blocklist
// hasn't been updated on all clients by the time sync has kicked in - 
// so it's safest not to. Take care to add any other extension lists here
// in the future if they are added.

[Marc Treib, 2015/07/20 09:28:22]

Sure, done. I've also added your name in the TODO(..) though ;P

-      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, include_everything, &data);
+  FillSyncDataList(
+      registry->disabled_extensions(), type, include_everything, &data);
+  FillSyncDataList(
+      registry->terminated_extensions(), type, include_everything, &data);
+  return data;
 }
 
 void ExtensionSyncService::FillSyncDataList(
     const ExtensionSet& extensions,
     syncer::ModelType type,
+    bool include_everything,
     std::vector<ExtensionSyncData>* sync_data_list) const {
   const SyncBundle* bundle = GetSyncBundle(type);
   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()) &&
+        (include_everything || ExtensionNeedsSync(extension->id()))) {
       sync_data_list->push_back(CreateSyncData(*extension));
     }
   }
 }
 
 bool ExtensionSyncService::ApplyExtensionSyncDataHelper(
     const ExtensionSyncData& extension_sync_data,
     syncer::ModelType type) {
   const std::string& id = extension_sync_data.id();
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
@@ skipping 35 matching lines @@
     // local one. Otherwise we just enable it without granting permissions. If
     // any permissions are missing, CheckPermissionsIncrease will soon disable
     // it again.
     bool grant_permissions =
         extension_sync_data.supports_disable_reasons() &&
         extension && (version_compare_result == 0);
     if (grant_permissions)
       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)) {
         // In the non-legacy case (>=M45) where disable reasons are synced at
         // all, DISABLE_REMOTE_INSTALL should be among them already.
         DCHECK(!extension_sync_data.supports_disable_reasons());
         disable_reasons |= Extension::DISABLE_REMOTE_INSTALL;
       }
     } else if (!extension_sync_data.supports_disable_reasons()) {
       // Legacy case (<M45), from before we synced disable reasons (see
@@ skipping 51 matching lines @@
       // non-INTERNAL location.  Add to pending_sync_data, even though it will
       // never be removed (we'll never install a syncable version of the
       // extension), so that GetAllSyncData() continues to send it.
     }
     // Track pending extensions so that we can return them in GetAllSyncData().
     return false;
   }
 
   return true;
 }