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 | |
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
43 files changed, 560 insertions, 442 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( diff --git a/chrome/browser/prefs/pref_model_associator.cc b/chrome/browser/prefs/pref_model_associator.cc index 21c53a2..b70302a 100644 --- a/chrome/browser/prefs/pref_model_associator.cc +++ b/chrome/browser/prefs/pref_model_associator.cc @@ -22,14 +22,12 @@ using syncable::PREFERENCES; PrefModelAssociator::PrefModelAssociator() : models_associated_(false), processing_syncer_changes_(false), - pref_service_(NULL), - sync_processor_(NULL) { + pref_service_(NULL) { DCHECK(CalledOnValidThread()); } PrefModelAssociator::~PrefModelAssociator() { DCHECK(CalledOnValidThread()); - sync_processor_ = NULL; pref_service_ = NULL; } @@ -109,12 +107,13 @@ void PrefModelAssociator::InitPrefAndAssociate( SyncError PrefModelAssociator::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { DCHECK_EQ(type, PREFERENCES); DCHECK(CalledOnValidThread()); DCHECK(pref_service_); - DCHECK(!sync_processor_); - sync_processor_ = sync_processor; + DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); + sync_processor_ = sync_processor.Pass(); SyncChangeList new_changes; std::set<std::string> remaining_preferences = registered_preferences_; @@ -161,7 +160,7 @@ SyncError PrefModelAssociator::MergeDataAndStartSyncing( void PrefModelAssociator::StopSyncing(syncable::ModelType type) { DCHECK_EQ(type, PREFERENCES); models_associated_ = false; - sync_processor_ = NULL; + sync_processor_.reset(); } Value* PrefModelAssociator::MergePreference( @@ -425,8 +424,6 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) { SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes); - if (error.IsSet()) - StopSyncing(PREFERENCES); } void PrefModelAssociator::SetPrefService(PrefService* pref_service) { diff --git a/chrome/browser/prefs/pref_model_associator.h b/chrome/browser/prefs/pref_model_associator.h index 3d2fd1f..de4b645 100644 --- a/chrome/browser/prefs/pref_model_associator.h +++ b/chrome/browser/prefs/pref_model_associator.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 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. @@ -43,7 +43,7 @@ class PrefModelAssociator 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; // Returns the list of preference names that are registered as syncable, and @@ -141,7 +141,7 @@ class PrefModelAssociator PrefService* pref_service_; // Sync's SyncChange handler. We push all our changes through this. - SyncChangeProcessor* sync_processor_; + scoped_ptr<SyncChangeProcessor> sync_processor_; DISALLOW_COPY_AND_ASSIGN(PrefModelAssociator); }; diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index 4977bc5..3c40566 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -115,7 +115,6 @@ TemplateURLService::TemplateURLService(Profile* profile) time_provider_(&base::Time::Now), models_associated_(false), processing_syncer_changes_(false), - sync_processor_(NULL), pending_synced_default_search_(false) { DCHECK(profile_); Init(NULL, 0); @@ -134,7 +133,6 @@ TemplateURLService::TemplateURLService(const Initializer* initializers, time_provider_(&base::Time::Now), models_associated_(false), processing_syncer_changes_(false), - sync_processor_(NULL), pending_synced_default_search_(false) { Init(initializers, count); } @@ -824,11 +822,12 @@ SyncError TemplateURLService::ProcessSyncChanges( SyncError TemplateURLService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { DCHECK(loaded()); DCHECK_EQ(type, syncable::SEARCH_ENGINES); - DCHECK(!sync_processor_); - sync_processor_ = sync_processor; + DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); + sync_processor_ = sync_processor.Pass(); // We just started syncing, so set our wait-for-default flag if we are // expecting a default from Sync. @@ -936,7 +935,7 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( void TemplateURLService::StopSyncing(syncable::ModelType type) { DCHECK_EQ(type, syncable::SEARCH_ENGINES); models_associated_ = false; - sync_processor_ = NULL; + sync_processor_.reset(); } void TemplateURLService::ProcessTemplateURLChange( @@ -1603,7 +1602,8 @@ void TemplateURLService::SetDefaultSearchProviderNoNotify( // If we are syncing, we want to set the synced pref that will notify other // instances to change their default to this new search provider. - if (sync_processor_ && url && !url->sync_guid().empty() && GetPrefs()) { + if (sync_processor_.get() && url && !url->sync_guid().empty() && + GetPrefs()) { GetPrefs()->SetString(prefs::kSyncedDefaultSearchProviderGUID, url->sync_guid()); } @@ -1849,7 +1849,7 @@ void TemplateURLService::MergeSyncAndLocalURLDuplicates( void TemplateURLService::SetDefaultSearchProviderIfNewlySynced( const std::string& guid) { // If we're not syncing or if default search is managed by policy, ignore. - if (!sync_processor_ || is_default_search_managed_) + if (!sync_processor_.get() || is_default_search_managed_) return; PrefService* prefs = GetPrefs(); diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h index c8ee77c..9fd8eed 100644 --- a/chrome/browser/search_engines/template_url_service.h +++ b/chrome/browser/search_engines/template_url_service.h @@ -273,7 +273,7 @@ class TemplateURLService : public WebDataServiceConsumer, 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; // Processes a local TemplateURL change for Sync. |turl| is the TemplateURL @@ -584,7 +584,7 @@ class TemplateURLService : public WebDataServiceConsumer, bool processing_syncer_changes_; // Sync's SyncChange handler. We push all our changes through this. - SyncChangeProcessor* sync_processor_; + scoped_ptr<SyncChangeProcessor> sync_processor_; // Whether or not we are waiting on the default search provider to come in // from Sync. This is to facilitate the fact that changes to the value of diff --git a/chrome/browser/search_engines/template_url_service_sync_unittest.cc b/chrome/browser/search_engines/template_url_service_sync_unittest.cc index f7f9119..0b59038 100644 --- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc +++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc @@ -127,11 +127,36 @@ 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); +}; + class TemplateURLServiceSyncTest : public testing::Test { public: typedef TemplateURLService::SyncDataMap SyncDataMap; - TemplateURLServiceSyncTest() {} + TemplateURLServiceSyncTest() + : sync_processor_(new TestChangeProcessor), + sync_processor_delegate_(new SyncChangeProcessorDelegate( + sync_processor_.get())) {} virtual void SetUp() { profile_a_.reset(new TestingProfile); @@ -153,7 +178,10 @@ class TemplateURLServiceSyncTest : public testing::Test { // involve syncing two models. TemplateURLService* model_a() { return model_a_.get(); } TemplateURLService* model_b() { return model_b_.get(); } - TestChangeProcessor* processor() { return &processor_; } + TestChangeProcessor* processor() { return sync_processor_.get(); } + scoped_ptr<SyncChangeProcessor> PassProcessor() { + return sync_processor_delegate_.PassAs<SyncChangeProcessor>(); + } // Create a TemplateURL with some test values. The caller owns the returned // TemplateURL*. @@ -275,7 +303,8 @@ class TemplateURLServiceSyncTest : public testing::Test { scoped_ptr<TemplateURLService> model_b_; // Our dummy ChangeProcessor used to inspect changes pushed to Sync. - TestChangeProcessor processor_; + scoped_ptr<TestChangeProcessor> sync_processor_; + scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_; DISALLOW_COPY_AND_ASSIGN(TemplateURLServiceSyncTest); }; @@ -518,7 +547,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncAndLocalURLDuplicates) { TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) { model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, SyncDataList(), - processor()); + PassProcessor()); EXPECT_EQ(0U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); EXPECT_EQ(0, processor()->change_list_size()); @@ -528,7 +557,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) { SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); // We expect the model to have accepted all of the initial sync data. Search @@ -552,7 +581,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) { SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(6U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); // We expect the model to have accepted all of the initial sync data. Search @@ -585,7 +614,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncIsTheSame) { } model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); for (SyncDataList::const_iterator iter = initial_data.begin(); @@ -618,7 +647,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeUpdateFromSync) { TemplateURLService::CreateSyncDataFromTemplateURL(*turl2_older)); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); // Both were local updates, so we expect the same count. EXPECT_EQ(2U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -651,7 +680,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) { "http://unique.com", "ccc")); // add model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); // The dupe results in a merge. The other two should be added to the model. EXPECT_EQ(5U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -711,7 +740,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) { "http://unique.com", "ccc", 10)); // add model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); // The dupe results in a merge. The other two should be added to the model. EXPECT_EQ(5U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -750,7 +779,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) { TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) { // We initially have no data. model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, SyncDataList(), - processor()); + PassProcessor()); // Set up a bunch of ADDs. SyncChangeList changes; @@ -772,7 +801,7 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) { TEST_F(TemplateURLServiceSyncTest, ProcessChangesNoConflicts) { model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Process different types of changes, without conflicts. SyncChangeList changes; @@ -801,7 +830,7 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesNoConflicts) { TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) { model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Process different types of changes, with conflicts. Note that all this data // has a newer timestamp, so Sync will win in these scenarios. @@ -837,7 +866,7 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) { TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsLocalWins) { model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Process different types of changes, with conflicts. Note that all this data // has an older timestamp, so the local data will win in these scenarios. @@ -884,7 +913,7 @@ TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) { // Ensure that ProcessTemplateURLChange is called and pushes the correct // changes to Sync whenever local changes are made to TemplateURLs. model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Add a new search engine. TemplateURL* new_turl = @@ -919,12 +948,15 @@ TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) { TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsBasic) { // Start off B with some empty data. model_b()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Merge A and B. All of B's data should transfer over to A, which initially // has no data. + scoped_ptr<SyncChangeProcessorDelegate> delegate_b( + new SyncChangeProcessorDelegate(model_b())); model_a()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - model_b()->GetAllSyncData(syncable::SEARCH_ENGINES), model_b()); + model_b()->GetAllSyncData(syncable::SEARCH_ENGINES), + delegate_b.PassAs<SyncChangeProcessor>()); // They should be consistent. AssertEquals(model_a()->GetAllSyncData(syncable::SEARCH_ENGINES), @@ -934,7 +966,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsBasic) { TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { // Start off B with some empty data. model_b()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); // Set up A so we have some interesting duplicates and conflicts. model_a()->Add(CreateTestTemplateURL(ASCIIToUTF16("key4"), "http://key4.com", @@ -947,8 +979,11 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { "key6", 10)); // Conflict with key1 // Merge A and B. + scoped_ptr<SyncChangeProcessorDelegate> delegate_b( + new SyncChangeProcessorDelegate(model_b())); model_a()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - model_b()->GetAllSyncData(syncable::SEARCH_ENGINES), model_b()); + model_b()->GetAllSyncData(syncable::SEARCH_ENGINES), + delegate_b.PassAs<SyncChangeProcessor>()); // They should be consistent. AssertEquals(model_a()->GetAllSyncData(syncable::SEARCH_ENGINES), @@ -957,7 +992,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { TEST_F(TemplateURLServiceSyncTest, StopSyncing) { SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); ASSERT_FALSE(error.IsSet()); model()->StopSyncing(syncable::SEARCH_ENGINES); @@ -976,7 +1011,7 @@ TEST_F(TemplateURLServiceSyncTest, StopSyncing) { TEST_F(TemplateURLServiceSyncTest, SyncErrorOnInitialSync) { processor()->set_erroneous(true); SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); EXPECT_TRUE(error.IsSet()); // Ensure that if the initial merge was erroneous, then subsequence attempts @@ -999,7 +1034,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncErrorOnLaterSync) { // Ensure that if the SyncProcessor succeeds in the initial merge, but fails // in future ProcessSyncChanges, we still return an error. SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); ASSERT_FALSE(error.IsSet()); SyncChangeList changes; @@ -1021,7 +1056,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) { "key1", 10)); // earlier SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - initial_data, processor()); + initial_data, PassProcessor()); ASSERT_FALSE(error.IsSet()); // We should have updated the original TemplateURL with Sync's version. @@ -1041,8 +1076,10 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) { // Remerge the data again. This simulates shutting down and syncing again // at a different time, but the cloud data has not changed. model()->StopSyncing(syncable::SEARCH_ENGINES); + sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( + sync_processor_.get())); error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - initial_data, processor()); + initial_data, PassProcessor()); ASSERT_FALSE(error.IsSet()); // Check that the TemplateURL was not modified. @@ -1054,7 +1091,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) { TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) { SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key2")); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1115,7 +1152,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultArrivesAfterStartup) { // destined to become the new default. SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); // Ensure that the new default has been set. EXPECT_EQ(4U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1143,7 +1180,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultAlreadySetOnStartup) { // Now sync the initial data. SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); // Ensure that the new entries were added and the default has not changed. EXPECT_EQ(4U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1155,7 +1192,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncWithManagedDefaultSearch) { // default search provider. SyncDataList initial_data = CreateInitialSyncData(); model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - processor()); + PassProcessor()); model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key2")); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1211,7 +1248,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncMergeDeletesDefault) { // The key1 entry should be a duplicate of the default. model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, - CreateInitialSyncData(), processor()); + CreateInitialSyncData(), PassProcessor()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); EXPECT_FALSE(model()->GetTemplateURLForGUID("whateverguid")); diff --git a/chrome/browser/search_engines/template_url_service_test_util.cc b/chrome/browser/search_engines/template_url_service_test_util.cc index 0e9298c..4388e7f 100644 --- a/chrome/browser/search_engines/template_url_service_test_util.cc +++ b/chrome/browser/search_engines/template_url_service_test_util.cc @@ -237,3 +237,7 @@ TestingProfile* TemplateURLServiceTestUtil::profile() const { void TemplateURLServiceTestUtil::StartIOThread() { profile_->StartIOThread(); } + +void TemplateURLServiceTestUtil::PumpLoop() { + message_loop_.RunAllPending(); +} diff --git a/chrome/browser/search_engines/template_url_service_test_util.h b/chrome/browser/search_engines/template_url_service_test_util.h index f7fdc5f..6c52eaf 100644 --- a/chrome/browser/search_engines/template_url_service_test_util.h +++ b/chrome/browser/search_engines/template_url_service_test_util.h @@ -86,6 +86,9 @@ class TemplateURLServiceTestUtil : public TemplateURLServiceObserver { // Starts an I/O thread. void StartIOThread(); + // Runs all pending tasks on the UI loop. + void PumpLoop(); + private: MessageLoopForUI message_loop_; // Needed to make the DeleteOnUIThread trait of WebDataService work diff --git a/chrome/browser/sync/api/fake_syncable_service.cc b/chrome/browser/sync/api/fake_syncable_service.cc index 9823dbd..2f7d948 100644 --- a/chrome/browser/sync/api/fake_syncable_service.cc +++ b/chrome/browser/sync/api/fake_syncable_service.cc @@ -30,8 +30,8 @@ bool FakeSyncableService::syncing() const { SyncError FakeSyncableService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { - sync_processor_.reset(sync_processor); + scoped_ptr<SyncChangeProcessor> sync_processor) { + sync_processor_ = sync_processor.Pass(); type_ = type; if (!merge_data_and_start_syncing_error_.IsSet()) { syncing_ = true; @@ -41,6 +41,7 @@ SyncError FakeSyncableService::MergeDataAndStartSyncing( void FakeSyncableService::StopSyncing(syncable::ModelType type) { syncing_ = false; + sync_processor_.reset(); } SyncDataList FakeSyncableService::GetAllSyncData( diff --git a/chrome/browser/sync/api/fake_syncable_service.h b/chrome/browser/sync/api/fake_syncable_service.h index 9229665..724985c 100644 --- a/chrome/browser/sync/api/fake_syncable_service.h +++ b/chrome/browser/sync/api/fake_syncable_service.h @@ -27,7 +27,7 @@ class FakeSyncableService : public SyncableService { 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( diff --git a/chrome/browser/sync/api/syncable_service.h b/chrome/browser/sync/api/syncable_service.h index 6b5152b..1e1e6a2 100644 --- a/chrome/browser/sync/api/syncable_service.h +++ b/chrome/browser/sync/api/syncable_service.h @@ -9,14 +9,13 @@ #include <vector> #include "base/compiler_specific.h" +#include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/sync/api/sync_change_processor.h" #include "chrome/browser/sync/api/sync_data.h" #include "chrome/browser/sync/api/sync_error.h" #include "sync/syncable/model_type.h" -class SyncData; - typedef std::vector<SyncData> SyncDataList; // TODO(zea): remove SupportsWeakPtr in favor of having all SyncableService @@ -37,7 +36,7 @@ class SyncableService : public SyncChangeProcessor, virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) = 0; + scoped_ptr<SyncChangeProcessor> sync_processor) = 0; // Stop syncing the specified type and reset state. virtual void StopSyncing(syncable::ModelType type) = 0; diff --git a/chrome/browser/sync/api/syncable_service_mock.cc b/chrome/browser/sync/api/syncable_service_mock.cc deleted file mode 100644 index 0f4c300..0000000 --- a/chrome/browser/sync/api/syncable_service_mock.cc +++ /dev/null @@ -1,9 +0,0 @@ -// Copyright (c) 2011 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/sync/api/syncable_service_mock.h" - -SyncableServiceMock::SyncableServiceMock() {} - -SyncableServiceMock::~SyncableServiceMock() {} diff --git a/chrome/browser/sync/api/syncable_service_mock.h b/chrome/browser/sync/api/syncable_service_mock.h deleted file mode 100644 index 2575183..0000000 --- a/chrome/browser/sync/api/syncable_service_mock.h +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) 2011 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_SYNC_API_SYNCABLE_SERVICE_MOCK_H_ -#define CHROME_BROWSER_SYNC_API_SYNCABLE_SERVICE_MOCK_H_ -#pragma once - -#include "base/location.h" -#include "chrome/browser/sync/api/syncable_service.h" -#include "chrome/browser/sync/api/sync_change.h" -#include "testing/gmock/include/gmock/gmock.h" - -class SyncableServiceMock : public SyncableService { - public: - SyncableServiceMock(); - virtual ~SyncableServiceMock(); - - MOCK_METHOD3(MergeDataAndStartSyncing, - SyncError(syncable::ModelType, - const SyncDataList&, - SyncChangeProcessor* sync_processor)); - MOCK_METHOD1(StopSyncing, void(syncable::ModelType)); - MOCK_CONST_METHOD1(GetAllSyncData, SyncDataList(syncable::ModelType type)); - MOCK_METHOD2(ProcessSyncChanges, - SyncError(const tracked_objects::Location&, - const SyncChangeList&)); -}; - -#endif // CHROME_BROWSER_SYNC_API_SYNCABLE_SERVICE_MOCK_H_ diff --git a/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc b/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc index b63ef7c..f24a469 100644 --- a/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc @@ -31,6 +31,10 @@ using testing::SetArgumentPointee; namespace browser_sync { namespace { +ACTION(MakeSharedChangeProcessor) { + return new SharedChangeProcessor(); +} + ACTION_P(ReturnAndRelease, change_processor) { return change_processor->release(); } @@ -78,7 +82,12 @@ class SyncAppNotificationDataTypeControllerTest SetStartExpectations(); } - virtual void TearDown() { } + virtual void TearDown() { + // Must be done before we pump the loop. + syncable_service_.StopSyncing(syncable::APP_NOTIFICATIONS); + app_notif_dtc_ = NULL; + PumpLoop(); + } protected: // Waits until the file thread executes all tasks queued up so far. @@ -111,6 +120,8 @@ class SyncAppNotificationDataTypeControllerTest EXPECT_CALL(*profile_sync_factory_, GetSyncableServiceForType(syncable::APP_NOTIFICATIONS)). WillOnce(Return(syncable_service_.AsWeakPtr())); + EXPECT_CALL(*profile_sync_factory_, CreateSharedChangeProcessor()). + WillOnce(MakeSharedChangeProcessor()); EXPECT_CALL(*profile_sync_factory_, CreateGenericChangeProcessor(_, _, _)). WillOnce(ReturnAndRelease(&change_processor_)); } diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc index add06f8..fc74e61 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc @@ -128,7 +128,8 @@ void NewNonFrontendDataTypeController:: error = local_service_->MergeDataAndStartSyncing( type(), initial_sync_data, - new SharedChangeProcessorRef(shared_change_processor)); + scoped_ptr<SyncChangeProcessor>( + new SharedChangeProcessorRef(shared_change_processor))); RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { StartFailed(ASSOCIATION_FAILED, error); diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc index 59f7cc7..8a702fd 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc @@ -13,7 +13,7 @@ #include "base/synchronization/waitable_event.h" #include "base/test/test_timeouts.h" #include "base/tracked_objects.h" -#include "chrome/browser/sync/api/syncable_service_mock.h" +#include "chrome/browser/sync/api/fake_syncable_service.h" #include "chrome/browser/sync/glue/data_type_controller_mock.h" #include "chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h" #include "chrome/browser/sync/glue/shared_change_processor_mock.h" @@ -204,9 +204,6 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); EXPECT_CALL(*change_processor_, GetSyncData(_)). WillOnce(Return(SyncError())); - EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_,_,_)). - WillOnce(DoAll(SaveChangeProcessor(&saved_change_processor_), - Return(SyncError()))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); } @@ -218,11 +215,9 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { EXPECT_CALL(*dtc_mock_, StopModels()); EXPECT_CALL(*change_processor_, Disconnect()).WillOnce(Return(true)); EXPECT_CALL(service_, DeactivateDataType(_)); - EXPECT_CALL(syncable_service_, StopSyncing(_)); } void SetStartFailExpectations(DataTypeController::StartResult result) { - EXPECT_CALL(syncable_service_, StopSyncing(_)); EXPECT_CALL(*dtc_mock_, StopModels()).Times(AtLeast(1)); if (DataTypeController::IsUnrecoverableResult(result)) EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _)); @@ -242,7 +237,7 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { StrictMock<ProfileSyncServiceMock> service_; StartCallbackMock start_callback_; // Must be destroyed after new_non_frontend_dtc_. - SyncableServiceMock syncable_service_; + FakeSyncableService syncable_service_; scoped_refptr<NewNonFrontendDataTypeControllerFake> new_non_frontend_dtc_; scoped_refptr<NewNonFrontendDataTypeControllerMock> dtc_mock_; scoped_refptr<SharedChangeProcessorMock> change_processor_; @@ -270,9 +265,6 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartFirstRun) { WillOnce(DoAll(SetArgumentPointee<0>(false), Return(true))); EXPECT_CALL(*change_processor_, GetSyncData(_)). WillOnce(Return(SyncError())); - EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_,_,_)). - WillOnce(DoAll(SaveChangeProcessor(&saved_change_processor_), - Return(SyncError()))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetActivateExpectations(DataTypeController::OK_FIRST_RUN); EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state()); @@ -314,13 +306,12 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartAssociationFailed) { WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); EXPECT_CALL(*change_processor_, GetSyncData(_)). WillOnce(Return(SyncError())); - EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_,_,_)). - WillOnce(DoAll(SaveChangeProcessor(&saved_change_processor_), - Return(SyncError(FROM_HERE, "failed", AUTOFILL_PROFILE)))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED); // Set up association to fail with an association failed error. EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state()); + syncable_service_.set_merge_data_and_start_syncing_error( + SyncError(FROM_HERE, "Sync Error", new_non_frontend_dtc_->type())); new_non_frontend_dtc_->Start( base::Bind(&StartCallbackMock::Run, base::Unretained(&start_callback_))); WaitForDTC(); diff --git a/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc b/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc index 311f869..897bfe6 100644 --- a/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc @@ -29,6 +29,10 @@ using testing::SetArgumentPointee; namespace browser_sync { namespace { +ACTION(MakeSharedChangeProcessor) { + return new SharedChangeProcessor(); +} + ACTION_P(ReturnAndRelease, change_processor) { return change_processor->release(); } @@ -51,6 +55,9 @@ class SyncSearchEngineDataTypeControllerTest : public testing::Test { } virtual void TearDown() { + // Must be done before we pump the loop. + syncable_service_.StopSyncing(syncable::SEARCH_ENGINES); + search_engine_dtc_ = NULL; test_util_.TearDown(); } @@ -65,6 +72,8 @@ class SyncSearchEngineDataTypeControllerTest : public testing::Test { EXPECT_CALL(*profile_sync_factory_, GetSyncableServiceForType(syncable::SEARCH_ENGINES)). WillOnce(Return(syncable_service_.AsWeakPtr())); + EXPECT_CALL(*profile_sync_factory_, CreateSharedChangeProcessor()). + WillOnce(MakeSharedChangeProcessor()); EXPECT_CALL(*profile_sync_factory_, CreateGenericChangeProcessor(_, _, _)). WillOnce(ReturnAndRelease(&change_processor_)); } @@ -194,6 +203,9 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, OnUnrecoverableError) { base::Bind(&StartCallbackMock::Run, base::Unretained(&start_callback_))); // This should cause search_engine_dtc_->Stop() to be called. search_engine_dtc_->OnUnrecoverableError(FROM_HERE, "Test"); + test_util_.PumpLoop(); + EXPECT_EQ(DataTypeController::NOT_RUNNING, search_engine_dtc_->state()); + EXPECT_FALSE(syncable_service_.syncing()); } } // namespace diff --git a/chrome/browser/sync/glue/shared_change_processor_unittest.cc b/chrome/browser/sync/glue/shared_change_processor_unittest.cc index 4a6f5fa..8279cdf 100644 --- a/chrome/browser/sync/glue/shared_change_processor_unittest.cc +++ b/chrome/browser/sync/glue/shared_change_processor_unittest.cc @@ -10,7 +10,7 @@ #include "base/bind_helpers.h" #include "base/compiler_specific.h" #include "base/message_loop.h" -#include "chrome/browser/sync/api/syncable_service_mock.h" +#include "chrome/browser/sync/api/fake_syncable_service.h" #include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/glue/data_type_error_handler_mock.h" #include "chrome/browser/sync/profile_sync_components_factory_impl.h" @@ -41,7 +41,7 @@ class SyncSharedChangeProcessorTest : public testing::Test { db_syncable_service_(NULL) {} virtual ~SyncSharedChangeProcessorTest() { - EXPECT_FALSE(db_syncable_service_); + EXPECT_FALSE(db_syncable_service_.get()); } protected: @@ -85,16 +85,15 @@ class SyncSharedChangeProcessorTest : public testing::Test { // Used by SetUp(). void SetUpDBSyncableService() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); - DCHECK(!db_syncable_service_); - db_syncable_service_ = new NiceMock<SyncableServiceMock>(); + DCHECK(!db_syncable_service_.get()); + db_syncable_service_.reset(new FakeSyncableService()); } // Used by TearDown(). void TearDownDBSyncableService() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); - DCHECK(db_syncable_service_); - delete db_syncable_service_; - db_syncable_service_ = NULL; + DCHECK(db_syncable_service_.get()); + db_syncable_service_.reset(); } // Used by Connect(). The SharedChangeProcessor is passed in @@ -104,7 +103,7 @@ class SyncSharedChangeProcessorTest : public testing::Test { const scoped_refptr<SharedChangeProcessor>& shared_change_processor) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); EXPECT_CALL(sync_factory_, GetSyncableServiceForType(syncable::AUTOFILL)). - WillOnce(GetWeakPtrToSyncableService(db_syncable_service_)); + WillOnce(GetWeakPtrToSyncableService(db_syncable_service_.get())); EXPECT_TRUE(shared_change_processor->Connect(&sync_factory_, &sync_service_, &error_handler_, @@ -121,7 +120,7 @@ class SyncSharedChangeProcessorTest : public testing::Test { StrictMock<DataTypeErrorHandlerMock> error_handler_; // Used only on DB thread. - NiceMock<SyncableServiceMock>* db_syncable_service_; + scoped_ptr<FakeSyncableService> db_syncable_service_; }; // Simply connect the shared change processor. It should succeed, and diff --git a/chrome/browser/sync/glue/ui_data_type_controller.cc b/chrome/browser/sync/glue/ui_data_type_controller.cc index e4475db..b2d3b29 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller.cc +++ b/chrome/browser/sync/glue/ui_data_type_controller.cc @@ -8,7 +8,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/api/syncable_service.h" -#include "chrome/browser/sync/glue/generic_change_processor.h" +#include "chrome/browser/sync/glue/shared_change_processor_ref.h" #include "chrome/browser/sync/profile_sync_components_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "content/public/browser/browser_thread.h" @@ -59,6 +59,13 @@ void UIDataTypeController::Start(const StartCallback& start_callback) { start_callback_ = start_callback; + // Since we can't be called multiple times before Stop() is called, + // |shared_change_processor_| must be NULL here. + DCHECK(!shared_change_processor_.get()); + shared_change_processor_ = + profile_sync_factory_->CreateSharedChangeProcessor(); + DCHECK(shared_change_processor_.get()); + state_ = MODEL_STARTING; if (!StartModels()) { // If we are waiting for some external service to load before associating @@ -84,58 +91,54 @@ bool UIDataTypeController::StartModels() { void UIDataTypeController::Associate() { DCHECK_EQ(state_, ASSOCIATING); - local_service_ = profile_sync_factory_->GetSyncableServiceForType(type()); + + // Connect |shared_change_processor_| to the syncer and get the + // SyncableService associated with type(). + local_service_ = shared_change_processor_->Connect(profile_sync_factory_, + sync_service_, + this, + type()); if (!local_service_.get()) { - SyncError error(FROM_HERE, "Failed to connect to syncable service.", - type()); + SyncError error(FROM_HERE, "Failed to connect to syncer.", type()); StartFailed(UNRECOVERABLE_ERROR, error); return; } - // We maintain ownership until MergeDataAndStartSyncing is called. - scoped_ptr<GenericChangeProcessor> sync_processor( - profile_sync_factory_->CreateGenericChangeProcessor( - sync_service_, this, local_service_)); - - if (!sync_processor->CryptoReadyIfNecessary(type())) { + if (!shared_change_processor_->CryptoReadyIfNecessary()) { StartFailed(NEEDS_CRYPTO, SyncError()); return; } bool sync_has_nodes = false; - if (!sync_processor->SyncModelHasUserCreatedNodes(type(), &sync_has_nodes)) { + if (!shared_change_processor_->SyncModelHasUserCreatedNodes( + &sync_has_nodes)) { SyncError error(FROM_HERE, "Failed to load sync nodes", type()); StartFailed(UNRECOVERABLE_ERROR, error); return; } base::TimeTicks start_time = base::TimeTicks::Now(); + SyncError error; SyncDataList initial_sync_data; - SyncError error = sync_processor->GetSyncDataForType( - type(), &initial_sync_data); + error = shared_change_processor_->GetSyncData(&initial_sync_data); if (error.IsSet()) { StartFailed(ASSOCIATION_FAILED, error); return; } - // TODO(zea): this should use scoped_ptr<T>::Pass semantics. - GenericChangeProcessor* saved_sync_processor = sync_processor.get(); - // Takes ownership of sync_processor. - error = local_service_->MergeDataAndStartSyncing(type(), - initial_sync_data, - sync_processor.release()); + // Passes a reference to |shared_change_processor_|. + error = local_service_->MergeDataAndStartSyncing( + type(), + initial_sync_data, + scoped_ptr<SyncChangeProcessor>( + new SharedChangeProcessorRef(shared_change_processor_))); + RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { StartFailed(ASSOCIATION_FAILED, error); return; } - RecordAssociationTime(base::TimeTicks::Now() - start_time); - - sync_service_->ActivateDataType(type(), model_safe_group(), - saved_sync_processor); - // StartDone(..) invokes the DataTypeManager callback, which can lead to a - // call to Stop() if one of the other data types being started generates an - // error. + shared_change_processor_->ActivateDataType(model_safe_group()); state_ = RUNNING; StartDone(sync_has_nodes ? OK : OK_FIRST_RUN); } @@ -153,6 +156,11 @@ void UIDataTypeController::StartFailed(StartResult result, } RecordStartFailure(result); + if (shared_change_processor_.get()) { + shared_change_processor_->Disconnect(); + shared_change_processor_ = NULL; + } + // We have to release the callback before we call it, since it's possible // invoking the callback will trigger a call to Stop(), which will get // confused by the non-NULL start_callback_. @@ -175,6 +183,12 @@ void UIDataTypeController::StartDone(StartResult result) { void UIDataTypeController::Stop() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(syncable::IsRealDataType(type_)); + + if (shared_change_processor_.get()) { + shared_change_processor_->Disconnect(); + shared_change_processor_ = NULL; + } + // If Stop() is called while Start() is waiting for the datatype model to // load, abort the start. if (state_ == MODEL_STARTING) { diff --git a/chrome/browser/sync/glue/ui_data_type_controller.h b/chrome/browser/sync/glue/ui_data_type_controller.h index 857e50f..ba378ce 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller.h +++ b/chrome/browser/sync/glue/ui_data_type_controller.h @@ -13,6 +13,7 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/shared_change_processor.h" class Profile; class ProfileSyncService; @@ -91,6 +92,23 @@ class UIDataTypeController : public DataTypeController { // The sync datatype being controlled. syncable::ModelType type_; + // Sync's interface to the datatype. All sync changes for |type_| are pushed + // through it to the datatype as well as vice versa. + // + // Lifetime: it gets created when Start()) is called, and a reference to it + // is passed to |local_service_| during Associate(). We release our reference + // when Stop() or StartFailed() is called, and |local_service_| releases its + // reference when local_service_->StopSyncing() is called or when it is + // destroyed. + // + // Note: we use refcounting here primarily so that we can keep a uniform + // SyncableService API, whether the datatype lives on the UI thread or not + // (a SyncableService takes ownership of its SyncChangeProcessor when + // MergeDataAndStartSyncing is called). This will help us eventually merge the + // two datatype controller implementations (for ui and non-ui thread + // datatypes). + scoped_refptr<SharedChangeProcessor> shared_change_processor_; + // A weak pointer to the actual local syncable service, which performs all the // real work. We do not own the object. base::WeakPtr<SyncableService> local_service_; diff --git a/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc b/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc index a9ed7b0..4a99e9b 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc @@ -25,6 +25,10 @@ using testing::Return; namespace browser_sync { namespace { +ACTION(MakeSharedChangeProcessor) { + return new SharedChangeProcessor(); +} + ACTION_P(ReturnAndRelease, change_processor) { return change_processor->release(); } @@ -50,12 +54,21 @@ class SyncUIDataTypeControllerTest : public testing::Test { SetStartExpectations(); } + virtual void TearDown() { + // Must be done before we pump the loop. + syncable_service_.StopSyncing(type_); + preference_dtc_ = NULL; + PumpLoop(); + } + protected: void SetStartExpectations() { // Ownership gets passed to caller of CreateGenericChangeProcessor. change_processor_.reset(new FakeGenericChangeProcessor()); EXPECT_CALL(*profile_sync_factory_, GetSyncableServiceForType(type_)). WillOnce(Return(syncable_service_.AsWeakPtr())); + EXPECT_CALL(*profile_sync_factory_, CreateSharedChangeProcessor()). + WillOnce(MakeSharedChangeProcessor()); EXPECT_CALL(*profile_sync_factory_, CreateGenericChangeProcessor(_, _, _)). WillOnce(ReturnAndRelease(&change_processor_)); } diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index e72818d..b22406a 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -41,8 +41,8 @@ using base::JSONReader; using browser_sync::GenericChangeProcessor; +using browser_sync::SharedChangeProcessor; using browser_sync::UIDataTypeController; -using browser_sync::SyncBackendHost; using sync_api::ChangeRecord; using testing::_; using testing::Invoke; @@ -134,8 +134,10 @@ class ProfileSyncServicePreferenceTest factory, profile_.get(), service_.get()); - EXPECT_CALL(*factory, CreateGenericChangeProcessor(_, _, _)). - WillOnce(CreateAndSaveChangeProcessor(&change_processor_)); + EXPECT_CALL(*factory, CreateSharedChangeProcessor()). + WillOnce(Return(new SharedChangeProcessor())); + EXPECT_CALL(*factory, CreateGenericChangeProcessor(_, _, _)). + WillOnce(CreateAndSaveChangeProcessor(&change_processor_)); service_->RegisterDataTypeController(dtc_); TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( GaiaConstants::kSyncService, "token"); diff --git a/chrome/browser/webdata/autocomplete_syncable_service.cc b/chrome/browser/webdata/autocomplete_syncable_service.cc index af1cde1..3b3ecb1 100644 --- a/chrome/browser/webdata/autocomplete_syncable_service.cc +++ b/chrome/browser/webdata/autocomplete_syncable_service.cc @@ -79,9 +79,10 @@ AutocompleteSyncableService::AutocompleteSyncableService() SyncError AutocompleteSyncableService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { DCHECK(CalledOnValidThread()); DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); VLOG(1) << "Associating Autocomplete: MergeDataAndStartSyncing"; std::vector<AutofillEntry> entries; @@ -97,7 +98,7 @@ SyncError AutocompleteSyncableService::MergeDataAndStartSyncing( new_db_entries[it->key()] = std::make_pair(SyncChange::ACTION_ADD, it); } - sync_processor_.reset(sync_processor); + sync_processor_ = sync_processor.Pass(); std::vector<AutofillEntry> new_synced_entries; // Go through and check for all the entries that sync already knows about. diff --git a/chrome/browser/webdata/autocomplete_syncable_service.h b/chrome/browser/webdata/autocomplete_syncable_service.h index 963a1ad..0c38849 100644 --- a/chrome/browser/webdata/autocomplete_syncable_service.h +++ b/chrome/browser/webdata/autocomplete_syncable_service.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 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_WEBDATA_AUTOCOMPLETE_SYNCABLE_SERVICE_H_ @@ -50,7 +50,7 @@ class AutocompleteSyncableService 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( diff --git a/chrome/browser/webdata/autofill_profile_syncable_service.cc b/chrome/browser/webdata/autofill_profile_syncable_service.cc index f73a6618..fe1222d 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service.cc +++ b/chrome/browser/webdata/autofill_profile_syncable_service.cc @@ -59,9 +59,10 @@ AutofillProfileSyncableService::AutofillProfileSyncableService() SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, - SyncChangeProcessor* sync_processor) { + scoped_ptr<SyncChangeProcessor> sync_processor) { DCHECK(CalledOnValidThread()); DCHECK(!sync_processor_.get()); + DCHECK(sync_processor.get()); DVLOG(1) << "Associating Autofill: MergeDataAndStartSyncing"; if (!LoadAutofillData(&profiles_.get())) { @@ -84,7 +85,7 @@ SyncError AutofillProfileSyncableService::MergeDataAndStartSyncing( } } - sync_processor_.reset(sync_processor); + sync_processor_ = sync_processor.Pass(); GUIDToProfileMap remaining_profiles; CreateGUIDToProfileMap(profiles_.get(), &remaining_profiles); diff --git a/chrome/browser/webdata/autofill_profile_syncable_service.h b/chrome/browser/webdata/autofill_profile_syncable_service.h index 6926572..59c1e4a 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service.h +++ b/chrome/browser/webdata/autofill_profile_syncable_service.h @@ -51,7 +51,7 @@ class AutofillProfileSyncableService 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( diff --git a/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc b/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc index 147e2ef..11a4dc3 100644 --- a/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc +++ b/chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc @@ -169,7 +169,8 @@ TEST_F(AutofillProfileSyncableServiceTest, MergeDataAndStartSyncing) { // Takes ownership of sync_processor_. autofill_syncable_service_.MergeDataAndStartSyncing( - syncable::AUTOFILL_PROFILE, data_list, sync_processor_.release()); + syncable::AUTOFILL_PROFILE, data_list, + sync_processor_.PassAs<SyncChangeProcessor>()); autofill_syncable_service_.StopSyncing(syncable::AUTOFILL_PROFILE); } @@ -199,7 +200,8 @@ TEST_F(AutofillProfileSyncableServiceTest, GetAllSyncData) { SyncDataList data_list; // Takes ownership of sync_processor_. autofill_syncable_service_.MergeDataAndStartSyncing( - syncable::AUTOFILL_PROFILE, data_list, sync_processor_.release()); + syncable::AUTOFILL_PROFILE, data_list, + sync_processor_.PassAs<SyncChangeProcessor>()); SyncDataList data = autofill_syncable_service_.GetAllSyncData(syncable::AUTOFILL_PROFILE); diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 544fd74..7054587 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -413,8 +413,6 @@ 'sources': [ 'browser/sync/api/fake_syncable_service.cc', 'browser/sync/api/fake_syncable_service.h', - 'browser/sync/api/syncable_service_mock.cc', - 'browser/sync/api/syncable_service_mock.h', ], }, { diff --git a/tools/heapcheck/suppressions.txt b/tools/heapcheck/suppressions.txt index 903ae26b..ed6fd8c 100644 --- a/tools/heapcheck/suppressions.txt +++ b/tools/heapcheck/suppressions.txt @@ -1766,40 +1766,6 @@ fun:chromeos::DesktopNotificationsTest::SetUp } { - bug_117098_a - Heapcheck:Leak - fun:base::internal::WeakReferenceOwner::GetRef - fun:base::SupportsWeakPtr::AsWeakPtr - fun:ProfileSyncServicePreferenceTest::StartSyncService -} -{ - bug_117098_b - Heapcheck:Leak - fun:__gnu_cxx::new_allocator::allocate - fun:std::_Vector_base::_M_allocate - fun:std::vector::_M_insert_aux - fun:std::vector::push_back - fun:browser_sync::GenericChangeProcessor::ApplyChangesFromSyncModel -} -{ - bug_117098_c - Heapcheck:Leak - fun:CreateAndSaveChangeProcessorActionP::gmock_Impl::gmock_PerformImpl - fun:testing::internal::ActionHelper::Perform - fun:CreateAndSaveChangeProcessorActionP::gmock_Impl::Perform - fun:testing::Action::Perform - fun:testing::internal::ActionResultHolder::PerformAction - fun:testing::internal::FunctionMockerBase::UntypedPerformAction - fun:testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith - fun:testing::internal::FunctionMockerBase::InvokeWith - fun:testing::internal::FunctionMocker::Invoke - fun:ProfileSyncComponentsFactoryMock::CreateGenericChangeProcessor - fun:browser_sync::UIDataTypeController::Associate - fun:browser_sync::UIDataTypeController::Start - fun:browser_sync::DataTypeManagerImpl::StartNextType - fun:browser_sync::DataTypeManagerImpl::DownloadReady -} -{ bug_117389 Heapcheck:Leak ... diff --git a/tools/valgrind/drmemory/suppressions_full.txt b/tools/valgrind/drmemory/suppressions_full.txt index 8c52e7a..4856f7f 100644 --- a/tools/valgrind/drmemory/suppressions_full.txt +++ b/tools/valgrind/drmemory/suppressions_full.txt @@ -1228,31 +1228,6 @@ name=http://crbug.com/100608 *!SQLitePersistentCookieStore::Backend::LoadCookiesForDomains LEAK -name=http://crbug.com/117538 a -... -*!ProfileSyncComponentsFactoryMock::CreateGenericChangeProcessor -*!browser_sync::UIDataTypeController::Associate -*!browser_sync::UIDataTypeController::Start -*!browser_sync::DataTypeManagerImpl::StartNextType -*!browser_sync::DataTypeManagerImpl::DownloadReady - -LEAK -name=http://crbug.com/117538 b -... -*!ProfileSyncComponentsFactoryMock::CreateSharedChangeProcessor -*!browser_sync::NewNonFrontendDataTypeController::Start -*!browser_sync::DataTypeManagerImpl::StartNextType -*!browser_sync::DataTypeManagerImpl::DownloadReady - -LEAK -name=http://crbug.com/117538 c -... -*!ProfileSyncComponentsFactoryMock::CreateDataTypeManager -*!ProfileSyncService::ConfigureDataTypeManager -*!ProfileSyncService::OnBackendInitialized -*!TestProfileSyncService::OnBackendInitialized - -LEAK name=http://crbug.com/117539 *!generic_cpp_alloc *!operator new diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt index b1531aa..d177ff1 100644 --- a/tools/valgrind/memcheck/suppressions.txt +++ b/tools/valgrind/memcheck/suppressions.txt @@ -5696,25 +5696,6 @@ fun:_ZN8chromeos24DesktopNotificationsTest5SetUpEv } { - bug_117098 - Memcheck:Leak - fun:_Znw* - fun:_ZNK35CreateAndSaveChangeProcessorActionPIPPN12browser_sync22GenericChangeProcessorEE10gmock_ImplIFS2_P18ProfileSyncServicePNS0_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE17gmock_PerformImplIS7_S9_SF_N7testing8internal12ExcessiveArgESL_SL_SL_SL_SL_SL_EES2_RKNSt3tr15tupleIIS7_S9_SF_EEET_T0_T1_T2_T3_T4_T5_T6_T7_T8_ - fun:_ZN7testing8internal12ActionHelperIPN12browser_sync22GenericChangeProcessorEN35CreateAndSaveChangeProcessorActionPIPS4_E10gmock_ImplIFS4_P18ProfileSyncServicePNS2_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEEEE7PerformISA_SC_SI_EES4_PSK_RKNSt3tr15tupleIIT_T0_T1_EEE - fun:_ZN35CreateAndSaveChangeProcessorActionPIPPN12browser_sync22GenericChangeProcessorEE10gmock_ImplIFS2_P18ProfileSyncServicePNS0_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE7PerformERKNSt3tr15tupleIIS7_S9_SF_EEE - fun:_ZNK7testing6ActionIFPN12browser_sync22GenericChangeProcessorEP18ProfileSyncServicePNS1_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE7PerformERKNSt3tr15tupleIIS5_S7_SD_EEE - fun:_ZN7testing8internal18ActionResultHolderIPN12browser_sync22GenericChangeProcessorEE13PerformActionIFS4_P18ProfileSyncServicePNS2_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEEEPS5_RKNS_6ActionIT_EERKNS0_8FunctionISK_E13ArgumentTupleE - fun:_ZNK7testing8internal18FunctionMockerBaseIFPN12browser_sync22GenericChangeProcessorEP18ProfileSyncServicePNS2_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE20UntypedPerformActionEPKvSI_ - fun:_ZN7testing8internal25UntypedFunctionMockerBase17UntypedInvokeWithEPKv - fun:_ZN7testing8internal18FunctionMockerBaseIFPN12browser_sync22GenericChangeProcessorEP18ProfileSyncServicePNS2_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE10InvokeWithERKNSt3tr15tupleIIS6_S8_SE_EEE - fun:_ZN7testing8internal14FunctionMockerIFPN12browser_sync22GenericChangeProcessorEP18ProfileSyncServicePNS2_20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEEEE6InvokeES6_S8_SE_ - fun:_ZN32ProfileSyncComponentsFactoryMock28CreateGenericChangeProcessorEP18ProfileSyncServicePN12browser_sync20DataTypeErrorHandlerERKN4base7WeakPtrI15SyncableServiceEE - fun:_ZN12browser_sync20UIDataTypeController9AssociateEv - fun:_ZN12browser_sync20UIDataTypeController5StartERKN4base8CallbackIFvNS_18DataTypeController11StartResultERK9SyncErrorEEE - fun:_ZN12browser_sync19DataTypeManagerImpl13StartNextTypeEv - fun:_ZN12browser_sync19DataTypeManagerImpl13DownloadReadyENS_7EnumSetIN8syncable9ModelTypeELS3_2ELS3_16EEE -} -{ bug_117245 Memcheck:Uninitialized fun:_ZN12_GLOBAL__N_124GesturePrefsObserverAura13RegisterPrefsEP11PrefService |