ExtensionSyncService cleanup: process uninstalls in one step

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 94 matching lines @@
 }
 
 ExtensionSyncService::~ExtensionSyncService() {}
 
 // static
 ExtensionSyncService* ExtensionSyncService::Get(
     content::BrowserContext* context) {
   return ExtensionSyncServiceFactory::GetForBrowserContext(context);
 }
 
-syncer::SyncData ExtensionSyncService::PrepareToSyncUninstallExtension(
-    const Extension& extension) {
-  // Extract the data we need for sync now, but don't actually sync until we've
-  // completed the uninstallation.
+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;
-  const SyncBundle* bundle = GetSyncBundle(type);
-  if (extensions::util::ShouldSync(&extension, profile_)) {
-    if (bundle->IsSyncing())
-      return CreateSyncData(extension).GetSyncData();
+  SyncBundle* bundle = GetSyncBundle(type);
+  if (!bundle->IsSyncing()) {
     if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(type);  // Tell sync to start ASAP.
+    return;
   }
-
-  return syncer::SyncData();
-}
-
-void ExtensionSyncService::ProcessSyncUninstallExtension(
-    const std::string& extension_id,
-    const syncer::SyncData& sync_data) {
-  SyncBundle* bundle = GetSyncBundle(sync_data.GetDataType());
-  if (bundle->HasExtensionId(extension_id))
-    bundle->PushSyncDeletion(extension_id, sync_data);
+  const std::string& id = extension.id();
+  if (bundle->HasExtensionId(id))
+    bundle->PushSyncDeletion(id, CreateSyncData(extension).GetSyncData());

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

Btw it would be nice if this function could mostly be replaced by
"this->SyncExtensionChangeIfNeeded". Conceptually it's the same thing, just one
pushes a delete and the other pushes an add/update.

The weird thing is that bundle->PushSyncChangeIfNeeded doesn't take the SyncData
as an argument and instead constructs its own, whereas on the other hand
bundle->PushSyncDeletion *does*. Instead it turns around and calls
CreateSyncData on *this*. It would be much simpler to just pass the sync data
into bundle->PushSyncChangeIfNeeded.

The other weird thing is that it's called "PushSyncChangeIfNeeded" whereas it
actually always pushes the change. A better name would be "PushSyncAddOrUpdate".

So, a further nice cleanup would be to:
1. Rename bundle->PushSyncChangeIfNeeded to PushSyncAddOrUpdate, and make it
take the SyncData as an argument rather than grab it from the sync service.
2. Combine this->SyncUninstallExtension/SyncExtensionChangeIfNeeded to call a
single function like "SyncExtensionChangeIfNeedImpl" which takes a boolean
whether it's a removal, and uses that to decide whether to call
bundle->PushSyncDeletion or bundle->PushSyncAddOrUpdate. (it would be even
better if these classes could figure out whether it's a deletion but I don't
want to get crazy here).

[Marc Treib, 2015/07/16 08:12:29]

Yup, the other CL does just that :)
It's not (easily) possible here, because PendingEnables also calls
bundle->PushSyncChangeIfNeeded, and it only has an Extension* and can't easily
get the SyncData from it.
Done.
See above: Not yet possible, but the other CL does that.
Without 1. this would be a bit awkward, since PushSyncAddOrUpdate and
PushSyncDeletion still work differently. I'll do the cleanup in the other CL
though :)

 }
 
 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);
 }
 
@@ skipping 368 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;
 }