diff options
17 files changed, 146 insertions, 209 deletions
diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h index 7a2d3a2..99a9c29 100644 --- a/chrome/browser/sync/glue/data_type_manager.h +++ b/chrome/browser/sync/glue/data_type_manager.h @@ -28,8 +28,6 @@ class DataTypeManager { // TODO(tim): Deprecated. Bug 26339. PAUSE_PENDING, // Waiting for the sync backend to pause. CONFIGURING, // Data types are being started. - BLOCKED, // We can't move forward with configuration because some - // external action must take place (i.e. passphrase). // TODO(tim): Deprecated. Bug 26339. RESUME_PENDING, // Waiting for the sync backend to resume. diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc index a50139e..00d56bc 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -56,8 +56,8 @@ DataTypeManagerImpl::DataTypeManagerImpl( : backend_(backend), controllers_(controllers), state_(DataTypeManager::STOPPED), + current_dtc_(NULL), pause_pending_(false), - syncer_paused_(false), ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(backend_); @@ -76,25 +76,6 @@ DataTypeManagerImpl::DataTypeManagerImpl( DataTypeManagerImpl::~DataTypeManagerImpl() { } -bool DataTypeManagerImpl::GetControllersNeedingStart( - std::vector<DataTypeController*>* needs_start) { - // Add any data type controllers into the needs_start_ list that are - // currently NOT_RUNNING or STOPPING. - bool found_any = false; - for (TypeSet::const_iterator it = last_requested_types_.begin(); - it != last_requested_types_.end(); ++it) { - DataTypeController::TypeMap::const_iterator dtc = controllers_.find(*it); - if (dtc != controllers_.end() && - (dtc->second->state() == DataTypeController::NOT_RUNNING || - dtc->second->state() == DataTypeController::STOPPING)) { - found_any = true; - if (needs_start) - needs_start->push_back(dtc->second.get()); - } - } - return found_any; -} - void DataTypeManagerImpl::Configure(const TypeSet& desired_types) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (state_ == STOPPING) { @@ -104,8 +85,19 @@ void DataTypeManagerImpl::Configure(const TypeSet& desired_types) { } last_requested_types_ = desired_types; + // Add any data type controllers into the needs_start_ list that are + // currently NOT_RUNNING or STOPPING. needs_start_.clear(); - GetControllersNeedingStart(&needs_start_); + for (TypeSet::const_iterator it = desired_types.begin(); + it != desired_types.end(); ++it) { + DataTypeController::TypeMap::const_iterator dtc = controllers_.find(*it); + if (dtc != controllers_.end() && + (dtc->second->state() == DataTypeController::NOT_RUNNING || + dtc->second->state() == DataTypeController::STOPPING)) { + needs_start_.push_back(dtc->second.get()); + VLOG(1) << "Will start " << dtc->second->name(); + } + } // Sort these according to kStartOrder. std::sort(needs_start_.begin(), needs_start_.end(), @@ -158,8 +150,8 @@ void DataTypeManagerImpl::Restart() { << " configuring."; return; } - DCHECK(state_ == STOPPED || state_ == RESTARTING || state_ == CONFIGURED || - state_ == BLOCKED); + DCHECK(state_ == STOPPED || state_ == RESTARTING || state_ == CONFIGURED); + current_dtc_ = NULL; // Starting from a "steady state" (stopped or configured) state // should send a start notification. @@ -224,12 +216,6 @@ void DataTypeManagerImpl::DownloadReady() { return; } - if (syncer_paused_) { - state_ = CONFIGURING; - StartNextType(); - return; - } - // Pause the sync backend before starting the data types. state_ = PAUSE_PENDING; PauseSyncer(); @@ -239,24 +225,16 @@ void DataTypeManagerImpl::StartNextType() { // If there are any data types left to start, start the one at the // front of the list. if (!needs_start_.empty()) { - VLOG(1) << "Starting " << needs_start_[0]->name(); - needs_start_[0]->Start( + current_dtc_ = needs_start_[0]; + VLOG(1) << "Starting " << current_dtc_->name(); + current_dtc_->Start( NewCallback(this, &DataTypeManagerImpl::TypeStartCallback)); return; } + // If no more data types need starting, we're done. Resume the sync + // backend to finish. DCHECK_EQ(state_, CONFIGURING); - - // Do a fresh calculation to see if controllers need starting to account for - // things like encryption, which may still need to be sorted out before we - // can announce we're "Done" configuration entirely. - if (GetControllersNeedingStart(NULL)) { - state_ = BLOCKED; - return; - } - - // If no more data types need starting, we're done. Resume the sync backend - // to finish. state_ = RESUME_PENDING; ResumeSyncer(); } @@ -266,6 +244,7 @@ void DataTypeManagerImpl::TypeStartCallback( // When the data type controller invokes this callback, it must be // on the UI thread. DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(current_dtc_); // If configuration changed while this data type was starting, we // need to reset. Resume the syncer. @@ -275,10 +254,11 @@ void DataTypeManagerImpl::TypeStartCallback( } // We're done with the data type at the head of the list -- remove it. - DataTypeController* started_dtc = needs_start_[0]; + DataTypeController* started_dtc = current_dtc_; DCHECK(needs_start_.size()); DCHECK_EQ(needs_start_[0], started_dtc); needs_start_.erase(needs_start_.begin()); + current_dtc_ = NULL; // If we reach this callback while stopping, this means that // DataTypeManager::Stop() was called while the current data type @@ -344,9 +324,7 @@ void DataTypeManagerImpl::Stop() { // which will synchronously invoke the start callback. if (state_ == CONFIGURING) { state_ = STOPPING; - - DCHECK_LT(0U, needs_start_.size()); - needs_start_[0]->Stop(); + current_dtc_->Stop(); // By this point, the datatype should have invoked the start callback, // triggering FinishStop to be called, and the state to reach STOPPED. If we @@ -398,8 +376,7 @@ void DataTypeManagerImpl::FinishStop() { DCHECK(state_== CONFIGURING || state_ == STOPPING || state_ == PAUSE_PENDING || - state_ == RESUME_PENDING || - state_ == BLOCKED); + state_ == RESUME_PENDING); // Simply call the Stop() method on all running data types. for (DataTypeController::TypeMap::const_iterator it = controllers_.begin(); it != controllers_.end(); ++it) { @@ -428,7 +405,7 @@ void DataTypeManagerImpl::Observe(NotificationType type, DCHECK(state_ == PAUSE_PENDING || state_ == RESTARTING); DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); pause_pending_ = false; - syncer_paused_ = true; + RemoveObserver(NotificationType::SYNC_PAUSED); // If the state changed to RESTARTING while waiting to be @@ -444,7 +421,6 @@ void DataTypeManagerImpl::Observe(NotificationType type, case NotificationType::SYNC_RESUMED: DCHECK(state_ == RESUME_PENDING || state_ == RESTARTING); RemoveObserver(NotificationType::SYNC_RESUMED); - syncer_paused_ = false; // If we are resuming because of a restart, continue the restart. if (state_ == RESTARTING) { diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h index c7f02f2..29c84d6 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.h +++ b/chrome/browser/sync/glue/data_type_manager_impl.h @@ -59,11 +59,6 @@ class DataTypeManagerImpl : public DataTypeManager, void FinishStop(); void FinishStopAndNotify(ConfigureResult result); - // Returns true if any last_requested_types_ currently needs to start model - // association. If non-null, fills |needs_start| with all such controllers. - bool GetControllersNeedingStart( - std::vector<DataTypeController*>* needs_start); - void Restart(); void DownloadReady(); void AddObserver(NotificationType type); @@ -78,6 +73,7 @@ class DataTypeManagerImpl : public DataTypeManager, // This list is determined at startup by various command line flags. const DataTypeController::TypeMap controllers_; State state_; + DataTypeController* current_dtc_; std::map<syncable::ModelType, int> start_order_; TypeSet last_requested_types_; std::vector<DataTypeController*> needs_start_; @@ -88,9 +84,6 @@ class DataTypeManagerImpl : public DataTypeManager, // PAUSE_PENDING state. See http://crbug.com/73218. bool pause_pending_; - // Whether we've observed a SYNC_PAUSED but not SYNC_RESUMED. - bool syncer_paused_; - NotificationRegistrar notification_registrar_; ScopedRunnableMethodFactory<DataTypeManagerImpl> method_factory_; diff --git a/chrome/browser/sync/glue/data_type_manager_impl2.cc b/chrome/browser/sync/glue/data_type_manager_impl2.cc index e66e421..74a3d42 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl2.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl2.cc @@ -59,6 +59,7 @@ DataTypeManagerImpl2::DataTypeManagerImpl2(SyncBackendHost* backend, : backend_(backend), controllers_(controllers), state_(DataTypeManager::STOPPED), + current_dtc_(NULL), method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { DCHECK(backend_); // Ensure all data type controllers are stopped. @@ -74,25 +75,6 @@ DataTypeManagerImpl2::DataTypeManagerImpl2(SyncBackendHost* backend, DataTypeManagerImpl2::~DataTypeManagerImpl2() {} -bool DataTypeManagerImpl2::GetControllersNeedingStart( - std::vector<DataTypeController*>* needs_start) { - // Add any data type controllers into the needs_start_ list that are - // currently NOT_RUNNING or STOPPING. - bool found_any = false; - for (TypeSet::const_iterator it = last_requested_types_.begin(); - it != last_requested_types_.end(); ++it) { - DataTypeController::TypeMap::const_iterator dtc = controllers_.find(*it); - if (dtc != controllers_.end() && - (dtc->second->state() == DataTypeController::NOT_RUNNING || - dtc->second->state() == DataTypeController::STOPPING)) { - found_any = true; - if (needs_start) - needs_start->push_back(dtc->second.get()); - } - } - return found_any; -} - void DataTypeManagerImpl2::Configure(const TypeSet& desired_types) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (state_ == STOPPING) { @@ -102,8 +84,19 @@ void DataTypeManagerImpl2::Configure(const TypeSet& desired_types) { } last_requested_types_ = desired_types; + // Add any data type controllers into the needs_start_ list that are + // currently NOT_RUNNING or STOPPING. needs_start_.clear(); - GetControllersNeedingStart(&needs_start_); + for (TypeSet::const_iterator it = desired_types.begin(); + it != desired_types.end(); ++it) { + DataTypeController::TypeMap::const_iterator dtc = controllers_.find(*it); + if (dtc != controllers_.end() && + (dtc->second->state() == DataTypeController::NOT_RUNNING || + dtc->second->state() == DataTypeController::STOPPING)) { + needs_start_.push_back(dtc->second.get()); + VLOG(1) << "Will start " << dtc->second->name(); + } + } // Sort these according to kStartOrder. std::sort(needs_start_.begin(), needs_start_.end(), @@ -150,8 +143,8 @@ void DataTypeManagerImpl2::Restart() { return; } - DCHECK(state_ == STOPPED || state_ == RESTARTING || state_ == CONFIGURED || - state_ == BLOCKED); + DCHECK(state_ == STOPPED || state_ == RESTARTING || state_ == CONFIGURED); + current_dtc_ = NULL; // Starting from a "steady state" (stopped or configured) state // should send a start notification. @@ -194,23 +187,15 @@ void DataTypeManagerImpl2::StartNextType() { // If there are any data types left to start, start the one at the // front of the list. if (!needs_start_.empty()) { - VLOG(1) << "Starting " << needs_start_[0]->name(); - needs_start_[0]->Start( + current_dtc_ = needs_start_[0]; + VLOG(1) << "Starting " << current_dtc_->name(); + current_dtc_->Start( NewCallback(this, &DataTypeManagerImpl2::TypeStartCallback)); return; } - DCHECK_EQ(state_, CONFIGURING); - - // Do a fresh calculation to see if controllers need starting to account for - // things like encryption, which may still need to be sorted out before we - // can announce we're "Done" configuration entirely. - if (GetControllersNeedingStart(NULL)) { - state_ = BLOCKED; - return; - } - // If no more data types need starting, we're done. + DCHECK_EQ(state_, CONFIGURING); state_ = CONFIGURED; NotifyDone(OK); } @@ -220,6 +205,7 @@ void DataTypeManagerImpl2::TypeStartCallback( // When the data type controller invokes this callback, it must be // on the UI thread. DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(current_dtc_); if (state_ == RESTARTING) { // If configuration changed while this data type was starting, we @@ -241,10 +227,11 @@ void DataTypeManagerImpl2::TypeStartCallback( } // We're done with the data type at the head of the list -- remove it. - DataTypeController* started_dtc = needs_start_[0]; + DataTypeController* started_dtc = current_dtc_; DCHECK(needs_start_.size()); DCHECK_EQ(needs_start_[0], started_dtc); needs_start_.erase(needs_start_.begin()); + current_dtc_ = NULL; // If the type started normally, continue to the next type. // If the type is waiting for the cryptographer, continue to the next type. @@ -288,9 +275,7 @@ void DataTypeManagerImpl2::Stop() { // which will synchronously invoke the start callback. if (state_ == CONFIGURING) { state_ = STOPPING; - - DCHECK_LT(0U, needs_start_.size()); - needs_start_[0]->Stop(); + current_dtc_->Stop(); // By this point, the datatype should have invoked the start callback, // triggering FinishStop to be called, and the state to reach STOPPED. If we @@ -314,7 +299,7 @@ void DataTypeManagerImpl2::Stop() { } void DataTypeManagerImpl2::FinishStop() { - DCHECK(state_== CONFIGURING || state_ == STOPPING || state_ == BLOCKED); + DCHECK(state_== CONFIGURING || state_ == STOPPING); // Simply call the Stop() method on all running data types. for (DataTypeController::TypeMap::const_iterator it = controllers_.begin(); it != controllers_.end(); ++it) { diff --git a/chrome/browser/sync/glue/data_type_manager_impl2.h b/chrome/browser/sync/glue/data_type_manager_impl2.h index f7dcca2..570a38b 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl2.h +++ b/chrome/browser/sync/glue/data_type_manager_impl2.h @@ -44,11 +44,6 @@ class DataTypeManagerImpl2 : public DataTypeManager { void FinishStop(); void FinishStopAndNotify(ConfigureResult result); - // Returns true if any last_requested_types_ currently needs to start model - // association. If non-null, fills |needs_start| with all such controllers. - bool GetControllersNeedingStart( - std::vector<DataTypeController*>* needs_start); - void Restart(); void DownloadReady(); void NotifyStart(); @@ -59,6 +54,7 @@ class DataTypeManagerImpl2 : public DataTypeManager { // This list is determined at startup by various command line flags. const DataTypeController::TypeMap controllers_; State state_; + DataTypeController* current_dtc_; std::map<syncable::ModelType, int> start_order_; TypeSet last_requested_types_; std::vector<DataTypeController*> needs_start_; diff --git a/chrome/browser/sync/glue/data_type_manager_impl2_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl2_unittest.cc index 0801be9..31c302c 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl2_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl2_unittest.cc @@ -33,7 +33,6 @@ using testing::_; using testing::DoAll; using testing::DoDefault; using testing::InSequence; -using testing::Mock; using testing::Property; using testing::Pointee; using testing::Return; @@ -207,12 +206,12 @@ TEST_F(DataTypeManagerImpl2Test, ConfigureOneStopWhileAssociating) { TEST_F(DataTypeManagerImpl2Test, OneWaitingForCrypto) { DataTypeControllerMock* password_dtc = MakePasswordDTC(); - EXPECT_CALL(*password_dtc, state()).Times(3). - WillRepeatedly(Return(DataTypeController::NOT_RUNNING)). - RetiresOnSaturation(); + EXPECT_CALL(*password_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); EXPECT_CALL(*password_dtc, Start(_)). - WillOnce(InvokeCallback((DataTypeController::NEEDS_CRYPTO))). - RetiresOnSaturation(); + WillOnce(InvokeCallback((DataTypeController::NEEDS_CRYPTO))); + EXPECT_CALL(*password_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); controllers_[syncable::PASSWORDS] = password_dtc; EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _)).Times(1); @@ -220,24 +219,9 @@ TEST_F(DataTypeManagerImpl2Test, OneWaitingForCrypto) { DataTypeManagerImpl2 dtm(&backend_, controllers_); types_.insert(syncable::PASSWORDS); SetConfigureStartExpectation(); - - dtm.Configure(types_); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm.state()); - - Mock::VerifyAndClearExpectations(&backend_); - Mock::VerifyAndClearExpectations(&observer_); - SetConfigureDoneExpectation(DataTypeManager::OK); - EXPECT_CALL(*password_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillRepeatedly(Return(DataTypeController::RUNNING)); - EXPECT_CALL(*password_dtc, Start(_)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _)).Times(1); dtm.Configure(types_); - EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); - EXPECT_CALL(*password_dtc, Stop()).Times(1); dtm.Stop(); EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); } diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc index 0ca32be..dfb0881 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -32,7 +32,6 @@ using testing::_; using testing::DoAll; using testing::DoDefault; using testing::InSequence; -using testing::Mock; using testing::Property; using testing::Pointee; using testing::Return; @@ -213,42 +212,23 @@ TEST_F(DataTypeManagerImplTest, ConfigureOneStopWhileAssociating) { TEST_F(DataTypeManagerImplTest, OneWaitingForCrypto) { DataTypeControllerMock* password_dtc = MakePasswordDTC(); - EXPECT_CALL(*password_dtc, state()).Times(3). - WillRepeatedly(Return(DataTypeController::NOT_RUNNING)). - RetiresOnSaturation(); + EXPECT_CALL(*password_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); EXPECT_CALL(*password_dtc, Start(_)). - WillOnce(InvokeCallback((DataTypeController::NEEDS_CRYPTO))). - RetiresOnSaturation(); + WillOnce(InvokeCallback((DataTypeController::NEEDS_CRYPTO))); + EXPECT_CALL(*password_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); controllers_[syncable::PASSWORDS] = password_dtc; - EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _)).Times(1); - EXPECT_CALL(backend_, StartSyncingWithServer()).Times(1); - EXPECT_CALL(backend_, RequestPause()).Times(1); + SetBackendExpectations(1); DataTypeManagerImpl dtm(&backend_, controllers_); types_.insert(syncable::PASSWORDS); SetConfigureStartExpectation(); - dtm.Configure(types_); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm.state()); - - Mock::VerifyAndClearExpectations(&backend_); - Mock::VerifyAndClearExpectations(&observer_); - SetConfigureDoneExpectation(DataTypeManager::OK); - EXPECT_CALL(*password_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillRepeatedly(Return(DataTypeController::RUNNING)); - EXPECT_CALL(*password_dtc, Start(_)). - WillOnce(InvokeCallback((DataTypeController::OK))); - - EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _)).Times(1); - EXPECT_CALL(backend_, StartSyncingWithServer()).Times(1); - EXPECT_CALL(backend_, RequestResume()).Times(1); dtm.Configure(types_); EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); - EXPECT_CALL(*password_dtc, Stop()).Times(1); dtm.Stop(); - EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); } diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index f951c28..76a8637 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -417,14 +417,15 @@ void SyncBackendHost::ScheduleSyncEventForConfigChange(bool deleted_type, // We can only nudge when we've either deleted a dataype or added one, else // we can cause unnecessary syncs. Unit tests cover this. if (using_new_syncer_thread_) { - if (added_types.size() > 0) + if (added_types.size() > 0) { RequestConfig(added_types); - - // TODO(tim): Bug 76233. Fix this once only one impl exists. - if (deleted_type) { - core_thread_.message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(core_.get(), - &SyncBackendHost::Core::DeferNudgeForCleanup)); + // TODO(tim): If we've added and deleted types, because we don't want to + // nudge until association finishes, circumstances of bug 56416 exist. We + // may need a way to nudge only for data type cleanup. Alternatively, we + // can chain configure_ready_task_ by appending a task to nudge in this + // case. + } else if (deleted_type) { + RequestNudge(); } } else if (deleted_type || !core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) { @@ -480,6 +481,9 @@ void SyncBackendHost::DeactivateDataType( std::map<syncable::ModelType, ChangeProcessor*>::size_type erased = processors_.erase(data_type_controller->type()); DCHECK_EQ(erased, 1U); + + // TODO(sync): At this point we need to purge the data associated + // with this data type from the sync db. } bool SyncBackendHost::RequestPause() { @@ -727,9 +731,6 @@ void SyncBackendHost::Core::DoUpdateEnabledTypes() { void SyncBackendHost::Core::DoStartSyncing() { DCHECK(MessageLoop::current() == host_->core_thread_.message_loop()); syncapi_->StartSyncing(); - if (deferred_nudge_for_cleanup_requested_) - syncapi_->RequestNudge(); - deferred_nudge_for_cleanup_requested_ = false; } void SyncBackendHost::Core::DoSetPassphrase(const std::string& passphrase, @@ -1131,9 +1132,4 @@ void SyncBackendHost::Core::DoProcessMessage( syncapi_->GetJsBackend()->ProcessMessage(name, args, sender); } -void SyncBackendHost::Core::DeferNudgeForCleanup() { - DCHECK_EQ(MessageLoop::current(), host_->core_thread_.message_loop()); - deferred_nudge_for_cleanup_requested_ = true; -} - } // namespace browser_sync diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 42d6ef4..978da4b 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -341,9 +341,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { void DoRequestResume(); void DoRequestClearServerData(); - // Sets |deferred_nudge_for_cleanup_requested_| to true. See comment below. - void DeferNudgeForCleanup(); - // Called on our SyncBackendHost's |core_thread_| to set the passphrase // on behalf of SyncBackendHost::SupplyPassphrase. void DoSetPassphrase(const std::string& passphrase, bool is_explicit); @@ -504,10 +501,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // modified from within the frontend_loop_ (UI thread). bool processing_passphrase_; - // True when a datatype has been disabled so that we nudge once sync is - // resumed (after configuration is finished). - bool deferred_nudge_for_cleanup_requested_; - DISALLOW_COPY_AND_ASSIGN(Core); }; diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc index 0c1fc7e..d8db395 100644 --- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc @@ -311,6 +311,7 @@ class ProfileSyncServiceAutofillTest : public AbstractProfileSyncServiceTest { EXPECT_CALL(profile_, GetTokenService()). WillRepeatedly(Return(&token_service_)); + service_->set_num_expected_resumes(will_fail_association ? 0 : 1); service_->RegisterDataTypeController(data_type_controller); service_->Initialize(); MessageLoop::current()->Run(); diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc index 2db29d6..0856f5a 100644 --- a/chrome/browser/sync/profile_sync_service_password_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc @@ -115,12 +115,17 @@ class PasswordTestProfileSyncService : public TestProfileSyncService { virtual ~PasswordTestProfileSyncService() {} + virtual void OnPassphraseRequired(bool for_decryption) { + TestProfileSyncService::OnPassphraseRequired(for_decryption); + ADD_FAILURE(); + } + virtual void OnPassphraseAccepted() { + TestProfileSyncService::OnPassphraseAccepted(); + if (passphrase_accept_task_) { passphrase_accept_task_->Run(); } - - TestProfileSyncService::OnPassphraseAccepted(); } private: @@ -152,11 +157,19 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { } void StartSyncService(Task* root_task, Task* node_task) { + StartSyncService(root_task, node_task, 2, 2); + } + + void StartSyncService(Task* root_task, Task* node_task, + int num_resume_expectations, + int num_pause_expectations) { if (!service_.get()) { service_.reset(new PasswordTestProfileSyncService( &factory_, &profile_, "test_user", false, root_task, node_task)); service_->RegisterPreferences(); profile_.GetPrefs()->SetBoolean(prefs::kSyncPasswords, true); + service_->set_num_expected_resumes(num_resume_expectations); + service_->set_num_expected_pauses(num_pause_expectations); PasswordDataTypeController* data_type_controller = new PasswordDataTypeController(&factory_, &profile_, @@ -185,9 +198,16 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { NotificationType(NotificationType::SYNC_CONFIGURE_DONE),_,_)); service_->RegisterDataTypeController(data_type_controller); - service_->SetPassphrase("foo", false, true); service_->Initialize(); + MessageLoop::current()->Run(); + EXPECT_CALL( + observer_, + Observe( + NotificationType(NotificationType::SYNC_CONFIGURE_DONE), + _,_)). + WillOnce(QuitUIMessageLoop()); + service_->SetPassphrase("foo", false, true); MessageLoop::current()->Run(); } } @@ -278,7 +298,7 @@ class AddPasswordEntriesTask : public Task { }; TEST_F(ProfileSyncServicePasswordTest, FailModelAssociation) { - StartSyncService(NULL, NULL); + StartSyncService(NULL, NULL, 1, 2); EXPECT_TRUE(service_->unrecoverable_error_detected()); } diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index f5c170e..8c44e17a 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -83,6 +83,8 @@ class ProfileSyncServicePreferenceTest EXPECT_CALL(factory_, CreateDataTypeManager(_, _)). WillOnce(ReturnNewDataTypeManager()); + service_->set_num_expected_resumes(will_fail_association ? 0 : 1); + service_->RegisterDataTypeController( new PreferenceDataTypeController(&factory_, service_.get())); diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc index 6fa7d62..558debf 100644 --- a/chrome/browser/sync/profile_sync_service_session_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc @@ -115,6 +115,7 @@ class ProfileSyncServiceSessionTest model_associator_, change_processor_))); EXPECT_CALL(factory_, CreateDataTypeManager(_, _)). WillOnce(ReturnNewDataTypeManager()); + sync_service_->set_num_expected_resumes(will_fail_association ? 0 : 1); sync_service_->RegisterDataTypeController( new SessionDataTypeController(&factory_, sync_service_.get())); profile()->GetTokenService()->IssueAuthTokenForTest( diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index f9ab3ff..d5d6e71 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -24,7 +24,6 @@ using browser_sync::DataTypeManager; using browser_sync::DataTypeManagerMock; using testing::_; -using testing::AnyNumber; using testing::DoAll; using testing::InvokeArgument; using testing::Mock; @@ -59,6 +58,8 @@ class ProfileSyncServiceStartupTest : public testing::Test { service_.reset(new TestProfileSyncService(&factory_, &profile_, "test", true, NULL)); service_->AddObserver(&observer_); + service_->set_num_expected_resumes(0); + service_->set_num_expected_pauses(0); service_->set_synchronous_sync_configuration(); } @@ -91,7 +92,7 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFirstTime)) { // Should not actually start, rather just clean things up and wait // to be enabled. - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->Initialize(); // Preferences should be back to defaults. @@ -100,11 +101,11 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFirstTime)) { Mock::VerifyAndClearExpectations(data_type_manager); // Then start things up. - EXPECT_CALL(*data_type_manager, Configure(_)).Times(3); + EXPECT_CALL(*data_type_manager, Configure(_)).Times(2); EXPECT_CALL(*data_type_manager, state()). WillOnce(Return(DataTypeManager::CONFIGURED)); EXPECT_CALL(*data_type_manager, Stop()).Times(1); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(5); // Create some tokens in the token service; the service will startup when // it is notified that tokens are available. @@ -118,12 +119,12 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFirstTime)) { TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartNormal)) { DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); - EXPECT_CALL(*data_type_manager, Configure(_)).Times(2); + EXPECT_CALL(*data_type_manager, Configure(_)).Times(1); EXPECT_CALL(*data_type_manager, state()). WillOnce(Return(DataTypeManager::CONFIGURED)); EXPECT_CALL(*data_type_manager, Stop()).Times(1); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(3); // Pre load the tokens profile_.GetTokenService()->IssueAuthTokenForTest( @@ -136,7 +137,7 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(ManagedStartup)) { profile_.GetPrefs()->SetBoolean(prefs::kSyncManaged, true); EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).Times(0); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); // Service should not be started by Initialize() since it's managed. profile_.GetTokenService()->IssueAuthTokenForTest( @@ -146,8 +147,8 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(ManagedStartup)) { TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(SwitchManaged)) { DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); - EXPECT_CALL(*data_type_manager, Configure(_)).Times(2); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(*data_type_manager, Configure(_)).Times(1); + EXPECT_CALL(observer_, OnStateChanged()).Times(3); profile_.GetTokenService()->IssueAuthTokenForTest( GaiaConstants::kSyncService, "sync_token"); @@ -158,21 +159,21 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(SwitchManaged)) { EXPECT_CALL(*data_type_manager, state()). WillOnce(Return(DataTypeManager::CONFIGURED)); EXPECT_CALL(*data_type_manager, Stop()).Times(1); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(2); profile_.GetPrefs()->SetBoolean(prefs::kSyncManaged, true); // When switching back to unmanaged, the state should change, but the service // should not start up automatically (kSyncSetupCompleted will be false). Mock::VerifyAndClearExpectations(data_type_manager); EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).Times(0); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); profile_.GetPrefs()->ClearPref(prefs::kSyncManaged); } TEST_F(ProfileSyncServiceStartupTest, ClearServerData) { DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); - EXPECT_CALL(*data_type_manager, Configure(_)).Times(2); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(*data_type_manager, Configure(_)).Times(1); + EXPECT_CALL(observer_, OnStateChanged()).Times(3); profile_.GetTokenService()->IssueAuthTokenForTest( GaiaConstants::kSyncService, "sync_token"); @@ -186,12 +187,12 @@ TEST_F(ProfileSyncServiceStartupTest, ClearServerData) { EXPECT_TRUE(ProfileSyncService::CLEAR_NOT_STARTED == service_->GetClearServerDataState()); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->OnClearServerDataFailed(); EXPECT_TRUE(ProfileSyncService::CLEAR_FAILED == service_->GetClearServerDataState()); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->OnClearServerDataSucceeded(); EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED == service_->GetClearServerDataState()); @@ -205,12 +206,12 @@ TEST_F(ProfileSyncServiceStartupTest, ClearServerData) { EXPECT_TRUE(ProfileSyncService::CLEAR_NOT_STARTED == service_->GetClearServerDataState()); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->OnClearServerDataTimeout(); EXPECT_TRUE(ProfileSyncService::CLEAR_FAILED == service_->GetClearServerDataState()); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->OnClearServerDataSucceeded(); EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED == service_->GetClearServerDataState()); @@ -229,7 +230,7 @@ TEST_F(ProfileSyncServiceStartupTest, ClearServerData) { service_->GetClearServerDataState()); // Stop the timer and reset the state - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->OnClearServerDataSucceeded(); service_->ResetClearServerDataState(); } @@ -239,7 +240,7 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFailure)) { DataTypeManager::ConfigureResult result = DataTypeManager::ASSOCIATION_FAILED; EXPECT_CALL(*data_type_manager, Configure(_)). - WillRepeatedly(DoAll(NotifyFromDataTypeManager(data_type_manager, + WillOnce(DoAll(NotifyFromDataTypeManager(data_type_manager, NotificationType::SYNC_CONFIGURE_START), NotifyFromDataTypeManagerWithResult(data_type_manager, NotificationType::SYNC_CONFIGURE_DONE, @@ -247,7 +248,7 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFailure)) { EXPECT_CALL(*data_type_manager, state()). WillOnce(Return(DataTypeManager::STOPPED)); - EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); + EXPECT_CALL(observer_, OnStateChanged()).Times(3); profile_.GetTokenService()->IssueAuthTokenForTest( GaiaConstants::kSyncService, "sync_token"); diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index 83c7209..7c30844 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -55,7 +55,6 @@ using browser_sync::SyncBackendHost; using browser_sync::SyncBackendHostMock; using browser_sync::UnrecoverableErrorHandler; using testing::_; -using testing::AtLeast; using testing::AtMost; using testing::Return; using testing::StrictMock; @@ -561,6 +560,7 @@ TEST_F(ProfileSyncServiceTest, DisabledByPolicy) { TEST_F(ProfileSyncServiceTest, AbortedByShutdown) { service_.reset(new TestProfileSyncService(&factory_, profile_.get(), "test", true, NULL)); + service_->set_num_expected_resumes(0); EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).Times(0); EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_, _)).Times(0); service_->RegisterDataTypeController( @@ -990,7 +990,7 @@ TEST_F(ProfileSyncServiceTest, StrictMock<MockJsEventHandler> event_handler; EXPECT_CALL(event_handler, HandleJsEvent("onSyncServiceStateChanged", - HasArgs(JsArgList()))).Times(AtLeast(3)); + HasArgs(JsArgList()))).Times(3); // For some reason, these two events don't fire on Linux. EXPECT_CALL(event_handler, HandleJsEvent("onChangesApplied", _)) .Times(AtMost(1)); @@ -1104,7 +1104,7 @@ TEST_F(ProfileSyncServiceTest, const JsArgList kNoArgs; EXPECT_CALL(event_handler, HandleJsEvent("onSyncServiceStateChanged", - HasArgs(kNoArgs))).Times(AtLeast(3)); + HasArgs(kNoArgs))).Times(3); browser_sync::JsFrontend* js_backend = service_->GetJsFrontend(); diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 2805081..ed4668e 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -36,6 +36,8 @@ namespace browser_sync { SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest( Profile* profile, + int num_expected_resumes, + int num_expected_pauses, bool set_initial_sync_ended_on_init, bool synchronous_init) : browser_sync::SyncBackendHost(profile), @@ -53,8 +55,8 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest( &SyncBackendHostForProfileSyncTest:: SimulateSyncCycleCompletedInitialSyncEnded)); - EXPECT_CALL(*this, RequestPause()).Times(testing::AnyNumber()); - EXPECT_CALL(*this, RequestResume()).Times(testing::AnyNumber()); + EXPECT_CALL(*this, RequestPause()).Times(num_expected_pauses); + EXPECT_CALL(*this, RequestResume()).Times(num_expected_resumes); EXPECT_CALL(*this, RequestNudge()).Times(set_initial_sync_ended_on_init ? 0 : 1); } @@ -179,6 +181,8 @@ TestProfileSyncService::TestProfileSyncService( synchronous_backend_initialization_( synchronous_backend_initialization), synchronous_sync_configuration_(false), + num_expected_resumes_(1), + num_expected_pauses_(1), initial_condition_setup_task_(initial_condition_setup_task), set_initial_sync_ended_on_init_(true) { RegisterPreferences(); @@ -215,7 +219,6 @@ void TestProfileSyncService::OnBackendInitialized() { // Pretend we downloaded initial updates and set initial sync ended bits // if we were asked to. - bool send_passphrase_required = false; if (set_initial_sync_ended_on_init_) { UserShare* user_share = GetUserShare(); DirectoryManager* dir_manager = user_share->dir_manager.get(); @@ -228,18 +231,12 @@ void TestProfileSyncService::OnBackendInitialized() { ProfileSyncServiceTestHelper::CreateRoot( syncable::NIGORI, GetUserShare(), id_factory()); - - // A side effect of adding the NIGORI mode (normally done by the syncer) - // is a decryption attempt, which will fail the first time. - send_passphrase_required = true; } SetInitialSyncEndedForEnabledTypes(); } ProfileSyncService::OnBackendInitialized(); - if (send_passphrase_required) - OnPassphraseRequired(true); // TODO(akalin): Figure out a better way to do this. if (synchronous_backend_initialization_) { @@ -257,6 +254,12 @@ void TestProfileSyncService::Observe(NotificationType type, } } +void TestProfileSyncService::set_num_expected_resumes(int times) { + num_expected_resumes_ = times; +} +void TestProfileSyncService::set_num_expected_pauses(int num) { + num_expected_pauses_ = num; +} void TestProfileSyncService::dont_set_initial_sync_ended_on_init() { set_initial_sync_ended_on_init_ = false; } @@ -267,6 +270,7 @@ void TestProfileSyncService::set_synchronous_sync_configuration() { void TestProfileSyncService::CreateBackend() { backend_.reset(new browser_sync::SyncBackendHostForProfileSyncTest( profile(), + num_expected_resumes_, num_expected_pauses_, set_initial_sync_ended_on_init_, synchronous_backend_initialization_)); } diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index 4c01473..c027725 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -36,6 +36,8 @@ class SyncBackendHostForProfileSyncTest // completed setting itself up and called us back. SyncBackendHostForProfileSyncTest( Profile* profile, + int num_expected_resumes, + int num_expected_pauses, bool set_initial_sync_ended_on_init, bool synchronous_init); virtual ~SyncBackendHostForProfileSyncTest(); @@ -99,6 +101,9 @@ class TestProfileSyncService : public ProfileSyncService { const NotificationSource& source, const NotificationDetails& details); + void set_num_expected_resumes(int times); + void set_num_expected_pauses(int num); + // If this is called, configuring data types will require a syncer // nudge. void dont_set_initial_sync_ended_on_init(); @@ -128,6 +133,8 @@ class TestProfileSyncService : public ProfileSyncService { // step is performed synchronously. bool synchronous_sync_configuration_; bool set_expect_resume_expectations_; + int num_expected_resumes_; + int num_expected_pauses_; Task* initial_condition_setup_task_; bool set_initial_sync_ended_on_init_; |