diff options
author | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-23 21:59:00 +0000 |
---|---|---|
committer | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-23 21:59:00 +0000 |
commit | 0677c6951f4458c6b56484e9a0fc074466761dc8 (patch) | |
tree | 89e9df8281fb6a40d70f61a2abffd558bfa36ea9 /chrome/browser/extensions | |
parent | ef31c68d5fa6fdd04ff9a794a8064e566038a160 (diff) | |
download | chromium_src-0677c6951f4458c6b56484e9a0fc074466761dc8.zip chromium_src-0677c6951f4458c6b56484e9a0fc074466761dc8.tar.gz chromium_src-0677c6951f4458c6b56484e9a0fc074466761dc8.tar.bz2 |
[Sync] Have SyncableService's take ownership of their SyncChangeProcessor.
The UIDataTypeController now uses a SharedChangeProcessor, and passes a ref
to the SyncableService's. Every SyncableService now owns its own change
processor. Additionally, we use the ScopedPtr::Pass semantics for passing
around SyncChangeProcessors to ensure SyncableServices properly take
ownership.
This, along with all the test updates it requires (most of the patch) fixes
several leaks introduced in the previous patch to remove the Syncable Service
Adapter. SyncableServiceMock is removed as it didn't play nice with
scoped_ptr parameters (and was hardly used).
BUG=117098,117538
TEST=unit_tests with valgrind/drmemory/heapcheck
TBR=georgy@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9749012
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128578 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/extensions')
13 files changed, 335 insertions, 204 deletions
diff --git a/chrome/browser/extensions/app_notification_manager.cc b/chrome/browser/extensions/app_notification_manager.cc index 5a254f3..c9aca6b 100644 --- a/chrome/browser/extensions/app_notification_manager.cc +++ b/chrome/browser/extensions/app_notification_manager.cc @@ -66,7 +66,6 @@ const unsigned int AppNotificationManager::kMaxNotificationPerApp = 5; AppNotificationManager::AppNotificationManager(Profile* profile) : profile_(profile), - sync_processor_(NULL), models_associated_(false), processing_syncer_changes_(false) { registrar_.Add(this, @@ -383,15 +382,16 @@ SyncError AppNotificationManager::ProcessSyncChanges( SyncError AppNotificationManager::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // AppNotificationDataTypeController ensures that modei is fully should before // this method is called by waiting until the load notification is received // from AppNotificationManager. DCHECK(loaded()); DCHECK_EQ(type, syncable::APP_NOTIFICATIONS); - DCHECK(!sync_processor_); - sync_processor_ = sync_processor; + DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); + sync_processor_ = sync_processor.Pass(); // We may add, or remove notifications here, so ensure we don't step on // our own toes. @@ -443,7 +443,7 @@ SyncError AppNotificationManager::MergeDataAndStartSyncing( void AppNotificationManager::StopSyncing(syncable::ModelType type) { DCHECK_EQ(type, syncable::APP_NOTIFICATIONS); models_associated_ = false; - sync_processor_ = NULL; + sync_processor_.reset(); } void AppNotificationManager::SyncAddChange(const AppNotification& notif) { diff --git a/chrome/browser/extensions/app_notification_manager.h b/chrome/browser/extensions/app_notification_manager.h index 166298b..fae3b9e 100644 --- a/chrome/browser/extensions/app_notification_manager.h +++ b/chrome/browser/extensions/app_notification_manager.h @@ -73,7 +73,7 @@ class AppNotificationManager virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) OVERRIDE; + scoped_ptr<SyncChangeProcessor> sync_processor) OVERRIDE; virtual void StopSyncing(syncable::ModelType type) OVERRIDE; private: @@ -155,7 +155,7 @@ class AppNotificationManager scoped_ptr<AppNotificationStorage> storage_; // Sync change processor we use to push all our changes. - SyncChangeProcessor* sync_processor_; + scoped_ptr<SyncChangeProcessor> sync_processor_; // Whether the sync model is associated with the local model. // In other words, whether we are ready to apply sync changes. bool models_associated_; diff --git a/chrome/browser/extensions/app_notification_manager_sync_unittest.cc b/chrome/browser/extensions/app_notification_manager_sync_unittest.cc index bf82234..7542b54 100644 --- a/chrome/browser/extensions/app_notification_manager_sync_unittest.cc +++ b/chrome/browser/extensions/app_notification_manager_sync_unittest.cc @@ -61,13 +61,38 @@ class TestChangeProcessor : public SyncChangeProcessor { DISALLOW_COPY_AND_ASSIGN(TestChangeProcessor); }; +class SyncChangeProcessorDelegate : public SyncChangeProcessor { + public: + explicit SyncChangeProcessorDelegate(SyncChangeProcessor* recipient) + : recipient_(recipient) { + DCHECK(recipient_); + } + virtual ~SyncChangeProcessorDelegate() {} + + // SyncChangeProcessor implementation. + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE { + return recipient_->ProcessSyncChanges(from_here, change_list); + } + + private: + // The recipient of all sync changes. + SyncChangeProcessor* recipient_; + + DISALLOW_COPY_AND_ASSIGN(SyncChangeProcessorDelegate); +}; + } // namespace class AppNotificationManagerSyncTest : public testing::Test { public: AppNotificationManagerSyncTest() : ui_thread_(BrowserThread::UI, &ui_loop_), - file_thread_(BrowserThread::FILE) { } + file_thread_(BrowserThread::FILE), + sync_processor_(new TestChangeProcessor), + sync_processor_delegate_(new SyncChangeProcessorDelegate( + sync_processor_.get())) {} ~AppNotificationManagerSyncTest() { model_ = NULL; @@ -104,7 +129,11 @@ class AppNotificationManagerSyncTest : public testing::Test { } AppNotificationManager* model() { return model_.get(); } - TestChangeProcessor* processor() { return &processor_; } + TestChangeProcessor* processor() { return sync_processor_.get(); } + + scoped_ptr<SyncChangeProcessor> PassProcessor() { + return sync_processor_delegate_.PassAs<SyncChangeProcessor>(); + } // Creates a notification whose properties are set from the given integer. static AppNotification* CreateNotification(int suffix) { @@ -194,7 +223,8 @@ class AppNotificationManagerSyncTest : public testing::Test { scoped_ptr<TestingProfile> profile_; scoped_refptr<AppNotificationManager> model_; - TestChangeProcessor processor_; + scoped_ptr<TestChangeProcessor> sync_processor_; + scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_; DISALLOW_COPY_AND_ASSIGN(AppNotificationManagerSyncTest); }; @@ -273,7 +303,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothEmpty) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), // Empty. - processor()); + PassProcessor()); EXPECT_EQ(0U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size()); EXPECT_EQ(0, processor()->change_list_size()); @@ -290,7 +320,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocModelEmpty) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(4U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size()); // Model should all of the initial sync data. @@ -327,7 +357,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyNoOverlap) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(6U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size()); for (SyncDataList::const_iterator iter = initial_data.begin(); @@ -378,7 +408,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptySomeOverlap) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(6U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size()); for (SyncDataList::const_iterator iter = initial_data.begin(); @@ -426,7 +456,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyTitleMismatch) { SyncError sync_error = model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); EXPECT_TRUE(sync_error.IsSet()); EXPECT_EQ(syncable::APP_NOTIFICATIONS, sync_error.type()); @@ -450,7 +480,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyMatchesLocal) { SyncError sync_error = model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); EXPECT_TRUE(sync_error.IsSet()); EXPECT_EQ(syncable::APP_NOTIFICATIONS, sync_error.type()); @@ -463,7 +493,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesEmptyModel) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); // Set up a bunch of ADDs. SyncChangeList changes; @@ -489,7 +519,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesNonEmptyModel) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); // Some adds and some deletes. SyncChangeList changes; @@ -514,7 +544,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadAdd) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); // Some adds and some deletes. SyncChangeList changes; @@ -537,7 +567,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadDelete) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); // Some adds and some deletes. SyncChangeList changes; @@ -560,7 +590,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadUpdates) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); // Some adds and some deletes. SyncChangeList changes; @@ -584,7 +614,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesEmptyModelWithMax) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); for (unsigned int i = 0; i < AppNotificationManager::kMaxNotificationPerApp * 2; i++) { SyncChangeList changes; @@ -619,19 +649,19 @@ TEST_F(AppNotificationManagerSyncTest, // Stop syncing sets state correctly. TEST_F(AppNotificationManagerSyncTest, StopSyncing) { - EXPECT_FALSE(model()->sync_processor_); + EXPECT_FALSE(model()->sync_processor_.get()); EXPECT_FALSE(model()->models_associated_); model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); - EXPECT_TRUE(model()->sync_processor_); + EXPECT_TRUE(model()->sync_processor_.get()); EXPECT_TRUE(model()->models_associated_); model()->StopSyncing(syncable::APP_NOTIFICATIONS); - EXPECT_FALSE(model()->sync_processor_); + EXPECT_FALSE(model()->sync_processor_.get()); EXPECT_FALSE(model()->models_associated_); } @@ -640,7 +670,7 @@ TEST_F(AppNotificationManagerSyncTest, AddsGetsSynced) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, SyncDataList(), - processor()); + PassProcessor()); AppNotification* n1 = CreateNotification(1); model()->Add(n1); @@ -677,7 +707,7 @@ TEST_F(AppNotificationManagerSyncTest, ClearAllGetsSynced) { model()->MergeDataAndStartSyncing( syncable::APP_NOTIFICATIONS, initial_data, - processor()); + PassProcessor()); model()->ClearAll(ext_id); diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 968b3fc..070e378 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -1266,13 +1266,19 @@ namespace { } // namespace ExtensionService::SyncBundle::SyncBundle() - : filter(IsSyncableNone), - sync_processor(NULL) { + : filter(IsSyncableNone) { } ExtensionService::SyncBundle::~SyncBundle() { } +void ExtensionService::SyncBundle::Reset() { + filter = IsSyncableNone; + synced_extensions.clear(); + pending_sync_data.clear(); + sync_processor.reset(); +} + bool ExtensionService::SyncBundle::HasExtensionId(const std::string& id) const { return synced_extensions.find(id) != synced_extensions.end(); } @@ -1335,9 +1341,7 @@ ExtensionService::SyncBundle* ExtensionService::GetSyncBundleForModelType( SyncError ExtensionService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { - CHECK(sync_processor); - + scoped_ptr<SyncChangeProcessor> sync_processor) { SyncBundle* bundle = NULL; switch (type) { @@ -1354,8 +1358,9 @@ SyncError ExtensionService::MergeDataAndStartSyncing( default: LOG(FATAL) << "Got " << type << " ModelType"; } - - bundle->sync_processor = sync_processor; + DCHECK(!bundle->sync_processor.get()); + DCHECK(sync_processor.get()); + bundle->sync_processor = sync_processor.Pass(); // Process extensions from sync. for (SyncDataList::const_iterator i = initial_sync_data.begin(); @@ -1391,8 +1396,7 @@ SyncError ExtensionService::MergeDataAndStartSyncing( void ExtensionService::StopSyncing(syncable::ModelType type) { SyncBundle* bundle = GetSyncBundleForModelType(type); CHECK(bundle); - // This is the simplest way to clear out the bundle. - *bundle = SyncBundle(); + bundle->Reset(); } SyncDataList ExtensionService::GetAllSyncData(syncable::ModelType type) const { diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h index bbf433b..c459b34 100644 --- a/chrome/browser/extensions/extension_service.h +++ b/chrome/browser/extensions/extension_service.h @@ -419,7 +419,7 @@ class ExtensionService virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) OVERRIDE; + scoped_ptr<SyncChangeProcessor> sync_processor) OVERRIDE; virtual void StopSyncing(syncable::ModelType type) OVERRIDE; virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE; virtual SyncError ProcessSyncChanges( @@ -591,13 +591,16 @@ class ExtensionService SyncBundle(); ~SyncBundle(); + void Reset(); + bool HasExtensionId(const std::string& id) const; bool HasPendingExtensionId(const std::string& id) const; + // Note: all members of the struct need to be explicitly cleared in Reset(). ExtensionFilter filter; std::set<std::string> synced_extensions; std::map<std::string, ExtensionSyncData> pending_sync_data; - SyncChangeProcessor* sync_processor; + scoped_ptr<SyncChangeProcessor> sync_processor; }; // Contains Extension data that can change during the life of the process, diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index fe23bae..f871a0e 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -3999,9 +3999,8 @@ TEST_F(ExtensionServiceTest, GetSyncData) { const Extension* extension = service_->GetInstalledExtension(good_crx); ASSERT_TRUE(extension); - TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); SyncDataList list = service_->GetAllSyncData(syncable::EXTENSIONS); ASSERT_EQ(list.size(), 1U); @@ -4024,7 +4023,7 @@ TEST_F(ExtensionServiceTest, GetSyncDataTerminated) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); SyncDataList list = service_->GetAllSyncData(syncable::EXTENSIONS); ASSERT_EQ(list.size(), 1U); @@ -4046,7 +4045,7 @@ TEST_F(ExtensionServiceTest, GetSyncDataFilter) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::APPS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); SyncDataList list = service_->GetAllSyncData(syncable::EXTENSIONS); ASSERT_EQ(list.size(), 0U); @@ -4060,7 +4059,7 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); { SyncDataList list = service_->GetAllSyncData(syncable::EXTENSIONS); @@ -4107,7 +4106,7 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::APPS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); StringOrdinal initial_ordinal = StringOrdinal::CreateInitialOrdinal(); { @@ -4152,7 +4151,7 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettingsOnExtensionMoved) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::APPS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); service_->OnExtensionMoved(apps[0]->id(), apps[1]->id(), apps[2]->id()); { @@ -4187,9 +4186,9 @@ TEST_F(ExtensionServiceTest, GetSyncDataList) { TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::APPS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); service_->DisableExtension(page_action); TerminateExtension(theme2_crx); @@ -4202,7 +4201,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataUninstall) { InitializeEmptyExtensionService(); TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); sync_pb::EntitySpecifics specifics; sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension(); @@ -4277,7 +4276,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) { InitializeExtensionProcessManager(); TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); EXPECT_TRUE(service_->IsExtensionEnabled(good_crx)); @@ -4331,7 +4330,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataTerminatedExtension) { InitializeExtensionServiceWithUpdater(); TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); TerminateExtension(good_crx); @@ -4362,7 +4361,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { InitializeRequestContext(); TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); EXPECT_TRUE(service_->IsExtensionEnabled(good_crx)); @@ -4419,7 +4418,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) { InitializeRequestContext(); TestSyncProcessorStub processor; service_->MergeDataAndStartSyncing(syncable::EXTENSIONS, SyncDataList(), - &processor); + scoped_ptr<SyncChangeProcessor>(new TestSyncProcessorStub)); sync_pb::EntitySpecifics specifics; sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension(); diff --git a/chrome/browser/extensions/settings/settings_apitest.cc b/chrome/browser/extensions/settings/settings_apitest.cc index c5990cc0..2bfa7be 100644 --- a/chrome/browser/extensions/settings/settings_apitest.cc +++ b/chrome/browser/extensions/settings/settings_apitest.cc @@ -42,6 +42,28 @@ class NoopSyncChangeProcessor : public SyncChangeProcessor { virtual ~NoopSyncChangeProcessor() {}; }; +class SyncChangeProcessorDelegate : public SyncChangeProcessor { + public: + explicit SyncChangeProcessorDelegate(SyncChangeProcessor* recipient) + : recipient_(recipient) { + DCHECK(recipient_); + } + virtual ~SyncChangeProcessorDelegate() {} + + // SyncChangeProcessor implementation. + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE { + return recipient_->ProcessSyncChanges(from_here, change_list); + } + + private: + // The recipient of all sync changes. + SyncChangeProcessor* recipient_; + + DISALLOW_COPY_AND_ASSIGN(SyncChangeProcessorDelegate); +}; + } // namespace class ExtensionSettingsApiTest : public ExtensionApiTest { @@ -139,7 +161,8 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { EXPECT_FALSE(settings_service->MergeDataAndStartSyncing( kModelType, SyncDataList(), - sync_processor).IsSet()); + scoped_ptr<SyncChangeProcessor>( + new SyncChangeProcessorDelegate(sync_processor))).IsSet()); } void SendChangesToSyncableService( diff --git a/chrome/browser/extensions/settings/settings_backend.cc b/chrome/browser/extensions/settings/settings_backend.cc index 1ee4516..acc1cfb 100644 --- a/chrome/browser/extensions/settings/settings_backend.cc +++ b/chrome/browser/extensions/settings/settings_backend.cc @@ -31,8 +31,7 @@ SettingsBackend::SettingsBackend( base_path_(base_path), quota_(quota), observers_(observers), - sync_type_(syncable::UNSPECIFIED), - sync_processor_(NULL) { + sync_type_(syncable::UNSPECIFIED) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); } @@ -72,9 +71,11 @@ SyncableSettingsStorage* SettingsBackend::GetOrCreateStorageWithSyncData( storage)); storage_objs_[extension_id] = syncable_storage; - if (sync_processor_) { + if (sync_processor_.get()) { SyncError error = - syncable_storage->StartSyncing(sync_type_, sync_data, sync_processor_); + syncable_storage->StartSyncing(sync_type_, + sync_data, + sync_processor_.get()); if (error.IsSet()) { syncable_storage.get()->StopSyncing(); } @@ -171,15 +172,16 @@ SyncDataList SettingsBackend::GetAllSyncData( SyncError SettingsBackend::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DCHECK(type == syncable::EXTENSION_SETTINGS || type == syncable::APP_SETTINGS); DCHECK_EQ(sync_type_, syncable::UNSPECIFIED); - DCHECK(!sync_processor_); + DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); sync_type_ = type; - sync_processor_ = sync_processor; + sync_processor_ = sync_processor.Pass(); // Group the initial sync data by extension id. std::map<std::string, linked_ptr<DictionaryValue> > grouped_sync_data; @@ -206,11 +208,11 @@ SyncError SettingsBackend::MergeDataAndStartSyncing( SyncError error; if (maybe_sync_data != grouped_sync_data.end()) { error = it->second->StartSyncing( - type, *maybe_sync_data->second, sync_processor); + type, *maybe_sync_data->second, sync_processor_.get()); grouped_sync_data.erase(it->first); } else { DictionaryValue empty; - error = it->second->StartSyncing(type, empty, sync_processor); + error = it->second->StartSyncing(type, empty, sync_processor_.get()); } if (error.IsSet()) { it->second->StopSyncing(); @@ -232,7 +234,7 @@ SyncError SettingsBackend::ProcessSyncChanges( const tracked_objects::Location& from_here, const SyncChangeList& sync_changes) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - DCHECK(sync_processor_); + DCHECK(sync_processor_.get()); // Group changes by extension, to pass all changes in a single method call. std::map<std::string, SettingSyncDataList> grouped_sync_data; @@ -261,10 +263,6 @@ void SettingsBackend::StopSyncing(syncable::ModelType type) { DCHECK(type == syncable::EXTENSION_SETTINGS || type == syncable::APP_SETTINGS); DCHECK_EQ(type, sync_type_); - DCHECK(sync_processor_); - - sync_type_ = syncable::UNSPECIFIED; - sync_processor_ = NULL; for (StorageObjMap::iterator it = storage_objs_.begin(); it != storage_objs_.end(); ++it) { @@ -272,6 +270,9 @@ void SettingsBackend::StopSyncing(syncable::ModelType type) { // and syncing was disabled, but StopSyncing is safe to call multiple times. it->second->StopSyncing(); } + + sync_type_ = syncable::UNSPECIFIED; + sync_processor_.reset(); } } // namespace extensions diff --git a/chrome/browser/extensions/settings/settings_backend.h b/chrome/browser/extensions/settings/settings_backend.h index 6f09cd6..26bea44 100644 --- a/chrome/browser/extensions/settings/settings_backend.h +++ b/chrome/browser/extensions/settings/settings_backend.h @@ -48,7 +48,7 @@ class SettingsBackend : public SyncableService { virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) OVERRIDE; + scoped_ptr<SyncChangeProcessor> sync_processor) OVERRIDE; virtual SyncError ProcessSyncChanges( const tracked_objects::Location& from_here, const SyncChangeList& change_list) OVERRIDE; @@ -88,7 +88,7 @@ class SettingsBackend : public SyncableService { syncable::ModelType sync_type_; // Current sync processor, if any. - SyncChangeProcessor* sync_processor_; + scoped_ptr<SyncChangeProcessor> sync_processor_; DISALLOW_COPY_AND_ASSIGN(SettingsBackend); }; diff --git a/chrome/browser/extensions/settings/settings_sync_unittest.cc b/chrome/browser/extensions/settings/settings_sync_unittest.cc index 87945e9..175d18f 100644 --- a/chrome/browser/extensions/settings/settings_sync_unittest.cc +++ b/chrome/browser/extensions/settings/settings_sync_unittest.cc @@ -142,6 +142,28 @@ class MockSyncChangeProcessor : public SyncChangeProcessor { bool fail_all_requests_; }; +class SyncChangeProcessorDelegate : public SyncChangeProcessor { + public: + explicit SyncChangeProcessorDelegate(SyncChangeProcessor* recipient) + : recipient_(recipient) { + DCHECK(recipient_); + } + virtual ~SyncChangeProcessorDelegate() {} + + // SyncChangeProcessor implementation. + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE { + return recipient_->ProcessSyncChanges(from_here, change_list); + } + + private: + // The recipient of all sync changes. + SyncChangeProcessor* recipient_; + + DISALLOW_COPY_AND_ASSIGN(SyncChangeProcessorDelegate); +}; + // SettingsStorageFactory which always returns TestingSettingsStorage objects, // and allows individually created objects to be returned. class TestingSettingsStorageFactory : public SettingsStorageFactory { @@ -182,7 +204,10 @@ class ExtensionSettingsSyncTest : public testing::Test { ExtensionSettingsSyncTest() : ui_thread_(BrowserThread::UI, MessageLoop::current()), file_thread_(BrowserThread::FILE, MessageLoop::current()), - storage_factory_(new util::ScopedSettingsStorageFactory()) {} + storage_factory_(new util::ScopedSettingsStorageFactory()), + sync_processor_(new MockSyncChangeProcessor), + sync_processor_delegate_(new SyncChangeProcessorDelegate( + sync_processor_.get())) {} virtual void SetUp() OVERRIDE { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); @@ -233,10 +258,11 @@ class ExtensionSettingsSyncTest : public testing::Test { content::TestBrowserThread file_thread_; ScopedTempDir temp_dir_; - MockSyncChangeProcessor sync_; scoped_ptr<util::MockProfile> profile_; scoped_ptr<SettingsFrontend> frontend_; scoped_refptr<util::ScopedSettingsStorageFactory> storage_factory_; + scoped_ptr<MockSyncChangeProcessor> sync_processor_; + scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_; }; // Get a semblance of coverage for both EXTENSION_SETTINGS and APP_SETTINGS @@ -253,14 +279,16 @@ TEST_F(ExtensionSettingsSyncTest, NoDataDoesNotInvokeSync) { EXPECT_EQ(0u, GetAllSyncData(model_type).size()); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); AddExtensionAndGetStorage("s2", type); EXPECT_EQ(0u, GetAllSyncData(model_type).size()); GetSyncableService(model_type)->StopSyncing(model_type); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); EXPECT_EQ(0u, GetAllSyncData(model_type).size()); } @@ -293,19 +321,20 @@ TEST_F(ExtensionSettingsSyncTest, InSyncDataDoesNotInvokeSync) { "s2", "bar", value2, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // Already in sync, so no changes. - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); // Regression test: not-changing the synced value shouldn't result in a sync // change, and changing the synced value should result in an update. storage1->Set(DEFAULTS, "foo", value1); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); storage1->Set(DEFAULTS, "foo", value2); - EXPECT_EQ(1u, sync_.changes().size()); - SettingSyncData change = sync_.GetOnlyChange("s1", "foo"); + EXPECT_EQ(1u, sync_processor_->changes().size()); + SettingSyncData change = sync_processor_->GetOnlyChange("s1", "foo"); EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); @@ -327,14 +356,16 @@ TEST_F(ExtensionSettingsSyncTest, LocalDataWithNoSyncDataIsPushedToSync) { storage2->Set(DEFAULTS, "bar", value2); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // All settings should have been pushed to sync. - EXPECT_EQ(2u, sync_.changes().size()); - SettingSyncData change = sync_.GetOnlyChange("s1", "foo"); + EXPECT_EQ(2u, sync_processor_->changes().size()); + SettingSyncData change = sync_processor_->GetOnlyChange("s1", "foo"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value1.Equals(&change.value())); - change = sync_.GetOnlyChange("s2", "bar"); + change = sync_processor_->GetOnlyChange("s2", "bar"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); @@ -363,14 +394,15 @@ TEST_F(ExtensionSettingsSyncTest, AnySyncDataOverwritesLocalData) { sync_data.push_back(settings_sync_util::CreateData( "s2", "bar", value2, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); expected1.Set("foo", value1.DeepCopy()); expected2.Set("bar", value2.DeepCopy()); SettingsStorage* storage2 = AddExtensionAndGetStorage("s2", type); // All changes should be local, so no sync changes. - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); // Sync settings should have been pushed to local settings. EXPECT_PRED_FORMAT2(SettingsEq, expected1, storage1->Get()); @@ -403,7 +435,8 @@ TEST_F(ExtensionSettingsSyncTest, ProcessSyncChanges) { "s2", "bar", value2, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); expected2.Set("bar", value2.DeepCopy()); // Make sync add some settings. @@ -475,7 +508,8 @@ TEST_F(ExtensionSettingsSyncTest, PushToSync) { "s4", "bar", value2, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // Add something locally. storage1->Set(DEFAULTS, "bar", value2); @@ -483,45 +517,45 @@ TEST_F(ExtensionSettingsSyncTest, PushToSync) { storage3->Set(DEFAULTS, "foo", value1); storage4->Set(DEFAULTS, "foo", value1); - SettingSyncData change = sync_.GetOnlyChange("s1", "bar"); + SettingSyncData change = sync_processor_->GetOnlyChange("s1", "bar"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); - sync_.GetOnlyChange("s2", "bar"); + sync_processor_->GetOnlyChange("s2", "bar"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); - change = sync_.GetOnlyChange("s3", "foo"); + change = sync_processor_->GetOnlyChange("s3", "foo"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value1.Equals(&change.value())); - change = sync_.GetOnlyChange("s4", "foo"); + change = sync_processor_->GetOnlyChange("s4", "foo"); EXPECT_EQ(SyncChange::ACTION_ADD, change.change_type()); EXPECT_TRUE(value1.Equals(&change.value())); // Change something locally, storage1/3 the new setting and storage2/4 the // initial setting, for all combinations of local vs sync intialisation and // new vs initial. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); storage1->Set(DEFAULTS, "bar", value1); storage2->Set(DEFAULTS, "foo", value2); storage3->Set(DEFAULTS, "bar", value1); storage4->Set(DEFAULTS, "foo", value2); - change = sync_.GetOnlyChange("s1", "bar"); + change = sync_processor_->GetOnlyChange("s1", "bar"); EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type()); EXPECT_TRUE(value1.Equals(&change.value())); - change = sync_.GetOnlyChange("s2", "foo"); + change = sync_processor_->GetOnlyChange("s2", "foo"); EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); - change = sync_.GetOnlyChange("s3", "bar"); + change = sync_processor_->GetOnlyChange("s3", "bar"); EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type()); EXPECT_TRUE(value1.Equals(&change.value())); - change = sync_.GetOnlyChange("s4", "foo"); + change = sync_processor_->GetOnlyChange("s4", "foo"); EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type()); EXPECT_TRUE(value2.Equals(&change.value())); // Remove something locally, storage1/3 the new setting and storage2/4 the // initial setting, for all combinations of local vs sync intialisation and // new vs initial. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); storage1->Remove("foo"); storage2->Remove("bar"); storage3->Remove("foo"); @@ -529,25 +563,25 @@ TEST_F(ExtensionSettingsSyncTest, PushToSync) { EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s1", "foo").change_type()); + sync_processor_->GetOnlyChange("s1", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s2", "bar").change_type()); + sync_processor_->GetOnlyChange("s2", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s3", "foo").change_type()); + sync_processor_->GetOnlyChange("s3", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s4", "bar").change_type()); + sync_processor_->GetOnlyChange("s4", "bar").change_type()); // Remove some nonexistent settings. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); storage1->Remove("foo"); storage2->Remove("bar"); storage3->Remove("foo"); storage4->Remove("bar"); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); // Clear the rest of the settings. Add the removed ones back first so that // more than one setting is cleared. @@ -556,7 +590,7 @@ TEST_F(ExtensionSettingsSyncTest, PushToSync) { storage3->Set(DEFAULTS, "foo", value1); storage4->Set(DEFAULTS, "bar", value2); - sync_.ClearChanges(); + sync_processor_->ClearChanges(); storage1->Clear(); storage2->Clear(); storage3->Clear(); @@ -564,28 +598,28 @@ TEST_F(ExtensionSettingsSyncTest, PushToSync) { EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s1", "foo").change_type()); + sync_processor_->GetOnlyChange("s1", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s1", "bar").change_type()); + sync_processor_->GetOnlyChange("s1", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s2", "foo").change_type()); + sync_processor_->GetOnlyChange("s2", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s2", "bar").change_type()); + sync_processor_->GetOnlyChange("s2", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s3", "foo").change_type()); + sync_processor_->GetOnlyChange("s3", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s3", "bar").change_type()); + sync_processor_->GetOnlyChange("s3", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s4", "foo").change_type()); + sync_processor_->GetOnlyChange("s4", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_DELETE, - sync_.GetOnlyChange("s4", "bar").change_type()); + sync_processor_->GetOnlyChange("s4", "bar").change_type()); GetSyncableService(model_type)->StopSyncing(model_type); } @@ -621,21 +655,27 @@ TEST_F(ExtensionSettingsSyncTest, ExtensionAndAppSettingsSyncSeparately) { sync_data.push_back(settings_sync_util::CreateData( "s1", "foo", value1, syncable::EXTENSION_SETTINGS)); - GetSyncableService(syncable::EXTENSION_SETTINGS)-> - MergeDataAndStartSyncing(syncable::EXTENSION_SETTINGS, sync_data, &sync_); + GetSyncableService(syncable::EXTENSION_SETTINGS)->MergeDataAndStartSyncing( + syncable::EXTENSION_SETTINGS, + sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); GetSyncableService(syncable::EXTENSION_SETTINGS)-> StopSyncing(syncable::EXTENSION_SETTINGS); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); sync_data.clear(); sync_data.push_back(settings_sync_util::CreateData( "s2", "bar", value2, syncable::APP_SETTINGS)); - GetSyncableService(syncable::APP_SETTINGS)-> - MergeDataAndStartSyncing(syncable::APP_SETTINGS, sync_data, &sync_); + scoped_ptr<SyncChangeProcessorDelegate> app_settings_delegate_( + new SyncChangeProcessorDelegate(sync_processor_.get())); + GetSyncableService(syncable::APP_SETTINGS)->MergeDataAndStartSyncing( + syncable::APP_SETTINGS, + sync_data, + app_settings_delegate_.PassAs<SyncChangeProcessor>()); GetSyncableService(syncable::APP_SETTINGS)-> StopSyncing(syncable::APP_SETTINGS); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); } TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { @@ -664,7 +704,9 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { sync_data.push_back(settings_sync_util::CreateData( "bad", "foo", fooValue, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, + sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); } testing_factory->GetExisting("bad")->SetFailAllRequests(false); @@ -679,14 +721,14 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { } // Changes made to good should be sent to sync, changes from bad shouldn't. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", barValue); bad->Set(DEFAULTS, "bar", barValue); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); { DictionaryValue dict; @@ -727,14 +769,14 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { // Changes made to bad still shouldn't go to sync, even though it didn't fail // last time. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", fooValue); bad->Set(DEFAULTS, "bar", fooValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "bar").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); { DictionaryValue dict; @@ -774,36 +816,40 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { } // Restarting sync should make bad start syncing again. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); GetSyncableService(model_type)->StopSyncing(model_type); + sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( + sync_processor_.get())); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // Local settings will have been pushed to sync, since it's empty (in this // test; presumably it wouldn't be live, since we've been getting changes). EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "bar").change_type()); - EXPECT_EQ(3u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "bar").change_type()); + EXPECT_EQ(3u, sync_processor_->changes().size()); // Live local changes now get pushed, too. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", barValue); bad->Set(DEFAULTS, "bar", barValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("bad", "bar").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "bar").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); // And ProcessSyncChanges work, too. { @@ -852,10 +898,12 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) { sync_data.push_back(settings_sync_util::CreateData( "bad", "foo", fooValue, model_type)); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, sync_data, &sync_); + model_type, + sync_data, + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); } - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); { DictionaryValue dict; @@ -893,14 +941,14 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) { } // No more changes sent to sync for bad. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "foo", barValue); bad->Set(DEFAULTS, "foo", barValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); // No more changes received from sync should go to bad. { @@ -955,27 +1003,29 @@ TEST_F(ExtensionSettingsSyncTest, FailingGetAllSyncDataDoesntStopSync) { // Sync shouldn't be disabled for good (nor bad -- but this is unimportant). GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "foo").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "foo").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", barValue); bad->Set(DEFAULTS, "bar", barValue); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "bar").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "bar").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); } TEST_F(ExtensionSettingsSyncTest, FailureToReadChangesToPushDisablesSync) { @@ -999,23 +1049,25 @@ TEST_F(ExtensionSettingsSyncTest, FailureToReadChangesToPushDisablesSync) { // get them so won't. testing_factory->GetExisting("bad")->SetFailAllRequests(true); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); testing_factory->GetExisting("bad")->SetFailAllRequests(false); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); // bad should now be disabled for sync. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", barValue); bad->Set(DEFAULTS, "bar", barValue); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); { SyncChangeList change_list; @@ -1043,36 +1095,40 @@ TEST_F(ExtensionSettingsSyncTest, FailureToReadChangesToPushDisablesSync) { // Re-enabling sync without failing should cause the local changes from bad // to be pushed to sync successfully, as should future changes to bad. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); GetSyncableService(model_type)->StopSyncing(model_type); + sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( + sync_processor_.get())); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "foo").change_type()); + sync_processor_->GetOnlyChange("bad", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "bar").change_type()); - EXPECT_EQ(4u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "bar").change_type()); + EXPECT_EQ(4u, sync_processor_->changes().size()); - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "bar", fooValue); bad->Set(DEFAULTS, "bar", fooValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "bar").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); } TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalStateDisablesSync) { @@ -1092,20 +1148,22 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalStateDisablesSync) { // Only set bad; setting good will cause it to fail below. bad->Set(DEFAULTS, "foo", fooValue); - sync_.SetFailAllRequests(true); + sync_processor_->SetFailAllRequests(true); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); - sync_.SetFailAllRequests(false); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); + sync_processor_->SetFailAllRequests(false); // Changes from good will be send to sync, changes from bad won't. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "foo", barValue); bad->Set(DEFAULTS, "foo", barValue); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); // Changes from sync will be sent to good, not to bad. { @@ -1130,33 +1188,37 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalStateDisablesSync) { } // Restarting sync makes everything work again. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); GetSyncableService(model_type)->StopSyncing(model_type); + sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( + sync_processor_.get())); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "foo").change_type()); - EXPECT_EQ(3u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "foo").change_type()); + EXPECT_EQ(3u, sync_processor_->changes().size()); - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "foo", fooValue); bad->Set(DEFAULTS, "foo", fooValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); } TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalChangeDisablesSync) { @@ -1174,28 +1236,30 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalChangeDisablesSync) { SettingsStorage* bad = AddExtensionAndGetStorage("bad", type); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // bad will fail to send changes. good->Set(DEFAULTS, "foo", fooValue); - sync_.SetFailAllRequests(true); + sync_processor_->SetFailAllRequests(true); bad->Set(DEFAULTS, "foo", fooValue); - sync_.SetFailAllRequests(false); + sync_processor_->SetFailAllRequests(false); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); // No further changes should be sent from bad. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "foo", barValue); bad->Set(DEFAULTS, "foo", barValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(1u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(1u, sync_processor_->changes().size()); // Changes from sync will be sent to good, not to bad. { @@ -1220,37 +1284,41 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalChangeDisablesSync) { } // Restarting sync makes everything work again. - sync_.ClearChanges(); + sync_processor_->ClearChanges(); GetSyncableService(model_type)->StopSyncing(model_type); + sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( + sync_processor_.get())); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("good", "bar").change_type()); + sync_processor_->GetOnlyChange("good", "bar").change_type()); EXPECT_EQ( SyncChange::ACTION_ADD, - sync_.GetOnlyChange("bad", "foo").change_type()); - EXPECT_EQ(3u, sync_.changes().size()); + sync_processor_->GetOnlyChange("bad", "foo").change_type()); + EXPECT_EQ(3u, sync_processor_->changes().size()); - sync_.ClearChanges(); + sync_processor_->ClearChanges(); good->Set(DEFAULTS, "foo", fooValue); bad->Set(DEFAULTS, "foo", fooValue); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); EXPECT_EQ( SyncChange::ACTION_UPDATE, - sync_.GetOnlyChange("good", "foo").change_type()); - EXPECT_EQ(2u, sync_.changes().size()); + sync_processor_->GetOnlyChange("good", "foo").change_type()); + EXPECT_EQ(2u, sync_processor_->changes().size()); } TEST_F(ExtensionSettingsSyncTest, - LargeOutgoingChangeRejectedButIncomingAccepted) { + LargeOutgoingChangeRejectedButIncomingAccepted) { syncable::ModelType model_type = syncable::APP_SETTINGS; Extension::Type type = Extension::TYPE_PACKAGED_APP; @@ -1262,12 +1330,14 @@ TEST_F(ExtensionSettingsSyncTest, StringValue large_value(string_5k); GetSyncableService(model_type)->MergeDataAndStartSyncing( - model_type, SyncDataList(), &sync_); + model_type, + SyncDataList(), + sync_processor_delegate_.PassAs<SyncChangeProcessor>()); // Large local change rejected and doesn't get sent out. SettingsStorage* storage1 = AddExtensionAndGetStorage("s1", type); EXPECT_TRUE(storage1->Set(DEFAULTS, "large_value", large_value).HasError()); - EXPECT_EQ(0u, sync_.changes().size()); + EXPECT_EQ(0u, sync_processor_->changes().size()); // Large incoming change should still get accepted. SettingsStorage* storage2 = AddExtensionAndGetStorage("s2", type); diff --git a/chrome/browser/extensions/settings/syncable_settings_storage.cc b/chrome/browser/extensions/settings/syncable_settings_storage.cc index 51c52b4..5d780ce 100644 --- a/chrome/browser/extensions/settings/syncable_settings_storage.cc +++ b/chrome/browser/extensions/settings/syncable_settings_storage.cc @@ -140,6 +140,7 @@ SyncError SyncableSettingsStorage::StartSyncing( type == syncable::APP_SETTINGS); DCHECK_EQ(sync_type_, syncable::UNSPECIFIED); DCHECK(!sync_processor_); + DCHECK(sync_processor); DCHECK(synced_keys_.empty()); sync_type_ = type; diff --git a/chrome/browser/extensions/test_extension_service.cc b/chrome/browser/extensions/test_extension_service.cc index 7ae9135..47111c7 100644 --- a/chrome/browser/extensions/test_extension_service.cc +++ b/chrome/browser/extensions/test_extension_service.cc @@ -68,7 +68,7 @@ void TestExtensionService::CheckForUpdatesSoon() { SyncError TestExtensionService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { ADD_FAILURE(); return SyncError(); } diff --git a/chrome/browser/extensions/test_extension_service.h b/chrome/browser/extensions/test_extension_service.h index 21fb976..121407d 100644 --- a/chrome/browser/extensions/test_extension_service.h +++ b/chrome/browser/extensions/test_extension_service.h @@ -46,7 +46,7 @@ class TestExtensionService : public ExtensionServiceInterface { virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) OVERRIDE; + scoped_ptr<SyncChangeProcessor> sync_processor) OVERRIDE; virtual void StopSyncing(syncable::ModelType type) OVERRIDE; virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE; virtual SyncError ProcessSyncChanges( |