From c349453205678fda3d6a8a7f1a90a5cec56b8216 Mon Sep 17 00:00:00 2001 From: treib Date: Tue, 21 Jul 2015 07:51:45 -0700 Subject: [PATCH] Extension syncing: Introduce a NeedsSync pref that indicates the extension has local changes that still need to be synced. It's set when something changes before sync is ready, and cleared once the extension state has been synced. This should handle all conflicts between sync and local state reasonably well, and as a bonus allows us to get rid of the (weird and not-really-working) PendingEnables class. BUG=509990 Review URL: https://codereview.chromium.org/1240573012 Cr-Commit-Position: refs/heads/master@{#339651} --- chrome/browser/extensions/extension_service.cc | 26 ++- chrome/browser/extensions/extension_service.h | 8 - .../extensions/extension_service_unittest.cc | 101 +++++------ chrome/browser/extensions/extension_sync_data.cc | 2 +- .../browser/extensions/extension_sync_service.cc | 187 +++++++++++---------- chrome/browser/extensions/extension_sync_service.h | 55 +++--- chrome/browser/extensions/pending_enables.cc | 62 ------- chrome/browser/extensions/pending_enables.h | 66 -------- chrome/browser/extensions/sync_bundle.cc | 68 +++----- chrome/browser/extensions/sync_bundle.h | 40 ++--- chrome/chrome_browser_extensions.gypi | 2 - extensions/browser/extension_prefs.cc | 14 ++ extensions/browser/extension_prefs.h | 6 + 13 files changed, 237 insertions(+), 400 deletions(-) delete mode 100644 chrome/browser/extensions/pending_enables.cc delete mode 100644 chrome/browser/extensions/pending_enables.h diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 4a1919f96453..94c94fd0ea52 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -278,7 +278,6 @@ ExtensionService::ExtensionService(Profile* profile, system_(extensions::ExtensionSystem::Get(profile)), extension_prefs_(extension_prefs), blacklist_(blacklist), - extension_sync_service_(NULL), registry_(extensions::ExtensionRegistry::Get(profile)), pending_extension_manager_(profile), install_directory_(install_directory), @@ -776,9 +775,8 @@ bool ExtensionService::UninstallExtension( // 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); + if (reason != extensions::UNINSTALL_REASON_REINSTALL) { + ExtensionSyncService::Get(profile_)->SyncUninstallExtension(*extension); } delayed_installs_.Remove(extension->id()); @@ -866,8 +864,7 @@ void ExtensionService::EnableExtension(const std::string& extension_id) { content::Source(profile_), content::Details(extension)); - if (extension_sync_service_) - extension_sync_service_->SyncEnableExtension(*extension); + ExtensionSyncService::Get(profile_)->SyncExtensionChangeIfNeeded(*extension); } void ExtensionService::DisableExtension(const std::string& extension_id, @@ -917,8 +914,7 @@ void ExtensionService::DisableExtension(const std::string& extension_id, registry_->RemoveTerminated(extension->id()); } - if (extension_sync_service_) - extension_sync_service_->SyncDisableExtension(*extension); + ExtensionSyncService::Get(profile_)->SyncExtensionChangeIfNeeded(*extension); } void ExtensionService::DisableUserExtensions( @@ -1510,8 +1506,8 @@ void ExtensionService::AddExtension(const Extension* extension) { } else if (!reloading && extension_prefs_->IsExtensionDisabled(extension->id())) { registry_->AddDisabled(extension); - if (extension_sync_service_) - extension_sync_service_->SyncExtensionChangeIfNeeded(*extension); + ExtensionSyncService::Get(profile_)->SyncExtensionChangeIfNeeded( + *extension); content::NotificationService::current()->Notify( extensions::NOTIFICATION_EXTENSION_UPDATE_DISABLED, content::Source(profile_), @@ -1548,8 +1544,8 @@ void ExtensionService::AddExtension(const Extension* extension) { } registry_->AddEnabled(extension); - if (extension_sync_service_) - extension_sync_service_->SyncExtensionChangeIfNeeded(*extension); + ExtensionSyncService::Get(profile_)->SyncExtensionChangeIfNeeded( + *extension); NotifyExtensionLoaded(extension); } system_->runtime_data()->SetBeingUpgraded(extension->id(), false); @@ -2062,8 +2058,10 @@ void ExtensionService::PromoteEphemeralApp( registry_->TriggerOnInstalled(extension, true); - if (!is_from_sync && extension_sync_service_) - extension_sync_service_->SyncExtensionChangeIfNeeded(*extension); + if (!is_from_sync) { + ExtensionSyncService::Get(profile_)->SyncExtensionChangeIfNeeded( + *extension); + } } const Extension* ExtensionService::GetPendingExtensionUpdate( diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h index 01efc6156303..c70e9eeed193 100644 --- a/chrome/browser/extensions/extension_service.h +++ b/chrome/browser/extensions/extension_service.h @@ -405,11 +405,6 @@ class ExtensionService Profile* profile() { return profile_; } - void set_extension_sync_service( - ExtensionSyncService* extension_sync_service) { - extension_sync_service_ = extension_sync_service; - } - // Note that this may return NULL if autoupdate is not turned on. extensions::ExtensionUpdater* updater() { return updater_.get(); } @@ -619,9 +614,6 @@ class ExtensionService // Blacklist for the owning profile. extensions::Blacklist* blacklist_; - // The ExtensionSyncService that is used by this ExtensionService. - ExtensionSyncService* extension_sync_service_; - // Sets of enabled/disabled/terminated/blacklisted extensions. Not owned. extensions::ExtensionRegistry* registry_; diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 3378d72db7de..1577828d182b 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -72,6 +72,7 @@ #include "chrome/common/extensions/api/plugins/plugins_handler.h" #include "chrome/common/extensions/manifest_handlers/app_launch_info.h" #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h" +#include "chrome/common/extensions/sync_helper.h" #include "chrome/common/pref_names.h" #include "chrome/common/url_constants.h" #include "chrome/test/base/scoped_browser_locale.h" @@ -1148,11 +1149,6 @@ class ExtensionServiceTest : public extensions::ExtensionServiceTestBase, #endif } - void InitializeExtensionSyncService() { - extension_sync_service_.reset(new ExtensionSyncService( - profile(), ExtensionPrefs::Get(browser_context()), service())); - } - void InitializeEmptyExtensionServiceWithTestingPrefs() { ExtensionServiceTestBase::ExtensionServiceInitParams params = CreateDefaultInitParams(); @@ -1165,13 +1161,12 @@ class ExtensionServiceTest : public extensions::ExtensionServiceTestBase, } ExtensionSyncService* extension_sync_service() { - return extension_sync_service_.get(); + return ExtensionSyncService::Get(profile()); } protected: typedef extensions::ExtensionManagementPrefUpdater ManagementPrefUpdater; - scoped_ptr extension_sync_service_; extensions::ExtensionList loaded_; std::string unloaded_id_; UnloadedExtensionInfo::Reason unloaded_reason_; @@ -5796,7 +5791,6 @@ TEST_F(ExtensionServiceTest, ComponentExtensions) { TEST_F(ExtensionServiceTest, DeferredSyncStartupPreInstalledComponent) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); bool flare_was_called = false; syncer::ModelType triggered_type(syncer::UNSPECIFIED); @@ -5823,7 +5817,6 @@ TEST_F(ExtensionServiceTest, DeferredSyncStartupPreInstalledComponent) { TEST_F(ExtensionServiceTest, DeferredSyncStartupPreInstalledNormal) { InitializeGoodInstalledExtensionService(); - InitializeExtensionSyncService(); bool flare_was_called = false; syncer::ModelType triggered_type(syncer::UNSPECIFIED); @@ -5846,7 +5839,6 @@ TEST_F(ExtensionServiceTest, DeferredSyncStartupPreInstalledNormal) { TEST_F(ExtensionServiceTest, DeferredSyncStartupOnInstall) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); service()->Init(); ASSERT_TRUE(service()->is_ready()); @@ -5890,7 +5882,6 @@ TEST_F(ExtensionServiceTest, DisableExtensionFromSync) { source_install_dir.DirName().Append(chrome::kPreferencesFilename); InitializeInstalledExtensionService(pref_path, source_install_dir); - InitializeExtensionSyncService(); // The user has enabled sync. ProfileSyncService* sync_service = @@ -5907,30 +5898,34 @@ TEST_F(ExtensionServiceTest, DisableExtensionFromSync) { ASSERT_TRUE(extension); ASSERT_TRUE(service()->IsExtensionEnabled(good0)); + // Sync starts up. + extension_sync_service()->MergeDataAndStartSyncing( + syncer::EXTENSIONS, + syncer::SyncDataList(), + make_scoped_ptr(new syncer::FakeSyncChangeProcessor), + scoped_ptr(new syncer::SyncErrorFactoryMock())); + // Then sync data arrives telling us to disable |good0|. ExtensionSyncData disable_good_crx(*extension, false, Extension::DISABLE_USER_ACTION, false, false, ExtensionSyncData::BOOLEAN_UNSET); - syncer::SyncDataList sync_data; - sync_data.push_back(disable_good_crx.GetSyncData()); - extension_sync_service()->MergeDataAndStartSyncing( - syncer::EXTENSIONS, - sync_data, - scoped_ptr( - new syncer::FakeSyncChangeProcessor), - scoped_ptr(new syncer::SyncErrorFactoryMock())); + syncer::SyncChange sync_change(FROM_HERE, + syncer::SyncChange::ACTION_UPDATE, + disable_good_crx.GetSyncData()); + syncer::SyncChangeList list(1, sync_change); + extension_sync_service()->ProcessSyncChanges(FROM_HERE, list); + ASSERT_FALSE(service()->IsExtensionEnabled(good0)); } -TEST_F(ExtensionServiceTest, DontDisableExtensionWithPendingEnableFromSync) { - // Start the extensions service with one external extension already installed. +TEST_F(ExtensionServiceTest, IgnoreSyncChangesWhenLocalStateIsMoreRecent) { + // Start the extension service with three extensions already installed. base::FilePath source_install_dir = data_dir().AppendASCII("good").AppendASCII("Extensions"); base::FilePath pref_path = source_install_dir.DirName().Append(chrome::kPreferencesFilename); InitializeInstalledExtensionService(pref_path, source_install_dir); - InitializeExtensionSyncService(); // The user has enabled sync. ProfileSyncService* sync_service = @@ -5941,39 +5936,48 @@ TEST_F(ExtensionServiceTest, DontDisableExtensionWithPendingEnableFromSync) { ASSERT_TRUE(service()->is_ready()); ASSERT_EQ(3u, loaded_.size()); - const Extension* extension = service()->GetExtensionById(good0, true); ASSERT_TRUE(service()->IsExtensionEnabled(good0)); + ASSERT_TRUE(service()->IsExtensionEnabled(good2)); - // Disable extension before first sync data arrives. + // Disable and re-enable good0 before first sync data arrives. service()->DisableExtension(good0, Extension::DISABLE_USER_ACTION); ASSERT_FALSE(service()->IsExtensionEnabled(good0)); - - // Enable extension - this is now the most recent state. service()->EnableExtension(good0); ASSERT_TRUE(service()->IsExtensionEnabled(good0)); - - // Now sync data comes in that says to disable good0. This should be - // ignored. - ExtensionSyncData disable_good_crx(*extension, false, - Extension::DISABLE_USER_ACTION, false, - false, ExtensionSyncData::BOOLEAN_UNSET); + // Disable good2 before first sync data arrives (good1 is considered + // non-syncable because it has plugin permission). + service()->DisableExtension(good2, Extension::DISABLE_USER_ACTION); + ASSERT_FALSE(service()->IsExtensionEnabled(good2)); + + const Extension* extension0 = service()->GetExtensionById(good0, true); + const Extension* extension2 = service()->GetExtensionById(good2, true); + ASSERT_TRUE(extensions::sync_helper::IsSyncable(extension0)); + ASSERT_TRUE(extensions::sync_helper::IsSyncable(extension2)); + + // Now sync data comes in that says to disable good0 and enable good2. + ExtensionSyncData disable_good0(*extension0, false, + Extension::DISABLE_USER_ACTION, false, false, + ExtensionSyncData::BOOLEAN_UNSET); + ExtensionSyncData enable_good2(*extension2, true, Extension::DISABLE_NONE, + false, false, + ExtensionSyncData::BOOLEAN_UNSET); syncer::SyncDataList sync_data; - sync_data.push_back(disable_good_crx.GetSyncData()); + sync_data.push_back(disable_good0.GetSyncData()); + sync_data.push_back(enable_good2.GetSyncData()); extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, sync_data, - scoped_ptr( - new syncer::FakeSyncChangeProcessor), - scoped_ptr(new syncer::SyncErrorFactoryMock())); + make_scoped_ptr(new syncer::FakeSyncChangeProcessor), + make_scoped_ptr(new syncer::SyncErrorFactoryMock)); - // The extension was enabled locally before the sync data arrived, so it - // should still be enabled now. - ASSERT_TRUE(service()->IsExtensionEnabled(good0)); + // Both sync changes should be ignored, since the local state was changed + // before sync started, and so the local state is considered more recent. + EXPECT_TRUE(service()->IsExtensionEnabled(good0)); + EXPECT_FALSE(service()->IsExtensionEnabled(good2)); } TEST_F(ExtensionServiceTest, GetSyncData) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW); const Extension* extension = service()->GetInstalledExtension(good_crx); ASSERT_TRUE(extension); @@ -6005,7 +6009,6 @@ TEST_F(ExtensionServiceTest, GetSyncData) { TEST_F(ExtensionServiceTest, GetSyncDataTerminated) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW); TerminateExtension(good_crx); const Extension* extension = service()->GetInstalledExtension(good_crx); @@ -6039,7 +6042,6 @@ TEST_F(ExtensionServiceTest, GetSyncDataTerminated) { TEST_F(ExtensionServiceTest, GetSyncDataFilter) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW); const Extension* extension = service()->GetInstalledExtension(good_crx); ASSERT_TRUE(extension); @@ -6059,7 +6061,6 @@ TEST_F(ExtensionServiceTest, GetSyncDataFilter) { TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW); const Extension* extension = service()->GetInstalledExtension(good_crx); ASSERT_TRUE(extension); @@ -6130,7 +6131,6 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) { TEST_F(ExtensionServiceTest, SyncForUninstalledExternalExtension) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRXWithLocation( data_dir().AppendASCII("good.crx"), Manifest::EXTERNAL_PREF, INSTALL_NEW); const Extension* extension = service()->GetInstalledExtension(good_crx); @@ -6171,7 +6171,6 @@ TEST_F(ExtensionServiceTest, SyncForUninstalledExternalExtension) { TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); const Extension* app = PackAndInstallCRX(data_dir().AppendASCII("app"), INSTALL_NEW); ASSERT_TRUE(app); @@ -6232,7 +6231,6 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) { // PackAndInstallCRX(). When we clean up a bit more, this should move out. TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettingsOnExtensionMoved) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); const size_t kAppCount = 3; const Extension* apps[kAppCount]; apps[0] = PackAndInstallCRX(data_dir().AppendASCII("app1"), INSTALL_NEW); @@ -6283,7 +6281,6 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettingsOnExtensionMoved) { TEST_F(ExtensionServiceTest, GetSyncDataList) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW); InstallCRX(data_dir().AppendASCII("page_action.crx"), INSTALL_NEW); InstallCRX(data_dir().AppendASCII("theme.crx"), INSTALL_NEW); @@ -6313,7 +6310,6 @@ TEST_F(ExtensionServiceTest, GetSyncDataList) { TEST_F(ExtensionServiceTest, ProcessSyncDataUninstall) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6354,7 +6350,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataUninstall) { TEST_F(ExtensionServiceTest, ProcessSyncDataWrongType) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); // Install the extension. base::FilePath extension_path = data_dir().AppendASCII("good.crx"); @@ -6402,7 +6397,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataWrongType) { TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6515,7 +6509,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) { TEST_F(ExtensionServiceTest, ProcessSyncDataNewExtension) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6596,7 +6589,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNewExtension) { TEST_F(ExtensionServiceTest, ProcessSyncDataTerminatedExtension) { InitializeExtensionServiceWithUpdater(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6634,7 +6626,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataTerminatedExtension) { TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { InitializeExtensionServiceWithUpdater(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6704,7 +6695,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) { InitializeExtensionServiceWithUpdater(); - InitializeExtensionSyncService(); syncer::FakeSyncChangeProcessor processor; extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, @@ -6746,7 +6736,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) { TEST_F(ExtensionServiceTest, ProcessSyncDataEnableDisable) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, syncer::SyncDataList(), @@ -6844,7 +6833,6 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataPermissionApproval) { "http://localhost/autoupdate/updates.xml"); InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, syncer::SyncDataList(), @@ -7132,7 +7120,6 @@ TEST_F(ExtensionServiceTest, SupervisedUser_UpdateWithPermissionIncrease) { TEST_F(ExtensionServiceTest, SupervisedUser_SyncUninstallByCustodianSkipsPolicy) { InitializeEmptyExtensionService(); - InitializeExtensionSyncService(); extension_sync_service()->MergeDataAndStartSyncing( syncer::EXTENSIONS, syncer::SyncDataList(), diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc index bacf39c721a4..6404e8b61ca6 100644 --- a/chrome/browser/extensions/extension_sync_data.cc +++ b/chrome/browser/extensions/extension_sync_data.cc @@ -202,7 +202,7 @@ void ExtensionSyncData::ToAppSpecifics(sync_pb::AppSpecifics* specifics) const { static_cast(launch_type_); // The corresponding validation of this value during processing of an - // AppSyncData is in ExtensionSyncService::ProcessAppSyncData. + // ExtensionSyncData is in ExtensionSyncService::ApplySyncData. if (launch_type_ >= LAUNCH_TYPE_FIRST && launch_type_ < NUM_LAUNCH_TYPES && sync_pb::AppSpecifics_LaunchType_IsValid(sync_launch_type)) { specifics->set_launch_type(sync_launch_type); diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc index 33613fe76b5b..5272d5448f0b 100644 --- a/chrome/browser/extensions/extension_sync_service.cc +++ b/chrome/browser/extensions/extension_sync_service.cc @@ -35,7 +35,6 @@ using extensions::ExtensionPrefs; using extensions::ExtensionRegistry; using extensions::ExtensionSet; using extensions::ExtensionSyncData; -using extensions::PendingEnables; using extensions::SyncBundle; namespace { @@ -79,6 +78,15 @@ bool IsCorrectSyncType(const Extension& extension, syncer::ModelType type) { (type == syncer::APPS && extension.is_app()); } +syncer::SyncDataList ToSyncerSyncDataList( + const std::vector& 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, @@ -86,21 +94,10 @@ ExtensionSyncService::ExtensionSyncService(Profile* profile, 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); } @@ -124,30 +121,12 @@ void ExtensionSyncService::SyncUninstallExtension( 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( @@ -158,10 +137,15 @@ void ExtensionSyncService::SyncExtensionChangeIfNeeded( 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( @@ -174,17 +158,31 @@ syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing( << "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_); + bundle->StartSyncing(sync_processor.Pass()); + + // 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 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 data_list = GetLocalSyncDataList(type, true); + 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); @@ -196,20 +194,31 @@ void ExtensionSyncService::StopSyncing(syncer::ModelType type) { syncer::SyncDataList ExtensionSyncService::GetAllSyncData( syncer::ModelType type) const { - std::vector 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 sync_data_list = + GetLocalSyncDataList(type, true); + + // Add pending data (where the local extension is either not installed yet or + // outdated). + std::vector 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 extension_sync_data( + ExtensionSyncData::CreateFromSyncChange(sync_change)); + if (extension_sync_data) + ApplySyncData(*extension_sync_data); } extension_prefs_->app_sorting()->FixNTPOrdinalCollisions(); @@ -243,6 +252,11 @@ ExtensionSyncData ExtensionSyncService::CreateSyncData( 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()) { @@ -256,8 +270,8 @@ bool ExtensionSyncService::ApplySyncData( 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( @@ -268,11 +282,8 @@ bool ExtensionSyncService::ApplySyncData( 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; } @@ -345,12 +356,6 @@ void ExtensionSyncService::SetSyncStartFlare( 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( const_cast(*this).GetSyncBundle(type)); @@ -361,38 +366,40 @@ const SyncBundle* ExtensionSyncService::GetSyncBundle( 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 ExtensionSyncService::GetSyncDataList( - syncer::ModelType type) const { +std::vector 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 extension_sync_list; - FillSyncDataList(registry->enabled_extensions(), type, &extension_sync_list); - FillSyncDataList(registry->disabled_extensions(), type, &extension_sync_list); + std::vector 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 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* sync_data_list) const { const SyncBundle* bundle = GetSyncBundle(type); for (const scoped_refptr& 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)); } } @@ -448,7 +455,7 @@ bool ExtensionSyncService::ApplyExtensionSyncDataHelper( 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)) { diff --git a/chrome/browser/extensions/extension_sync_service.h b/chrome/browser/extensions/extension_sync_service.h index 88e2c3c0f0dc..773a51657837 100644 --- a/chrome/browser/extensions/extension_sync_service.h +++ b/chrome/browser/extensions/extension_sync_service.h @@ -8,13 +8,13 @@ #include #include -#include "chrome/browser/extensions/pending_enables.h" #include "chrome/browser/extensions/sync_bundle.h" #include "components/keyed_service/core/keyed_service.h" #include "extensions/browser/extension_prefs.h" #include "extensions/common/extension.h" #include "sync/api/syncable_service.h" +class ExtensionService; class Profile; namespace extensions { @@ -43,11 +43,9 @@ class ExtensionSyncService : public syncer::SyncableService, // Convenience function to get the ExtensionSyncService for a BrowserContext. static ExtensionSyncService* Get(content::BrowserContext* context); + // Notifies Sync that the given |extension| has been uninstalled. void SyncUninstallExtension(const extensions::Extension& extension); - void SyncEnableExtension(const extensions::Extension& extension); - void SyncDisableExtension(const extensions::Extension& extension); - void SyncOrderingChange(const std::string& extension_id); // Notifies Sync (if needed) of a newly-installed extension or a change to @@ -66,44 +64,47 @@ class ExtensionSyncService : public syncer::SyncableService, const tracked_objects::Location& from_here, const syncer::SyncChangeList& change_list) override; - // Creates the ExtensionSyncData for the given app/extension. - extensions::ExtensionSyncData CreateSyncData( - const extensions::Extension& extension) const; - - // Applies the change specified passed in by either ExtensionSyncData to the - // current system. - // Returns false if the changes were not completely applied and were added - // to the pending list to be tried again. - bool ApplySyncData(const extensions::ExtensionSyncData& extension_sync_data); - // |flare| provides a StartSyncFlare to the SyncableService. See // sync_start_util for more. Public for testing. void SetSyncStartFlare(const syncer::SyncableService::StartSyncFlare& flare); private: - // Whether the given extension has been enabled before sync has started. - bool IsPendingEnable(const std::string& extension_id) const; + FRIEND_TEST_ALL_PREFIXES(TwoClientAppsSyncTest, UnexpectedLaunchType); + FRIEND_TEST_ALL_PREFIXES(ExtensionDisabledGlobalErrorTest, + HigherPermissionsFromSync); + FRIEND_TEST_ALL_PREFIXES(ExtensionDisabledGlobalErrorTest, RemoteInstall); + friend class EphemeralAppBrowserTest; // Gets the SyncBundle for the given |type|. extensions::SyncBundle* GetSyncBundle(syncer::ModelType type); const extensions::SyncBundle* GetSyncBundle(syncer::ModelType type) const; - // Gets the PendingEnables for apps/extensions. - extensions::PendingEnables* GetPendingEnables(bool for_apps); + // Creates the ExtensionSyncData for the given app/extension. + extensions::ExtensionSyncData CreateSyncData( + const extensions::Extension& extension) const; - // Gets the ExtensionSyncData for all apps or extensions. - std::vector GetSyncDataList( - syncer::ModelType type) const; + // Applies the given change coming in from the server to the local state. + // Returns false if the changes were not completely applied and were added + // to the pending list. + bool ApplySyncData(const extensions::ExtensionSyncData& extension_sync_data); + + // Collects the ExtensionSyncData for all installed apps or extensions. + // If |include_everything| is true, includes all installed extensions, + // otherwise only those that have the NeedsSync pref set, i.e. which have + // local changes that need to be pushed. + std::vector GetLocalSyncDataList( + syncer::ModelType type, bool include_everything) const; + // Helper for GetLocalSyncDataList. void FillSyncDataList( const extensions::ExtensionSet& extensions, syncer::ModelType type, + bool include_everything, std::vector* sync_data_list) const; // Handles applying the extension specific values in |extension_sync_data| to - // the current system. - // Returns false if the changes were not completely applied and need to be - // tried again later. + // the local state. + // Returns false if the changes were not completely applied. bool ApplyExtensionSyncDataHelper( const extensions::ExtensionSyncData& extension_sync_data, syncer::ModelType type); @@ -123,12 +124,6 @@ class ExtensionSyncService : public syncer::SyncableService, extensions::SyncBundle app_sync_bundle_; extensions::SyncBundle extension_sync_bundle_; - // Set of extensions/apps that have been enabled before sync has started. - // TODO(treib,kalman): This seems wrong. Why are enables special, as opposed - // to disables, or any other changes? - extensions::PendingEnables pending_app_enables_; - extensions::PendingEnables pending_extension_enables_; - // Run()ning tells sync to try and start soon, because syncable changes // have started happening. It will cause sync to call us back // asynchronously via MergeDataAndStartSyncing as soon as possible. diff --git a/chrome/browser/extensions/pending_enables.cc b/chrome/browser/extensions/pending_enables.cc deleted file mode 100644 index b8d1aa1759ab..000000000000 --- a/chrome/browser/extensions/pending_enables.cc +++ /dev/null @@ -1,62 +0,0 @@ -// 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/pending_enables.h" - -#include "chrome/browser/extensions/extension_service.h" -#include "chrome/browser/extensions/sync_bundle.h" -#include "components/sync_driver/sync_prefs.h" - -namespace extensions { - -PendingEnables::PendingEnables(scoped_ptr sync_prefs, - SyncBundle* sync_bundle, - syncer::ModelType enable_type) - : sync_prefs_(sync_prefs.Pass()), - sync_bundle_(sync_bundle), - enable_type_(enable_type), - is_sync_enabled_for_test_(false) {} - -PendingEnables::~PendingEnables() { -} - -void PendingEnables::Add(const std::string& extension_id) { - if (IsWaitingForSync()) - ids_.insert(extension_id); -} - -void PendingEnables::Remove(const std::string& extension_id) { - if (IsWaitingForSync()) - ids_.erase(extension_id); -} - -void PendingEnables::OnSyncStarted(ExtensionService* service) { - for (std::set::const_iterator it = ids_.begin(); - it != ids_.end(); ++it) { - const Extension* extension = service->GetExtensionById(*it, true); - if (extension) - sync_bundle_->PushSyncAddOrUpdate(*extension); - } - ids_.clear(); -} - -bool PendingEnables::Contains(const std::string& extension_id) const { - return ids_.find(extension_id) != ids_.end(); -} - -bool PendingEnables::IsSyncEnabled() { - if (is_sync_enabled_for_test_) - return true; - return sync_prefs_ && - sync_prefs_->HasSyncSetupCompleted() && - sync_prefs_->GetPreferredDataTypes(syncer::ModelTypeSet(enable_type_)) - .Has(enable_type_); -} - -bool PendingEnables::IsWaitingForSync() { - return IsSyncEnabled() && !sync_bundle_->IsSyncing(); -} - -} // namespace extensions - diff --git a/chrome/browser/extensions/pending_enables.h b/chrome/browser/extensions/pending_enables.h deleted file mode 100644 index c8e59b87f57d..000000000000 --- a/chrome/browser/extensions/pending_enables.h +++ /dev/null @@ -1,66 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_PENDING_ENABLES_H_ -#define CHROME_BROWSER_EXTENSIONS_PENDING_ENABLES_H_ - -#include - -// Included for syncer::ModelType, which is in -// sync/internal_api/public/base/model_type.h but that is disallowed by -// chrome/browser/DEPS. -#include "sync/api/syncable_service.h" - -class ExtensionService; -class ProfileSyncService; - -namespace sync_driver { -class SyncPrefs; -} - -namespace extensions { - -class SyncBundle; - -// A pending enable is when an app or extension is enabled locally before sync -// has started. We track these to prevent sync data arriving and clobbering -// local state, and also to ensure that these early enables get synced to the -// server when sync does start. -class PendingEnables { - public: - PendingEnables(scoped_ptr sync_prefs, - SyncBundle* sync_bundle, - syncer::ModelType enable_type); - ~PendingEnables(); - - // Called when an extension is enabled / disabled locally. - // These will check the sync state and figure out whether the change - // needs to be remembered for syncing when syncing starts. - void Add(const std::string& extension_id); - void Remove(const std::string& extension_id); - - // Called when |sync_bundle_| is ready to accept sync changes. - // Uses |service| to look up extensions from extension ids. - void OnSyncStarted(ExtensionService* service); - - // Whether |extension_id| has a pending enable. - bool Contains(const std::string& extension_id) const; - - private: - bool IsSyncEnabled(); - bool IsWaitingForSync(); - - scoped_ptr sync_prefs_; - SyncBundle* sync_bundle_; - syncer::ModelType enable_type_; - std::set ids_; - - bool is_sync_enabled_for_test_; - - DISALLOW_COPY_AND_ASSIGN(PendingEnables); -}; - -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_PENDING_ENABLES_H_ diff --git a/chrome/browser/extensions/sync_bundle.cc b/chrome/browser/extensions/sync_bundle.cc index 09afb3ad83a4..4e6a7756f1d7 100644 --- a/chrome/browser/extensions/sync_bundle.cc +++ b/chrome/browser/extensions/sync_bundle.cc @@ -13,24 +13,12 @@ namespace extensions { -SyncBundle::SyncBundle(ExtensionSyncService* sync_service) - : sync_service_(sync_service) {} - +SyncBundle::SyncBundle() {} SyncBundle::~SyncBundle() {} -void SyncBundle::MergeDataAndStartSyncing( - const syncer::SyncDataList& initial_sync_data, +void SyncBundle::StartSyncing( scoped_ptr sync_processor) { sync_processor_.reset(sync_processor.release()); - - for (const syncer::SyncData& sync_data : initial_sync_data) { - scoped_ptr extension_sync_data( - ExtensionSyncData::CreateFromSyncData(sync_data)); - if (extension_sync_data.get()) { - AddSyncedExtension(extension_sync_data->id()); - sync_service_->ApplySyncData(*extension_sync_data); - } - } } void SyncBundle::Reset() { @@ -43,17 +31,6 @@ 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; @@ -71,6 +48,9 @@ void SyncBundle::PushSyncDataList( 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, @@ -78,26 +58,21 @@ void SyncBundle::PushSyncDeletion(const std::string& extension_id, 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) { + PushSyncChanges(syncer::SyncChangeList( + 1, CreateSyncChange(extension_id, sync_data))); + AddSyncedExtension(extension_id); + // 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 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()); +void SyncBundle::ApplySyncData(const ExtensionSyncData& extension_sync_data) { + if (extension_sync_data.uninstalled()) + RemoveSyncedExtension(extension_sync_data.id()); else - AddSyncedExtension(extension_sync_data->id()); - sync_service_->ApplySyncData(*extension_sync_data); + AddSyncedExtension(extension_sync_data.id()); } bool SyncBundle::HasPendingExtensionId(const std::string& id) const { @@ -123,8 +98,8 @@ syncer::SyncChange SyncBundle::CreateSyncChange( 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); } @@ -141,9 +116,8 @@ 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 diff --git a/chrome/browser/extensions/sync_bundle.h b/chrome/browser/extensions/sync_bundle.h index 750933f5b916..adebbb10754c 100644 --- a/chrome/browser/extensions/sync_bundle.h +++ b/chrome/browser/extensions/sync_bundle.h @@ -17,34 +17,23 @@ class ExtensionSyncService; namespace extensions { -class Extension; class ExtensionSyncData; class SyncBundle { public: - explicit SyncBundle(ExtensionSyncService* sync_service); + SyncBundle(); ~SyncBundle(); - void MergeDataAndStartSyncing( - const syncer::SyncDataList& initial_sync_data, - scoped_ptr sync_processor); + void StartSyncing(scoped_ptr sync_processor); // Resets this class back to its default values, which will disable all // syncing until StartSyncing is called again. void Reset(); // Has this bundle started syncing yet? - // Returns true if MergeDataAndStartSyncing has been called, false otherwise. + // Returns true if StartSyncing has been called, false otherwise. bool IsSyncing() const; - // Checks if the extension with the given |id| is synced. - bool HasExtensionId(const std::string& id) const; - - // Whether the given extension should be included in the SyncDataList to be - // sent to the server. Returns false if there is pending data that should be - // used instead. - bool ShouldIncludeInLocalSyncDataList(const Extension& extension) const; - // Handles the given list of local SyncDatas. This updates the set of synced // extensions as appropriate, and then pushes the corresponding SyncChanges // to the server. @@ -57,12 +46,15 @@ class SyncBundle { const syncer::SyncData& sync_data); // Pushes any sync changes to |extension| to the server. - void PushSyncAddOrUpdate(const Extension& extension); + void PushSyncAddOrUpdate(const std::string& extension_id, + const syncer::SyncData& sync_data); - // Applies the given SyncChange coming from the server. - void ApplySyncChange(const syncer::SyncChange& sync_change); + // Applies the given sync change coming in from the server. This just updates + // the list of synced extensions. + void ApplySyncData(const ExtensionSyncData& extension_sync_data); - // Checks if the extension with the given |id| is pending to be synced. + // Checks if there is pending sync data for the extension with the given |id| + // that should be sent to the server instead of the local state. bool HasPendingExtensionId(const std::string& id) const; // Adds a pending extension to be synced. @@ -82,16 +74,18 @@ class SyncBundle { void AddSyncedExtension(const std::string& id); void RemoveSyncedExtension(const std::string& id); - - // Changes an extension from being pending to synced. - void MarkPendingExtensionSynced(const std::string& id); - - ExtensionSyncService* sync_service_; // Owns us. + bool HasSyncedExtension(const std::string& id) const; scoped_ptr sync_processor_; + // Stores the set of extensions we know about. Used to decide if a sync change + // should be ACTION_ADD or ACTION_UPDATE. std::set synced_extensions_; + // This stores changes we got from sync that we couldn't apply immediately + // (such as installing a new extension, or an update). We'll send this back + // to the server instead of the local state, to prevent the sync state from + // flipping back and forth until all clients are on the same state. std::map pending_sync_data_; DISALLOW_COPY_AND_ASSIGN(SyncBundle); diff --git a/chrome/chrome_browser_extensions.gypi b/chrome/chrome_browser_extensions.gypi index af291aaa83b8..9602ac5f8b90 100644 --- a/chrome/chrome_browser_extensions.gypi +++ b/chrome/chrome_browser_extensions.gypi @@ -779,8 +779,6 @@ 'browser/extensions/pack_extension_job.h', 'browser/extensions/path_util.cc', 'browser/extensions/path_util.h', - 'browser/extensions/pending_enables.cc', - 'browser/extensions/pending_enables.h', 'browser/extensions/pending_extension_info.cc', 'browser/extensions/pending_extension_info.h', 'browser/extensions/pending_extension_manager.cc', diff --git a/extensions/browser/extension_prefs.cc b/extensions/browser/extension_prefs.cc index 79bd54725553..58ccad71e6c0 100644 --- a/extensions/browser/extension_prefs.cc +++ b/extensions/browser/extension_prefs.cc @@ -192,6 +192,10 @@ const char kPrefDoNotSync[] = "do_not_sync"; const char kCorruptedDisableCount[] = "extensions.corrupted_disable_count"; +// A boolean preference that indicates whether the extension has local changes +// that need to be synced. Default value is false. +const char kPrefNeedsSync[] = "needs_sync"; + // Provider of write access to a dictionary storing extension prefs. class ScopedExtensionPrefUpdate : public DictionaryPrefUpdate { public: @@ -1852,6 +1856,16 @@ void ExtensionPrefs::IncrementCorruptedDisableCount() { prefs_->SetInteger(kCorruptedDisableCount, count + 1); } +bool ExtensionPrefs::NeedsSync(const std::string& extension_id) const { + return ReadPrefAsBooleanAndReturn(extension_id, kPrefNeedsSync); +} + +void ExtensionPrefs::SetNeedsSync(const std::string& extension_id, + bool needs_sync) { + UpdateExtensionPref(extension_id, kPrefNeedsSync, + needs_sync ? new base::FundamentalValue(true) : nullptr); +} + ExtensionPrefs::ExtensionPrefs( PrefService* prefs, const base::FilePath& root_dir, diff --git a/extensions/browser/extension_prefs.h b/extensions/browser/extension_prefs.h index 80507895954f..026a4c65464d 100644 --- a/extensions/browser/extension_prefs.h +++ b/extensions/browser/extension_prefs.h @@ -531,6 +531,12 @@ class ExtensionPrefs : public ExtensionScopedPrefs, public KeyedService { int GetCorruptedDisableCount() const; void IncrementCorruptedDisableCount(); + // Whether the extension with the given |id| needs to be synced. This is set + // when the state (such as enabled/disabled or allowed in incognito) is + // changed before Sync is ready. + bool NeedsSync(const std::string& extension_id) const; + void SetNeedsSync(const std::string& extension_id, bool needs_sync); + private: friend class ExtensionPrefsBlacklistedExtensions; // Unit test. friend class ExtensionPrefsComponentExtension; // Unit test. -- 2.11.4.GIT