From 59ef2589e04cebc6d8a3c7d13c6b844b1362714b Mon Sep 17 00:00:00 2001 From: treib Date: Thu, 16 Jul 2015 01:55:01 -0700 Subject: [PATCH] ExtensionSyncService cleanup: process uninstalls in one step Before, the process was split into PrepareToSyncUninstallExtension and ProcessSyncUninstallExtension for no good reason. BUG=None Review URL: https://codereview.chromium.org/1236363002 Cr-Commit-Position: refs/heads/master@{#339003} --- chrome/browser/extensions/extension_service.cc | 17 ++++-------- .../browser/extensions/extension_sync_service.cc | 31 +++++++++------------- chrome/browser/extensions/extension_sync_service.h | 10 +------ chrome/browser/extensions/pending_enables.cc | 2 +- chrome/browser/extensions/sync_bundle.cc | 2 +- chrome/browser/extensions/sync_bundle.h | 2 +- 6 files changed, 21 insertions(+), 43 deletions(-) diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 516985a9db9f..4a1919f96453 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -743,15 +743,6 @@ bool ExtensionService::UninstallExtension( return false; } - syncer::SyncData sync_data; - // Don't sync the uninstall if we're going to reinstall the extension - // momentarily. - if (extension_sync_service_ && - reason != extensions::UNINSTALL_REASON_REINSTALL) { - sync_data = extension_sync_service_->PrepareToSyncUninstallExtension( - *extension); - } - InstallVerifier::Get(GetBrowserContext())->Remove(extension->id()); UMA_HISTOGRAM_ENUMERATION("Extensions.UninstallType", @@ -783,9 +774,11 @@ bool ExtensionService::UninstallExtension( ExtensionRegistry::Get(profile_) ->TriggerOnUninstalled(extension.get(), reason); - if (sync_data.IsValid()) { - extension_sync_service_->ProcessSyncUninstallExtension(extension->id(), - sync_data); + // Don't sync the uninstall if we're going to reinstall the extension + // momentarily. + if (extension_sync_service_ && + reason != extensions::UNINSTALL_REASON_REINSTALL) { + extension_sync_service_->SyncUninstallExtension(*extension); } delayed_installs_.Remove(extension->id()); diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc index 4b55a2748424..33613fe76b5b 100644 --- a/chrome/browser/extensions/extension_sync_service.cc +++ b/chrome/browser/extensions/extension_sync_service.cc @@ -112,33 +112,26 @@ ExtensionSyncService* ExtensionSyncService::Get( 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()); } void ExtensionSyncService::SyncEnableExtension(const Extension& extension) { @@ -166,7 +159,7 @@ void ExtensionSyncService::SyncExtensionChangeIfNeeded( extension.is_app() ? syncer::APPS : syncer::EXTENSIONS; SyncBundle* bundle = GetSyncBundle(type); if (bundle->IsSyncing()) - bundle->PushSyncChangeIfNeeded(extension); + bundle->PushSyncAddOrUpdate(extension); else if (extension_service_->is_ready() && !flare_.is_null()) flare_.Run(type); } diff --git a/chrome/browser/extensions/extension_sync_service.h b/chrome/browser/extensions/extension_sync_service.h index a85b109751e8..88e2c3c0f0dc 100644 --- a/chrome/browser/extensions/extension_sync_service.h +++ b/chrome/browser/extensions/extension_sync_service.h @@ -43,15 +43,7 @@ class ExtensionSyncService : public syncer::SyncableService, // Convenience function to get the ExtensionSyncService for a BrowserContext. static ExtensionSyncService* Get(content::BrowserContext* context); - // Extracts the data needed to sync the uninstall of |extension|, but doesn't - // actually sync anything now. Call |ProcessSyncUninstallExtension| later with - // the returned SyncData to actually commit the change. - syncer::SyncData PrepareToSyncUninstallExtension( - const extensions::Extension& extension); - // Commit a sync uninstall that was previously prepared with - // PrepareToSyncUninstallExtension. - void ProcessSyncUninstallExtension(const std::string& extension_id, - const syncer::SyncData& sync_data); + void SyncUninstallExtension(const extensions::Extension& extension); void SyncEnableExtension(const extensions::Extension& extension); void SyncDisableExtension(const extensions::Extension& extension); diff --git a/chrome/browser/extensions/pending_enables.cc b/chrome/browser/extensions/pending_enables.cc index a1de94506817..b8d1aa1759ab 100644 --- a/chrome/browser/extensions/pending_enables.cc +++ b/chrome/browser/extensions/pending_enables.cc @@ -36,7 +36,7 @@ void PendingEnables::OnSyncStarted(ExtensionService* service) { it != ids_.end(); ++it) { const Extension* extension = service->GetExtensionById(*it, true); if (extension) - sync_bundle_->PushSyncChangeIfNeeded(*extension); + sync_bundle_->PushSyncAddOrUpdate(*extension); } ids_.clear(); } diff --git a/chrome/browser/extensions/sync_bundle.cc b/chrome/browser/extensions/sync_bundle.cc index 4887b0a58183..09afb3ad83a4 100644 --- a/chrome/browser/extensions/sync_bundle.cc +++ b/chrome/browser/extensions/sync_bundle.cc @@ -78,7 +78,7 @@ void SyncBundle::PushSyncDeletion(const std::string& extension_id, sync_data))); } -void SyncBundle::PushSyncChangeIfNeeded(const Extension& extension) { +void SyncBundle::PushSyncAddOrUpdate(const Extension& extension) { syncer::SyncChangeList sync_change_list( 1, CreateSyncChange(extension.id(), diff --git a/chrome/browser/extensions/sync_bundle.h b/chrome/browser/extensions/sync_bundle.h index 8ee58cf567b5..750933f5b916 100644 --- a/chrome/browser/extensions/sync_bundle.h +++ b/chrome/browser/extensions/sync_bundle.h @@ -57,7 +57,7 @@ class SyncBundle { const syncer::SyncData& sync_data); // Pushes any sync changes to |extension| to the server. - void PushSyncChangeIfNeeded(const Extension& extension); + void PushSyncAddOrUpdate(const Extension& extension); // Applies the given SyncChange coming from the server. void ApplySyncChange(const syncer::SyncChange& sync_change); -- 2.11.4.GIT