Extension syncing: Introduce a NeedsSync pref

chrome/browser/extensions/sync_bundle.cc

 // Copyright 2015 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/sync_bundle.h"
 
 #include "base/location.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_sync_service.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 10 matching lines @@
 void SyncBundle::MergeDataAndStartSyncing(
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor) {
   sync_processor_.reset(sync_processor.release());
 
   for (const syncer::SyncData& sync_data : initial_sync_data) {
     scoped_ptr<ExtensionSyncData> extension_sync_data(
         ExtensionSyncData::CreateFromSyncData(sync_data));
     if (extension_sync_data.get()) {
       AddSyncedExtension(extension_sync_data->id());
-      sync_service_->ApplySyncData(*extension_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 (!sync_service_->ExtensionNeedsSync(extension_sync_data->id()))
+        sync_service_->ApplySyncData(*extension_sync_data);

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

It's odd to me that we make this check here, but the corresponding code which
gives it context is in ExtensionSyncService::MergeDataAndStartSyncing which
calls this method.

In fact can we just get rid of MergeDataAndStartSyncing here? Move it into
ExtensionSyncService::MergeDataAndStartSyncing?

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

Not quite - this also calls AddSyncedExtension for all the items in
initial_sync_data, which still needs to happen. I've moved the actual applying
into ExtensionSyncService though and renamed this to Init.

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

Ah right, thanks. "StartSyncing" would be even better I think.

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

Done.

     }
   }
 }
 
 void SyncBundle::Reset() {
   sync_processor_.reset();
   synced_extensions_.clear();
   pending_sync_data_.clear();
 }
 
 bool SyncBundle::IsSyncing() const {
   return sync_processor_ != nullptr;
 }
 
-bool SyncBundle::HasExtensionId(const std::string& id) const {
-  return synced_extensions_.find(id) != synced_extensions_.end();
-}
-
-bool SyncBundle::ShouldIncludeInLocalSyncDataList(
-    const Extension& extension) const {
-  // If there is pending data for this extension, then this version is out of
-  // date. We'll sync back the version we got from sync.
-  return IsSyncing() && !HasPendingExtensionId(extension.id());
-}
-
 void SyncBundle::PushSyncDataList(
     const syncer::SyncDataList& sync_data_list) {
   syncer::SyncChangeList sync_change_list;
   for (const syncer::SyncData& sync_data : sync_data_list) {
     const syncer::SyncDataLocal sync_data_local(sync_data);
     const std::string& extension_id = sync_data_local.GetTag();
 
     sync_change_list.push_back(CreateSyncChange(extension_id, sync_data));
 
     AddSyncedExtension(extension_id);
   }
 
   PushSyncChanges(sync_change_list);
 }
 
 void SyncBundle::PushSyncDeletion(const std::string& extension_id,
                                   const syncer::SyncData& sync_data) {
+  if (!HasSyncedExtension(extension_id))
+    return;
+
   RemoveSyncedExtension(extension_id);
   PushSyncChanges(syncer::SyncChangeList(1,
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_DELETE,
                          sync_data)));
 }
 
-void SyncBundle::PushSyncAddOrUpdate(const Extension& extension) {
-  syncer::SyncChangeList sync_change_list(
-      1,
-      CreateSyncChange(extension.id(),
-                       sync_service_->CreateSyncData(extension).GetSyncData()));
-  PushSyncChanges(sync_change_list);
-  MarkPendingExtensionSynced(extension.id());
+void SyncBundle::PushSyncAddOrUpdate(const std::string& extension_id,
+                                     const syncer::SyncData& sync_data) {
+  AddSyncedExtension(extension_id);
+  PushSyncChanges(syncer::SyncChangeList(
+      1, CreateSyncChange(extension_id, sync_data)));
+  // Now sync and local state agree. If we had any pending change from sync,
+  // clear it now.
+  pending_sync_data_.erase(extension_id);
 }
 
 void SyncBundle::ApplySyncChange(const syncer::SyncChange& sync_change) {
   scoped_ptr<ExtensionSyncData> extension_sync_data(
       ExtensionSyncData::CreateFromSyncChange(sync_change));
   if (!extension_sync_data.get())
     return;  // TODO(treib,kalman): Warning message?
 
   if (extension_sync_data->uninstalled())
     RemoveSyncedExtension(extension_sync_data->id());
   else
     AddSyncedExtension(extension_sync_data->id());

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

Seems like the wrong place to be adding/removing the extensions. Should be the
job of just observing the ExtensionRegistry.

In fact could you replace AddSyncExtension/RemoveSyncedExtension with querying
the ExtensionRegistry then checking that util::ShouldSync method?

Not something for right now...

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

Almost, but not quite :(
There's some special code in ExtensionService so that uninstalls with
UNINSTALL_REASON_REINSTALL aren't synced.
Yup, I'll look into it. I'd really like to get rid of the almost-pointless
synced_extensions_ list.

   sync_service_->ApplySyncData(*extension_sync_data);
 }
 
 bool SyncBundle::HasPendingExtensionId(const std::string& id) const {
   return pending_sync_data_.find(id) != pending_sync_data_.end();
 }
 
 void SyncBundle::AddPendingExtension(
     const std::string& id,
     const ExtensionSyncData& extension_sync_data) {
   pending_sync_data_.insert(std::make_pair(id, extension_sync_data));
 }
 
 std::vector<ExtensionSyncData> SyncBundle::GetPendingData() const {
   std::vector<ExtensionSyncData> pending_extensions;
   for (const auto& data : pending_sync_data_)
     pending_extensions.push_back(data.second);
 
   return pending_extensions;
 }
 
 syncer::SyncChange SyncBundle::CreateSyncChange(
     const std::string& extension_id,
     const syncer::SyncData& sync_data) const {
   return syncer::SyncChange(
       FROM_HERE,
-      HasExtensionId(extension_id) ? syncer::SyncChange::ACTION_UPDATE
-                                   : syncer::SyncChange::ACTION_ADD,
+      HasSyncedExtension(extension_id) ? syncer::SyncChange::ACTION_UPDATE
+                                       : syncer::SyncChange::ACTION_ADD,
       sync_data);
 }
 
 void SyncBundle::PushSyncChanges(
     const syncer::SyncChangeList& sync_change_list) {
   sync_processor_->ProcessSyncChanges(FROM_HERE, sync_change_list);
 }
 
 void SyncBundle::AddSyncedExtension(const std::string& id) {
   synced_extensions_.insert(id);
 }
 
 void SyncBundle::RemoveSyncedExtension(const std::string& id) {
   synced_extensions_.erase(id);
 }
 
-void SyncBundle::MarkPendingExtensionSynced(const std::string& id) {
-  pending_sync_data_.erase(id);
-  AddSyncedExtension(id);
+bool SyncBundle::HasSyncedExtension(const std::string& id) const {
+  return synced_extensions_.find(id) != synced_extensions_.end();
 }
 
 }  // namespace extensions