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());

[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,
                                            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_sync_bundle_(this) {
   SetSyncStartFlare(sync_start_util::GetFlareForSyncableService(
       profile_->GetPath()));
 
   extension_service_->set_extension_sync_service(this);
   extension_prefs_->app_sorting()->SetExtensionSyncService(this);
 }
 
 ExtensionSyncService::~ExtensionSyncService() {}
 
 // static
@@ skipping 17 matching lines @@
   if (!bundle->IsSyncing()) {
     if (extension_service_->is_ready() && !flare_.is_null())
       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);
-}
-
 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->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(
     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->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();
 
   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;
+  return ToSyncerSyncDataList(GetSyncDataList(type));
 }
 
 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);
   }
 
@@ skipping 23 matching lines @@
   }
   return ExtensionSyncData(
       extension, enabled, disable_reasons, incognito_enabled, remote_install,
       allowed_on_all_url);
 }
 
 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()) {
       extension_prefs_->app_sorting()->SetAppLaunchOrdinal(
           id,
           extension_sync_data.app_launch_ordinal());
       extension_prefs_->app_sorting()->SetPageOrdinal(
           id,
           extension_sync_data.page_ordinal());
     }
@@ skipping 80 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 {
+  // 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);
   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(),
                              pending_extensions.begin(),
                              pending_extensions.end());
 
   return extension_sync_list;
 }
 
 void ExtensionSyncService::FillSyncDataList(
     const ExtensionSet& extensions,
     syncer::ModelType type,
     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->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));
     }
   }
 }
 
 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;
 }