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->StartSyncing(sync_processor.Pass());
 
-  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 the extension has local state that needs to be synced, ignore this
+    // change (we assume the local state is more recent).
+    if (extension_sync_data &&
+        !ExtensionPrefs::Get(profile_)->NeedsSync(extension_sync_data->id())) {
+      ApplySyncData(*extension_sync_data);
+    }
+  }
 
-  // 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.
+  // TODO(treib,kalman): We should only have to send out changes for extensions
+  // which have NeedsSync set (i.e. |GetLocalSyncDataList(type, false)|). That
+  // makes some sync_integration_tests fail though - figure out why and fix it!
+  std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true);

[Marc Treib, 2015/07/21 10:54:00]

I still haven't figured out the actual problem here (or whether it's an actual
problem or just an issue with the test setup), so I've reverted to "send
everything on sync startup" for now.
Are you okay with submitting like this for now? The "send only stuff that
actually changed" part is really unrelated to the rest, and I'd like to get the
rest landed...

[not at google - send to devlin, 2015/07/21 13:43:30]

I'll leave it to your judgement. If you think it's ok, go for it.

+  bundle->PushSyncDataList(ToSyncerSyncDataList(data_list));
+  for (const ExtensionSyncData& data : data_list)
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false);
 
-  if (is_apps)
+  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)
+      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 13 matching lines @@
         extension_prefs_->app_sorting()->GetPageOrdinal(extension.id()),
         extensions::GetLaunchTypePrefValue(extension_prefs_, extension.id()));
   }
   return ExtensionSyncData(
       extension, enabled, disable_reasons, incognito_enabled, remote_install,
       allowed_on_all_url);
 }
 
 bool ExtensionSyncService::ApplySyncData(
     const ExtensionSyncData& extension_sync_data) {
+  syncer::ModelType type = extension_sync_data.is_app() ? syncer::APPS
+                                                        : syncer::EXTENSIONS;
+  SyncBundle* bundle = GetSyncBundle(type);
+  bundle->ApplySyncData(extension_sync_data);
+
   const std::string& id = extension_sync_data.id();
 
   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());
     }
 
-    // The corresponding validation of this value during AppSyncData population
-    // is in AppSyncData::PopulateAppSpecifics.
+    // The corresponding validation of this value during ExtensionSyncData
+    // population is in ExtensionSyncData::ToAppSpecifics.
     if (extension_sync_data.launch_type() >= extensions::LAUNCH_TYPE_FIRST &&
         extension_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {
       extensions::SetLaunchType(
           profile_, id, extension_sync_data.launch_type());
     }
 
     if (!extension_sync_data.bookmark_app_url().empty())
       ApplyBookmarkAppSyncData(extension_sync_data);
   }
 
-  syncer::ModelType type = extension_sync_data.is_app() ? syncer::APPS
-                                                        : syncer::EXTENSIONS;
-
   if (!ApplyExtensionSyncDataHelper(extension_sync_data, type)) {
-    GetSyncBundle(type)->AddPendingExtension(id, extension_sync_data);
+    bundle->AddPendingExtension(id, extension_sync_data);
     extension_service_->CheckForUpdatesSoon();
     return false;
   }
 
   return true;
 }
 
 void ExtensionSyncService::ApplyBookmarkAppSyncData(
     const extensions::ExtensionSyncData& extension_sync_data) {
   DCHECK(extension_sync_data.is_app());
@@ skipping 52 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;
+  // TODO(treib, kalman): 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.
   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, 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 ||
+         ExtensionPrefs::Get(profile_)->NeedsSync(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;
 }