diff options
27 files changed, 773 insertions, 328 deletions
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.h b/chrome/browser/sync/glue/bookmark_data_type_controller.h index f0719de..49a3b94 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller.h +++ b/chrome/browser/sync/glue/bookmark_data_type_controller.h @@ -50,6 +50,11 @@ class BookmarkDataTypeController : public DataTypeController, return browser_sync::GROUP_UI; } + virtual const char* name() const { + // For logging only. + return "bookmark"; + } + virtual State state() { return state_; } diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc index 7c4f6d6..88c69b8 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc @@ -20,6 +20,7 @@ #include "chrome/common/notification_source.h" #include "chrome/common/notification_type.h" #include "chrome/test/testing_profile.h" +#include "testing/gmock/include/gmock/gmock.h" using browser_sync::BookmarkDataTypeController; using browser_sync::ChangeProcessorMock; diff --git a/chrome/browser/sync/glue/data_type_controller.h b/chrome/browser/sync/glue/data_type_controller.h index 343dee9..9103813 100644 --- a/chrome/browser/sync/glue/data_type_controller.h +++ b/chrome/browser/sync/glue/data_type_controller.h @@ -71,6 +71,9 @@ class DataTypeController { // Unique model type for this data type controller. virtual syncable::ModelType type() = 0; + // Name of this data type. For logging purposes only. + virtual const char* name() const = 0; + // The model safe group of this data type. This should reflect the // thread that should be used to modify the data type's native // model. diff --git a/chrome/browser/sync/glue/data_type_controller_mock.h b/chrome/browser/sync/glue/data_type_controller_mock.h index c2fb102..71b3bc0 100644 --- a/chrome/browser/sync/glue/data_type_controller_mock.h +++ b/chrome/browser/sync/glue/data_type_controller_mock.h @@ -16,6 +16,7 @@ class DataTypeControllerMock : public DataTypeController { MOCK_METHOD0(Stop, void()); MOCK_METHOD0(enabled, bool()); MOCK_METHOD0(type, syncable::ModelType()); + MOCK_CONST_METHOD0(name, const char*()); MOCK_METHOD0(model_safe_group, browser_sync::ModelSafeGroup()); MOCK_METHOD0(state, State()); }; diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h new file mode 100644 index 0000000..5701acd --- /dev/null +++ b/chrome/browser/sync/glue/data_type_manager.h @@ -0,0 +1,67 @@ +// Copyright (c) 2010 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_GLUE_DATA_TYPE_MANAGER_H__ +#define CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_H__ + +#include "base/task.h" +#include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/syncable/model_type.h" + +namespace browser_sync { + +// This interface is for managing the start up and shut down life cycle +// of many different syncable data types. +class DataTypeManager { + public: + enum State { + STOPPED, // No data types are currently running. + STARTING, // Data types are being started. + STARTED, // All enabled data types are running. + STOPPING // Data types are being stopped. + }; + + enum StartResult { + OK, // All enabled data types were started normally. + BUSY, // Start() was called while start is already + // in progress. + ASSOCIATION_FAILED, // An error occurred during model association. + ABORTED // Start was aborted by calling Stop() before + // all types were started. + }; + + typedef Callback1<StartResult>::Type StartCallback; + + virtual ~DataTypeManager() {} + + // Begins asynchronous start up of all registered data types. Upon + // successful completion or failure, the start_callback will be + // invoked with the corresponding StartResult. This is an all or + // nothing process -- if any of the registered data types fail to + // start, the startup process will halt and any data types that have + // been started will be stopped. + virtual void Start(StartCallback* start_callback) = 0; + + // Synchronously stops all registered data types. If called after + // Start() is called but before it finishes, it will abort the start + // and any data types that have been started will be stopped. + virtual void Stop() = 0; + + // Returns true if the given data type has been registered. + virtual bool IsRegistered(syncable::ModelType type) = 0; + + // Returns true if the given data type has been registered and is + // enabled. + virtual bool IsEnabled(syncable::ModelType type) = 0; + + // Reference to map of data type controllers. + virtual const DataTypeController::TypeMap& controllers() = 0; + + // The current state of the data type manager. + virtual State state() = 0; +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_H__ diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc new file mode 100644 index 0000000..322c37f --- /dev/null +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -0,0 +1,163 @@ +// Copyright (c) 2010 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 "base/logging.h" +#include "base/task.h" +#include "chrome/browser/chrome_thread.h" +#include "chrome/browser/sync/glue/data_type_manager_impl.h" + +static const int kStartOrderStart = -1; + +namespace { + +static const syncable::ModelType kStartOrder[] = { + syncable::BOOKMARKS, + syncable::PREFERENCES +}; + +} // namespace + +namespace browser_sync { + +DataTypeManagerImpl::DataTypeManagerImpl( + const DataTypeController::TypeMap& controllers) + : controllers_(controllers), + state_(DataTypeManager::STOPPED), + current_type_(kStartOrderStart) { + DCHECK(arraysize(kStartOrder) > 0); + // Ensure all data type controllers are stopped. + for (DataTypeController::TypeMap::const_iterator it = controllers_.begin(); + it != controllers_.end(); ++it) { + DCHECK_EQ(DataTypeController::NOT_RUNNING, (*it).second->state()); + } +} + +void DataTypeManagerImpl::Start(StartCallback* start_callback) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + if (state_ != STOPPED) { + start_callback->Run(BUSY); + delete start_callback; + return; + } + + state_ = STARTING; + start_callback_.reset(start_callback); + current_type_ = kStartOrderStart; + StartNextType(); +} + +void DataTypeManagerImpl::StartNextType() { + // Ensure that the current type has indeed started. + DCHECK(current_type_ == kStartOrderStart || + controllers_[kStartOrder[current_type_]]->state() == + DataTypeController::RUNNING); + + // Find the next startable type. + while (current_type_ < static_cast<int>(arraysize(kStartOrder)) - 1) { + current_type_++; + syncable::ModelType type = kStartOrder[current_type_]; + if (IsEnabled(type)) { + LOG(INFO) << "Starting " << controllers_[type]->name(); + controllers_[type]->Start( + true, + NewCallback(this, &DataTypeManagerImpl::TypeStartCallback)); + return; + } + } + + // No more startable types found, we must be done. + DCHECK_EQ(state_, STARTING); + state_ = STARTED; + start_callback_->Run(OK); + start_callback_.reset(); +} + +void DataTypeManagerImpl::TypeStartCallback( + DataTypeController::StartResult result) { + // When the data type controller invokes this callback, it must be + // on the UI thread. + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + + // If we reach this callback while stopping, this means that the + // current data type was stopped while still starting up. Now that + // the data type is aborted, we can finish stop. + if (state_ == STOPPING) { + FinishStop(); + start_callback_->Run(DataTypeManager::ABORTED); + start_callback_.reset(); + return; + } + + // If the type started normally, continue to the next type. + syncable::ModelType type = kStartOrder[current_type_]; + if (result == DataTypeController::OK || + result == DataTypeController::OK_FIRST_RUN) { + LOG(INFO) << "Started " << controllers_[type]->name(); + StartNextType(); + return; + } + + // Any other result is a fatal error. Shut down any types we've + // managed to start up to this point and pass the result to the + // callback. + LOG(INFO) << "Failed " << controllers_[type]->name(); + FinishStop(); + StartResult start_result = DataTypeManager::ABORTED; + switch(result) { + case DataTypeController::ABORTED: + start_result = DataTypeManager::ABORTED; + break; + case DataTypeController::ASSOCIATION_FAILED: + start_result = DataTypeManager::ASSOCIATION_FAILED; + break; + default: + NOTREACHED(); + break; + } + start_callback_->Run(start_result); + start_callback_.reset(); +} + +void DataTypeManagerImpl::Stop() { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + if (state_ == STOPPED) + return; + + // If we are currently starting, then the current type is in a + // partially started state. Abort the startup of the current type + // and continue shutdown when the abort completes. + if (state_ == STARTING) { + state_ = STOPPING; + syncable::ModelType type = kStartOrder[current_type_]; + controllers_[type]->Stop(); + return; + } + + state_ = STOPPING; + FinishStop(); +} + +void DataTypeManagerImpl::FinishStop() { + DCHECK(state_== STARTING || state_ == STOPPING); + // Simply call the Stop() method on all running data types. + for (unsigned int i = 0; i < arraysize(kStartOrder); ++i) { + syncable::ModelType type = kStartOrder[i]; + if (IsRegistered(type) && + controllers_[type]->state() == DataTypeController::RUNNING) { + controllers_[type]->Stop(); + LOG(INFO) << "Stopped " << controllers_[type]->name(); + } + } + state_ = STOPPED; +} + +bool DataTypeManagerImpl::IsRegistered(syncable::ModelType type) { + return controllers_.count(type) == 1; +} + +bool DataTypeManagerImpl::IsEnabled(syncable::ModelType type) { + return IsRegistered(type) && controllers_[type]->enabled(); +} + +} // namespace browser_sync diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h new file mode 100644 index 0000000..a9b87d4 --- /dev/null +++ b/chrome/browser/sync/glue/data_type_manager_impl.h @@ -0,0 +1,59 @@ +// Copyright (c) 2010 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_GLUE_DATA_TYPE_MANAGER_IMPL_H__ +#define CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_IMPL_H__ + +#include "base/basictypes.h" +#include "base/scoped_ptr.h" +#include "chrome/browser/sync/glue/data_type_manager.h" + +namespace browser_sync { + +class DataTypeManagerImpl : public DataTypeManager { + public: + explicit DataTypeManagerImpl(const DataTypeController::TypeMap& controllers); + virtual ~DataTypeManagerImpl() {} + + // DataTypeManager interface. + virtual void Start(StartCallback* start_callback); + + virtual void Stop(); + + virtual bool IsRegistered(syncable::ModelType type); + + virtual bool IsEnabled(syncable::ModelType type); + + virtual const DataTypeController::TypeMap& controllers() { + return controllers_; + }; + + virtual State state() { + return state_; + } + + private: + // Starts the next data type in the kStartOrder list, indicated by + // the current_type_ member. If there are no more data types to + // start, the stashed start_callback_ is invoked. + void StartNextType(); + + // Callback passed to each data type controller on startup. + void TypeStartCallback(DataTypeController::StartResult result); + + // Stops all data types. + void FinishStop(); + + DataTypeController::TypeMap controllers_; + State state_; + int current_type_; + + scoped_ptr<StartCallback> start_callback_; + + DISALLOW_COPY_AND_ASSIGN(DataTypeManagerImpl); +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_IMPL_H__ diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc new file mode 100644 index 0000000..3c12dd5 --- /dev/null +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -0,0 +1,246 @@ +// Copyright (c) 2010 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 "testing/gtest/include/gtest/gtest.h" +#include "base/message_loop.h" +#include "base/scoped_ptr.h" +#include "base/stl_util-inl.h" +#include "base/task.h" +#include "chrome/browser/chrome_thread.h" +#include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/data_type_controller_mock.h" +#include "chrome/browser/sync/glue/data_type_manager_impl.h" +#include "testing/gmock/include/gmock/gmock.h" + +using browser_sync::DataTypeManager; +using browser_sync::DataTypeManagerImpl; +using browser_sync::DataTypeController; +using browser_sync::DataTypeControllerMock; +using testing::_; +using testing::Return; +using testing::SaveArg; + +ACTION_P(InvokeCallback, callback_result) { + arg1->Run(callback_result); + delete arg1; +} + +class StartCallback { + public: + MOCK_METHOD1(Run, void(DataTypeManager::StartResult result)); +}; + +class DataTypeManagerImplTest : public testing::Test { + public: + DataTypeManagerImplTest() + : ui_thread_(ChromeThread::UI, &message_loop_) {} + + virtual ~DataTypeManagerImplTest() { + STLDeleteValues(&controllers_); + } + + protected: + DataTypeControllerMock* MakeBookmarkDTC() { + DataTypeControllerMock* dtc = new DataTypeControllerMock(); + EXPECT_CALL(*dtc, enabled()).WillRepeatedly(Return(true)); + EXPECT_CALL(*dtc, type()).WillRepeatedly(Return(syncable::BOOKMARKS)); + EXPECT_CALL(*dtc, name()).WillRepeatedly(Return("bookmark")); + return dtc; + } + + DataTypeControllerMock* MakePreferenceDTC() { + DataTypeControllerMock* dtc = new DataTypeControllerMock(); + EXPECT_CALL(*dtc, enabled()).WillRepeatedly(Return(true)); + EXPECT_CALL(*dtc, type()).WillRepeatedly(Return(syncable::PREFERENCES)); + EXPECT_CALL(*dtc, name()).WillRepeatedly(Return("preference")); + return dtc; + } + + void SetStartStopExpectations(DataTypeControllerMock* mock_dtc) { + EXPECT_CALL(*mock_dtc, Start(true, _)). + WillOnce(InvokeCallback((DataTypeController::OK))); + EXPECT_CALL(*mock_dtc, Stop()).Times(1); + + // The state() getter is used a few times in DCHECK code and needs + // different mock programming in debug vs. release mode. +#ifdef NDEBUG + EXPECT_CALL(*mock_dtc, state()). + WillOnce(Return(DataTypeController::RUNNING)); +#else + EXPECT_CALL(*mock_dtc, state()). + WillOnce(Return(DataTypeController::NOT_RUNNING)). + WillOnce(Return(DataTypeController::RUNNING)). + WillOnce(Return(DataTypeController::RUNNING)); +#endif + } + + MessageLoopForUI message_loop_; + ChromeThread ui_thread_; + DataTypeController::TypeMap controllers_; + StartCallback callback_; +}; + +TEST_F(DataTypeManagerImplTest, NoControllers) { + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::OK)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STARTED, dtm.state()); + dtm.Stop(); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, OneDisabledController) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*bookmark_dtc, enabled()).WillRepeatedly(Return(false)); + EXPECT_CALL(*bookmark_dtc, Start(_, _)).Times(0); + EXPECT_CALL(*bookmark_dtc, Stop()).Times(0); + EXPECT_CALL(*bookmark_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::OK)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STARTED, dtm.state()); + dtm.Stop(); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, OneEnabledController) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + SetStartStopExpectations(bookmark_dtc); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::OK)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STARTED, dtm.state()); + dtm.Stop(); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, OneFailingController) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*bookmark_dtc, Start(true, _)). + WillOnce(InvokeCallback((DataTypeController::ASSOCIATION_FAILED))); + EXPECT_CALL(*bookmark_dtc, Stop()).Times(0); + // The state() getter is used a few times in DCHECK code and needs + // different mock programming in debug vs. release mode. + EXPECT_CALL(*bookmark_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::ASSOCIATION_FAILED)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, TwoEnabledControllers) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + SetStartStopExpectations(bookmark_dtc); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); + SetStartStopExpectations(preference_dtc); + controllers_[syncable::PREFERENCES] = preference_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::OK)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STARTED, dtm.state()); + dtm.Stop(); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, InterruptedStart) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + SetStartStopExpectations(bookmark_dtc); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); + // Save the callback here so we can interrupt startup. + DataTypeController::StartCallback* callback; + EXPECT_CALL(*preference_dtc, Start(true, _)). + WillOnce(SaveArg<1>(&callback)); + EXPECT_CALL(*preference_dtc, Stop()).Times(1); + EXPECT_CALL(*preference_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::PREFERENCES] = preference_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::ABORTED)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STARTING, dtm.state()); + + // Call stop before the preference callback is invoked. + dtm.Stop(); + callback->Run(DataTypeController::ABORTED); + delete callback; + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, SecondControllerFails) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + SetStartStopExpectations(bookmark_dtc); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + + DataTypeControllerMock* preference_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*preference_dtc, Start(true, _)). + WillOnce(InvokeCallback((DataTypeController::ASSOCIATION_FAILED))); + EXPECT_CALL(*preference_dtc, Stop()).Times(0); + EXPECT_CALL(*preference_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::PREFERENCES] = preference_dtc; + + DataTypeManagerImpl dtm(controllers_); + EXPECT_CALL(callback_, Run(DataTypeManager::ASSOCIATION_FAILED)); + dtm.Start(NewCallback(&callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); +} + +TEST_F(DataTypeManagerImplTest, IsRegisteredNone) { + DataTypeManagerImpl dtm(controllers_); + EXPECT_FALSE(dtm.IsRegistered(syncable::BOOKMARKS)); +} + +TEST_F(DataTypeManagerImplTest, IsRegisteredButNoMatch) { + DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); + EXPECT_CALL(*preference_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::PREFERENCES] = preference_dtc; + DataTypeManagerImpl dtm(controllers_); + EXPECT_FALSE(dtm.IsRegistered(syncable::BOOKMARKS)); +} + +TEST_F(DataTypeManagerImplTest, IsRegisteredMatch) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*bookmark_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + DataTypeManagerImpl dtm(controllers_); + EXPECT_TRUE(dtm.IsRegistered(syncable::BOOKMARKS)); +} + +TEST_F(DataTypeManagerImplTest, IsNotEnabled) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*bookmark_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + EXPECT_CALL(*bookmark_dtc, enabled()). + WillRepeatedly(Return(false)); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + DataTypeManagerImpl dtm(controllers_); + EXPECT_FALSE(dtm.IsEnabled(syncable::BOOKMARKS)); +} + +TEST_F(DataTypeManagerImplTest, IsEnabled) { + DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); + EXPECT_CALL(*bookmark_dtc, state()). + WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); + EXPECT_CALL(*bookmark_dtc, enabled()). + WillRepeatedly(Return(true)); + controllers_[syncable::BOOKMARKS] = bookmark_dtc; + DataTypeManagerImpl dtm(controllers_); + EXPECT_TRUE(dtm.IsEnabled(syncable::BOOKMARKS)); +} diff --git a/chrome/browser/sync/glue/data_type_manager_mock.h b/chrome/browser/sync/glue/data_type_manager_mock.h new file mode 100644 index 0000000..540553b --- /dev/null +++ b/chrome/browser/sync/glue/data_type_manager_mock.h @@ -0,0 +1,25 @@ +// Copyright (c) 2010 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_GLUE_DATA_TYPE_MANAGER_MOCK_H__ +#define CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_MOCK_H__ + +#include "chrome/browser/sync/glue/data_type_manager.h" +#include "testing/gmock/include/gmock/gmock.h" + +namespace browser_sync { + +class DataTypeManagerMock : public DataTypeManager { + public: + MOCK_METHOD1(Start, void(StartCallback* start_callback)); + MOCK_METHOD0(Stop, void()); + MOCK_METHOD1(IsRegistered, bool(syncable::ModelType type)); + MOCK_METHOD1(IsEnabled, bool(syncable::ModelType type)); + MOCK_METHOD0(controllers, const DataTypeController::TypeMap&()); + MOCK_METHOD0(state, State()); +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_DATA_TYPE_MANAGER_MOCK_H__ diff --git a/chrome/browser/sync/glue/preference_data_type_controller.h b/chrome/browser/sync/glue/preference_data_type_controller.h index bffd6b2..68305ca 100644 --- a/chrome/browser/sync/glue/preference_data_type_controller.h +++ b/chrome/browser/sync/glue/preference_data_type_controller.h @@ -40,6 +40,11 @@ class PreferenceDataTypeController : public DataTypeController { return browser_sync::GROUP_UI; } + virtual const char* name() const { + // For logging only. + return "preference"; + } + virtual State state() { return state_; } diff --git a/chrome/browser/sync/profile_sync_factory.h b/chrome/browser/sync/profile_sync_factory.h index dc728af..fbb60b2 100644 --- a/chrome/browser/sync/profile_sync_factory.h +++ b/chrome/browser/sync/profile_sync_factory.h @@ -7,12 +7,14 @@ #include <utility> #include "base/task.h" +#include "chrome/browser/sync/glue/change_processor.h" +#include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/model_associator.h" class ProfileSyncService; namespace browser_sync { -class AssociatorInterface; -class ChangeProcessor; +class DataTypeManager; } // Factory class for all profile sync related classes. @@ -37,6 +39,11 @@ class ProfileSyncFactory { // is owned by the caller. virtual ProfileSyncService* CreateProfileSyncService() = 0; + // Instantiates a new DataTypeManager with a list of data type + // controllers. The return pointer is owned by the caller. + virtual browser_sync::DataTypeManager* CreateDataTypeManager( + const browser_sync::DataTypeController::TypeMap& controllers) = 0; + // Instantiates both a model associator and change processor for the // bookmark data type. The pointers in the return struct are owned // by the caller. diff --git a/chrome/browser/sync/profile_sync_factory_impl.cc b/chrome/browser/sync/profile_sync_factory_impl.cc index 2ae13f7..49293a3 100644 --- a/chrome/browser/sync/profile_sync_factory_impl.cc +++ b/chrome/browser/sync/profile_sync_factory_impl.cc @@ -5,23 +5,25 @@ #include "base/command_line.h" #include "chrome/browser/defaults.h" #include "chrome/browser/profile.h" -#include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/glue/bookmark_data_type_controller.h" #include "chrome/browser/sync/glue/bookmark_model_associator.h" +#include "chrome/browser/sync/glue/data_type_manager_impl.h" #include "chrome/browser/sync/glue/preference_change_processor.h" #include "chrome/browser/sync/glue/preference_data_type_controller.h" #include "chrome/browser/sync/glue/preference_model_associator.h" -#include "chrome/browser/sync/glue/model_associator.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_factory_impl.h" #include "chrome/common/chrome_switches.h" -using browser_sync::BookmarkDataTypeController; -using browser_sync::PreferenceDataTypeController; using browser_sync::BookmarkChangeProcessor; +using browser_sync::BookmarkDataTypeController; using browser_sync::BookmarkModelAssociator; +using browser_sync::DataTypeController; +using browser_sync::DataTypeManager; +using browser_sync::DataTypeManagerImpl; using browser_sync::PreferenceChangeProcessor; +using browser_sync::PreferenceDataTypeController; using browser_sync::PreferenceModelAssociator; ProfileSyncFactoryImpl::ProfileSyncFactoryImpl( @@ -32,7 +34,8 @@ ProfileSyncFactoryImpl::ProfileSyncFactoryImpl( ProfileSyncService* ProfileSyncFactoryImpl::CreateProfileSyncService() { ProfileSyncService* pss = - new ProfileSyncService(profile_, + new ProfileSyncService(this, + profile_, browser_defaults::kBootstrapSyncAuthentication); // Bookmark sync is enabled by default. Register unless explicitly @@ -52,6 +55,11 @@ ProfileSyncService* ProfileSyncFactoryImpl::CreateProfileSyncService() { return pss; } +DataTypeManager* ProfileSyncFactoryImpl::CreateDataTypeManager( + const DataTypeController::TypeMap& controllers) { + return new DataTypeManagerImpl(controllers); +} + ProfileSyncFactory::SyncComponents ProfileSyncFactoryImpl::CreateBookmarkSyncComponents( ProfileSyncService* profile_sync_service) { diff --git a/chrome/browser/sync/profile_sync_factory_impl.h b/chrome/browser/sync/profile_sync_factory_impl.h index a6c9547..7ff8fb03 100644 --- a/chrome/browser/sync/profile_sync_factory_impl.h +++ b/chrome/browser/sync/profile_sync_factory_impl.h @@ -19,6 +19,9 @@ class ProfileSyncFactoryImpl : public ProfileSyncFactory { // ProfileSyncFactory interface. virtual ProfileSyncService* CreateProfileSyncService(); + virtual browser_sync::DataTypeManager* CreateDataTypeManager( + const browser_sync::DataTypeController::TypeMap& controllers); + virtual SyncComponents CreateBookmarkSyncComponents( ProfileSyncService* profile_sync_service); diff --git a/chrome/browser/sync/profile_sync_factory_mock.h b/chrome/browser/sync/profile_sync_factory_mock.h index ec2ff2c..4257a0f 100644 --- a/chrome/browser/sync/profile_sync_factory_mock.h +++ b/chrome/browser/sync/profile_sync_factory_mock.h @@ -24,6 +24,10 @@ class ProfileSyncFactoryMock : public ProfileSyncFactory { MOCK_METHOD0(CreateProfileSyncService, ProfileSyncService*(void)); + MOCK_METHOD1(CreateDataTypeManager, + browser_sync::DataTypeManager*( + const browser_sync::DataTypeController::TypeMap& + controllers)); MOCK_METHOD1(CreateBookmarkSyncComponents, SyncComponents(ProfileSyncService* profile_sync_service)); MOCK_METHOD1(CreatePreferenceSyncComponents, diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 6445b06..4f2913e 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -18,6 +18,8 @@ #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/data_type_manager.h" +#include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" #include "chrome/common/time_format.h" @@ -27,6 +29,7 @@ using browser_sync::ChangeProcessor; using browser_sync::DataTypeController; +using browser_sync::DataTypeManager; using browser_sync::SyncBackendHost; typedef GoogleServiceAuthError AuthError; @@ -34,9 +37,11 @@ typedef GoogleServiceAuthError AuthError; // Default sync server URL. static const char kSyncServerUrl[] = "https://clients4.google.com/chrome-sync"; -ProfileSyncService::ProfileSyncService(Profile* profile, +ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory, + Profile* profile, bool bootstrap_sync_authentication) : last_auth_error_(AuthError::None()), + factory_(factory), profile_(profile), bootstrap_sync_authentication_(bootstrap_sync_authentication), sync_service_url_(kSyncServerUrl), @@ -176,12 +181,14 @@ void ProfileSyncService::Shutdown(bool sync_disabled) { if (backend_.get()) backend_->Shutdown(sync_disabled); - // Stop all data type controllers. - // TODO(skrul): Change this to support multiple data type controllers. - StopDataType(syncable::BOOKMARKS); - StopDataType(syncable::PREFERENCES); + // Stop all data type controllers, if needed. + if (data_type_manager_.get() && + data_type_manager_->state() != DataTypeManager::STOPPED) { + data_type_manager_->Stop(); + } backend_.reset(); + data_type_manager_.reset(); // Clear various flags. is_auth_in_progress_ = false; @@ -234,9 +241,9 @@ void ProfileSyncService::UpdateLastSyncedTime() { void ProfileSyncService::OnUnrecoverableError() { unrecoverable_error_detected_ = true; - // TODO(skrul): Change this to support multiple data type controllers. - StopDataType(syncable::BOOKMARKS); - StopDataType(syncable::PREFERENCES); + // Shut all data types down. + if (data_type_manager_.get()) + data_type_manager_->Stop(); // Tell the wizard so it can inform the user only if it is already open. wizard_.Step(SyncSetupWizard::FATAL_ERROR); @@ -351,14 +358,6 @@ string16 ProfileSyncService::GetAuthenticatedUsername() const { return backend_->GetAuthenticatedUsername(); } -void ProfileSyncService::StopDataType(syncable::ModelType model_type) { - if (data_type_controllers_.count(model_type) && - data_type_controllers_[model_type]->state() != - DataTypeController::NOT_RUNNING) { - data_type_controllers_[model_type]->Stop(); - } -} - void ProfileSyncService::OnUserSubmittedAuth( const std::string& username, const std::string& password, const std::string& captcha) { @@ -371,12 +370,8 @@ void ProfileSyncService::OnUserSubmittedAuth( } void ProfileSyncService::OnUserAcceptedMergeAndSync() { - // Start each data type with "merge_allowed" to correspond to the - // user's acceptance of merge. - // TODO(skrul): Change this to support multiple data type controllers. - data_type_controllers_[syncable::BOOKMARKS]->Start( - true, - NewCallback(this, &ProfileSyncService::BookmarkStartCallback)); + // TODO(skrul): Remove this. + NOTREACHED(); } void ProfileSyncService::OnUserCancelledDialog() { @@ -393,89 +388,27 @@ void ProfileSyncService::StartProcessingChangesIfReady() { DCHECK(backend_initialized_); startup_had_first_time_ = false; - // If the user has completed sync setup, we are always allowed to - // merge data. - bool merge_allowed = - profile_->GetPrefs()->GetBoolean(prefs::kSyncHasSetupCompleted); - // If we're running inside Chromium OS, always allow merges and // consider the sync setup complete. if (bootstrap_sync_authentication_) { - merge_allowed = true; SetSyncSetupCompleted(); } - // Start data types. - // TODO(skrul): Change this to support multiple data type controllers. - if (data_type_controllers_.count(syncable::BOOKMARKS)) { - data_type_controllers_[syncable::BOOKMARKS]->Start( - merge_allowed, - NewCallback(this, &ProfileSyncService::BookmarkStartCallback)); - } else { - if (data_type_controllers_.count(syncable::PREFERENCES)) { - data_type_controllers_[syncable::PREFERENCES]->Start( - true, - NewCallback(this, &ProfileSyncService::PreferenceStartCallback)); - } - } + data_type_manager_.reset( + factory_->CreateDataTypeManager(data_type_controllers_)); + data_type_manager_->Start( + NewCallback(this, &ProfileSyncService::DataTypeManagerStartCallback)); } -void ProfileSyncService::BookmarkStartCallback( - DataTypeController::StartResult result) { - switch (result) { - case DataTypeController::OK: - case DataTypeController::OK_FIRST_RUN: { - startup_had_first_time_ |= result == DataTypeController::OK_FIRST_RUN; - - // If the preference data type was registered, start it here. - // Since we only care about presenting the merge warning dialog - // for bookmarks, pass a "true" for merge_allowed so preferences - // will always start. If preferences is not registered, just - // call the callback directly so we can finish startup. - // TODO(skrul): Change this to support multiple data type - // controllers. - if (data_type_controllers_.count(syncable::PREFERENCES)) { - data_type_controllers_[syncable::PREFERENCES]->Start( - true, - NewCallback(this, &ProfileSyncService::PreferenceStartCallback)); - } else { - PreferenceStartCallback(DataTypeController::OK); - } - break; - } - - case DataTypeController::NEEDS_MERGE: { - ProfileSyncService::SyncEvent(MERGE_AND_SYNC_NEEDED); - wizard_.Step(SyncSetupWizard::MERGE_AND_SYNC); - break; - } - - default: { - LOG(ERROR) << "Bookmark start failed"; - OnUnrecoverableError(); - break; - } +void ProfileSyncService::DataTypeManagerStartCallback( + DataTypeManager::StartResult result) { + if (result != DataTypeManager::OK) { + OnUnrecoverableError(); + return; } -} - -void ProfileSyncService::PreferenceStartCallback( - DataTypeController::StartResult result) { - switch (result) { - case DataTypeController::OK: - case DataTypeController::OK_FIRST_RUN: { - startup_had_first_time_ |= result == DataTypeController::OK_FIRST_RUN; - wizard_.Step(startup_had_first_time_ ? SyncSetupWizard:: DONE_FIRST_TIME : - SyncSetupWizard::DONE); - FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged()); - break; - } - default: { - LOG(ERROR) << "Preference start failed"; - OnUnrecoverableError(); - break; - } - } + wizard_.Step(SyncSetupWizard::DONE); + FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged()); } void ProfileSyncService::ActivateDataType( @@ -511,26 +444,14 @@ bool ProfileSyncService::IsSyncEnabled() { } bool ProfileSyncService::ShouldPushChanges() { - // True only after all bootstrapping has succeeded: the bookmark model is - // loaded, the sync backend is initialized, the two domains are - // consistent with one another, and no unrecoverable error has transpired. - - // Don't push changes if there are no data type controllers registered. - if (data_type_controllers_.size() == 0) + // True only after all bootstrapping has succeeded: the sync backend + // is initialized, all enabled data types are consistent with one + // another, and no unrecoverable error has transpired. + if (unrecoverable_error_detected_) return false; - // TODO: make this size_t - DataTypeController::TypeMap::size_type running_data_type_controllers = 0; - if (data_type_controllers_.count(syncable::BOOKMARKS) && - data_type_controllers_[syncable::BOOKMARKS]->state() == - DataTypeController::RUNNING) - running_data_type_controllers++; - - if (data_type_controllers_.count(syncable::PREFERENCES) && - data_type_controllers_[syncable::PREFERENCES]->state() == - DataTypeController::RUNNING) - running_data_type_controllers++; + if (!data_type_manager_.get()) + return false; - // Return true only if all data type controllers are running. - return data_type_controllers_.size() == running_data_type_controllers; + return data_type_manager_->state() == DataTypeManager::STARTED; } diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index ce36f98..109e814 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -15,6 +15,7 @@ #include "chrome/browser/google_service_auth_error.h" #include "chrome/browser/profile.h" #include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/data_type_manager.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/notification_method.h" #include "chrome/browser/sync/sync_setup_wizard.h" @@ -22,6 +23,8 @@ #include "googleurl/src/gurl.h" #include "testing/gtest/include/gtest/gtest_prod.h" +class ProfileSyncFactory; + namespace browser_sync { class ChangeProcessor; @@ -86,7 +89,9 @@ class ProfileSyncService : public browser_sync::SyncFrontend, MAX_SYNC_EVENT_CODE }; - ProfileSyncService(Profile* profile, bool bootstrap_sync_authentication); + ProfileSyncService(ProfileSyncFactory* factory_, + Profile* profile, + bool bootstrap_sync_authentication); virtual ~ProfileSyncService(); // Initializes the object. This should be called every time an object of this @@ -236,10 +241,8 @@ class ProfileSyncService : public browser_sync::SyncFrontend, void RegisterPreferences(); void ClearPreferences(); - void BookmarkStartCallback( - browser_sync::DataTypeController::StartResult result); - void PreferenceStartCallback( - browser_sync::DataTypeController::StartResult result); + void DataTypeManagerStartCallback( + browser_sync::DataTypeManager::StartResult result); // Tests need to override this. If |delete_sync_data_folder| is true, then // this method will delete all previous "Sync Data" folders. (useful if the @@ -271,15 +274,15 @@ class ProfileSyncService : public browser_sync::SyncFrontend, // store to bootstrap the authentication process. virtual std::string GetLsidForAuthBootstraping(); - // Stops a data type. - void StopDataType(syncable::ModelType model_type); - // Time at which we begin an attempt a GAIA authorization. base::TimeTicks auth_start_time_; // Time at which error UI is presented for the new tab page. base::TimeTicks auth_error_time_; + // Factory used to create various dependent objects. + ProfileSyncFactory* factory_; + // The profile whose data we are synchronizing. Profile* profile_; @@ -339,6 +342,9 @@ class ProfileSyncService : public browser_sync::SyncFrontend, // Which peer-to-peer notification method to use. browser_sync::NotificationMethod notification_method_; + // Manages the start and stop of the various data types. + scoped_ptr<browser_sync::DataTypeManager> data_type_manager_; + ObserverList<Observer> observers_; DISALLOW_COPY_AND_ASSIGN(ProfileSyncService); diff --git a/chrome/browser/sync/profile_sync_service_mock.h b/chrome/browser/sync/profile_sync_service_mock.h index 661ada8..22e7f9e 100644 --- a/chrome/browser/sync/profile_sync_service_mock.h +++ b/chrome/browser/sync/profile_sync_service_mock.h @@ -15,7 +15,7 @@ class ProfileSyncServiceMock : public ProfileSyncService { public: - ProfileSyncServiceMock() : ProfileSyncService(NULL, false) {} + ProfileSyncServiceMock() : ProfileSyncService(NULL, NULL, false) {} virtual ~ProfileSyncServiceMock() {} MOCK_METHOD0(EnableForUser, void()); diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index 579ab4f..f4a5a7d 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -8,12 +8,14 @@ #include "chrome/browser/sync/glue/preference_data_type_controller.h" #include "chrome/browser/sync/glue/preference_model_associator.h" #include "chrome/browser/sync/glue/sync_backend_host.h" +#include "chrome/browser/sync/profile_sync_factory_mock.h" #include "chrome/browser/sync/profile_sync_test_util.h" #include "chrome/browser/sync/protocol/preference_specifics.pb.h" #include "chrome/browser/sync/test_profile_sync_service.h" #include "chrome/common/json_value_serializer.h" #include "chrome/common/pref_names.h" #include "chrome/test/testing_profile.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" using base::JSONReader; @@ -22,6 +24,8 @@ using browser_sync::PreferenceDataTypeController; using browser_sync::PreferenceModelAssociator; using browser_sync::SyncBackendHost; using sync_api::SyncManager; +using testing::_; +using testing::Return; class TestPreferenceModelAssociator : public TestModelAssociator<PreferenceModelAssociator> { @@ -50,18 +54,22 @@ class ProfileSyncServicePreferenceTest : public testing::Test { void StartSyncService() { if (!service_.get()) { - service_.reset(new TestProfileSyncService(profile_.get(), false)); + service_.reset(new TestProfileSyncService(&factory_, + profile_.get(), + false)); // Register the preference data type. model_associator_ = new TestPreferenceModelAssociator(service_.get()); change_processor_ = new PreferenceChangeProcessor(model_associator_, service_.get()); - factory_.reset(new TestProfileSyncFactory(NULL, - model_associator_, - change_processor_)); + EXPECT_CALL(factory_, CreatePreferenceSyncComponents(_)). + WillOnce(Return(ProfileSyncFactory::SyncComponents( + model_associator_, change_processor_))); + EXPECT_CALL(factory_, CreateDataTypeManager(_)). + WillOnce(MakeDataTypeManager()); service_->RegisterDataTypeController( - new PreferenceDataTypeController(factory_.get(), + new PreferenceDataTypeController(&factory_, service_.get())); service_->Initialize(); } @@ -124,7 +132,7 @@ class ProfileSyncServicePreferenceTest : public testing::Test { scoped_ptr<TestProfileSyncService> service_; scoped_ptr<TestingProfile> profile_; - scoped_ptr<TestProfileSyncFactory> factory_; + ProfileSyncFactoryMock factory_; PreferenceModelAssociator* model_associator_; PreferenceChangeProcessor* change_processor_; diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index 4072ad3..6b65540 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -7,23 +7,24 @@ #include "base/message_loop.h" #include "base/scoped_ptr.h" #include "chrome/browser/chrome_thread.h" -#include "chrome/browser/sync/glue/data_type_controller.h" -#include "chrome/browser/sync/glue/data_type_controller_mock.h" +#include "chrome/browser/sync/glue/data_type_manager.h" +#include "chrome/browser/sync/glue/data_type_manager_mock.h" +#include "chrome/browser/sync/profile_sync_factory_mock.h" #include "chrome/browser/sync/test_profile_sync_service.h" #include "chrome/common/pref_names.h" #include "chrome/test/testing_profile.h" #include "testing/gmock/include/gmock/gmock.h" -using browser_sync::DataTypeController; -using browser_sync::DataTypeControllerMock; +using browser_sync::DataTypeManager; +using browser_sync::DataTypeManagerMock; using testing::_; using testing::InvokeArgument; using testing::Mock; using testing::Return; ACTION_P(InvokeCallback, callback_result) { - arg1->Run(callback_result); - delete arg1; + arg0->Run(callback_result); + delete arg0; } // TODO(skrul) This test fails on the mac. See http://crbug.com/33443 @@ -51,7 +52,7 @@ class ProfileSyncServiceStartupTest : public testing::Test { } virtual void SetUp() { - service_.reset(new TestProfileSyncService(&profile_, false)); + service_.reset(new TestProfileSyncService(&factory_, &profile_, false)); service_->AddObserver(&observer_); } @@ -60,145 +61,71 @@ class ProfileSyncServiceStartupTest : public testing::Test { } protected: - DataTypeControllerMock* MakeBookmarkDTC() { - DataTypeControllerMock* dtc = new DataTypeControllerMock(); - EXPECT_CALL(*dtc, enabled()).WillRepeatedly(Return(true)); - EXPECT_CALL(*dtc, model_safe_group()). - WillRepeatedly(Return(browser_sync::GROUP_UI)); - EXPECT_CALL(*dtc, type()).WillRepeatedly(Return(syncable::BOOKMARKS)); - return dtc; - } - - DataTypeControllerMock* MakePreferenceDTC() { - DataTypeControllerMock* dtc = new DataTypeControllerMock(); - EXPECT_CALL(*dtc, enabled()).WillRepeatedly(Return(true)); - EXPECT_CALL(*dtc, model_safe_group()). - WillRepeatedly(Return(browser_sync::GROUP_UI)); - EXPECT_CALL(*dtc, type()).WillRepeatedly(Return(syncable::PREFERENCES)); - return dtc; - } - - void SetStartStopExpectations(DataTypeControllerMock* mock_dtc) { - EXPECT_CALL(*mock_dtc, Start(_, _)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(*mock_dtc, Stop()).Times(1); - EXPECT_CALL(*mock_dtc, state()). - WillOnce(Return(DataTypeController::RUNNING)); + DataTypeManagerMock* SetUpDataTypeManager() { + DataTypeManagerMock* data_type_manager = new DataTypeManagerMock(); + EXPECT_CALL(factory_, CreateDataTypeManager(_)). + WillOnce(Return(data_type_manager)); + return data_type_manager; } MessageLoop message_loop_; ChromeThread ui_thread_; TestingProfile profile_; + ProfileSyncFactoryMock factory_; scoped_ptr<TestProfileSyncService> service_; ProfileSyncServiceObserverMock observer_; }; TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFirstTime)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - EXPECT_CALL(*bookmark_dtc, Start(false, _)).Times(0); - EXPECT_CALL(*bookmark_dtc, state()). - WillRepeatedly(Return(DataTypeController::NOT_RUNNING)); - - EXPECT_CALL(observer_, OnStateChanged()).Times(1); + DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); + EXPECT_CALL(*data_type_manager, Start(_)).Times(0); + // We've never completed startup. profile_.GetPrefs()->ClearPref(prefs::kSyncHasSetupCompleted); - service_->RegisterDataTypeController(bookmark_dtc); + // Should not actually start, rather just clean things up and wait // to be enabled. + EXPECT_CALL(observer_, OnStateChanged()).Times(1); service_->Initialize(); // Preferences should be back to defaults. EXPECT_EQ(0, profile_.GetPrefs()->GetInt64(prefs::kSyncLastSyncedTime)); EXPECT_FALSE(profile_.GetPrefs()->GetBoolean(prefs::kSyncHasSetupCompleted)); - Mock::VerifyAndClearExpectations(&bookmark_dtc); + Mock::VerifyAndClearExpectations(data_type_manager); // Then start things up. - SetStartStopExpectations(bookmark_dtc); + EXPECT_CALL(*data_type_manager, Start(_)). + WillOnce(InvokeCallback(DataTypeManager::OK)); + EXPECT_CALL(*data_type_manager, state()). + WillOnce(Return(DataTypeManager::STARTED)); + EXPECT_CALL(*data_type_manager, Stop()).Times(1); EXPECT_CALL(observer_, OnStateChanged()).Times(3); service_->EnableForUser(); } -TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartBookmarks)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - SetStartStopExpectations(bookmark_dtc); +TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartNormal)) { + DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); + EXPECT_CALL(*data_type_manager, Start(_)). + WillOnce(InvokeCallback(DataTypeManager::OK)); + EXPECT_CALL(*data_type_manager, state()). + WillOnce(Return(DataTypeManager::STARTED)); + EXPECT_CALL(*data_type_manager, Stop()).Times(1); EXPECT_CALL(observer_, OnStateChanged()).Times(2); - service_->RegisterDataTypeController(bookmark_dtc); service_->Initialize(); } -TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartBookmarksWithMerge)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - EXPECT_CALL(*bookmark_dtc, Start(false, _)). - WillOnce(InvokeCallback((DataTypeController::NEEDS_MERGE))); - EXPECT_CALL(*bookmark_dtc, Start(true, _)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(*bookmark_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillOnce(Return(DataTypeController::RUNNING)); - - EXPECT_CALL(*bookmark_dtc, Stop()).Times(1); - - EXPECT_CALL(observer_, OnStateChanged()).Times(4); - - // Set kSyncHasSetupCompleted to false so the first attempt to start - // the bookmark dtc is done with merge_allowed = false. This has - // the side effect of disabling sync, so we must explicitly call - // EnableForUser. - profile_.GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false); - service_->RegisterDataTypeController(bookmark_dtc); - service_->Initialize(); - service_->EnableForUser(); - service_->OnUserAcceptedMergeAndSync(); -} - -TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartPreferences)) { - DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); - SetStartStopExpectations(preference_dtc); +TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFailure)) { + DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); + EXPECT_CALL(*data_type_manager, Start(_)). + WillOnce(InvokeCallback(DataTypeManager::ASSOCIATION_FAILED)); + EXPECT_CALL(*data_type_manager, Stop()).Times(1); + EXPECT_CALL(*data_type_manager, state()). + WillOnce(Return(DataTypeManager::STOPPED)); EXPECT_CALL(observer_, OnStateChanged()).Times(2); - service_->RegisterDataTypeController(preference_dtc); - service_->Initialize(); -} - -TEST_F(ProfileSyncServiceStartupTest, - SKIP_MACOSX(StartBookmarksAndPreferences)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - SetStartStopExpectations(bookmark_dtc); - - DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); - SetStartStopExpectations(preference_dtc); - - EXPECT_CALL(observer_, OnStateChanged()).Times(2); - - service_->RegisterDataTypeController(bookmark_dtc); - service_->RegisterDataTypeController(preference_dtc); - service_->Initialize(); -} - -TEST_F(ProfileSyncServiceStartupTest, - SKIP_MACOSX(StartBookmarksAndPreferencesFail)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - EXPECT_CALL(*bookmark_dtc, Start(_, _)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(*bookmark_dtc, Stop()).Times(1); - EXPECT_CALL(*bookmark_dtc, state()). - WillOnce(Return(DataTypeController::RUNNING)). - WillOnce(Return(DataTypeController::NOT_RUNNING)); - - DataTypeControllerMock* preference_dtc = MakePreferenceDTC(); - EXPECT_CALL(*preference_dtc, Start(_, _)). - WillOnce(InvokeCallback((DataTypeController::ASSOCIATION_FAILED))); - EXPECT_CALL(*preference_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillOnce(Return(DataTypeController::NOT_RUNNING)); - - EXPECT_CALL(observer_, OnStateChanged()).Times(2); - - service_->RegisterDataTypeController(bookmark_dtc); - service_->RegisterDataTypeController(preference_dtc); service_->Initialize(); EXPECT_TRUE(service_->unrecoverable_error_detected()); } @@ -210,45 +137,22 @@ class ProfileSyncServiceStartupBootstrapTest : virtual ~ProfileSyncServiceStartupBootstrapTest() {} virtual void SetUp() { - service_.reset(new TestProfileSyncService(&profile_, true)); + service_.reset(new TestProfileSyncService(&factory_, &profile_, true)); service_->AddObserver(&observer_); } }; TEST_F(ProfileSyncServiceStartupBootstrapTest, SKIP_MACOSX(StartFirstTime)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - EXPECT_CALL(*bookmark_dtc, Start(_, _)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(*bookmark_dtc, Stop()).Times(1); - EXPECT_CALL(*bookmark_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillOnce(Return(DataTypeController::RUNNING)); + DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); + EXPECT_CALL(*data_type_manager, Start(_)). + WillOnce(InvokeCallback(DataTypeManager::OK)); + EXPECT_CALL(*data_type_manager, state()). + WillOnce(Return(DataTypeManager::STARTED)); + EXPECT_CALL(*data_type_manager, Stop()).Times(1); EXPECT_CALL(observer_, OnStateChanged()).Times(3); profile_.GetPrefs()->ClearPref(prefs::kSyncHasSetupCompleted); - service_->RegisterDataTypeController(bookmark_dtc); // Will start sync even though setup hasn't been completed (since // setup is bypassed when bootstrapping is enabled). service_->Initialize(); } - -TEST_F(ProfileSyncServiceStartupBootstrapTest, - SKIP_MACOSX(StartBookmarksWithMerge)) { - DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC(); - EXPECT_CALL(*bookmark_dtc, Start(true, _)). - WillOnce(InvokeCallback((DataTypeController::OK))); - EXPECT_CALL(*bookmark_dtc, Stop()).Times(1); - EXPECT_CALL(*bookmark_dtc, state()). - WillOnce(Return(DataTypeController::NOT_RUNNING)). - WillOnce(Return(DataTypeController::RUNNING)); - - EXPECT_CALL(observer_, OnStateChanged()).Times(3); - - // Set kSyncHasSetupCompleted to false so the first attempt to start - // the bookmark dtc is done with merge_allowed = false. This has - // the side effect of disabling sync, but this does not impede - // startup when bootstrapping is enabled. - profile_.GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false); - service_->RegisterDataTypeController(bookmark_dtc); - service_->Initialize(); -} diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index d3da5ed..8aca17e 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -18,23 +18,29 @@ #include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/glue/bookmark_data_type_controller.h" #include "chrome/browser/sync/glue/bookmark_model_associator.h" +#include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/model_associator.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/notification_method.h" #include "chrome/browser/sync/profile_sync_factory.h" +#include "chrome/browser/sync/profile_sync_factory_mock.h" #include "chrome/browser/sync/test_profile_sync_service.h" #include "chrome/browser/sync/profile_sync_test_util.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" #include "chrome/test/testing_profile.h" +#include "testing/gmock/include/gmock/gmock.h" using std::vector; using browser_sync::AssociatorInterface; using browser_sync::BookmarkChangeProcessor; using browser_sync::BookmarkModelAssociator; using browser_sync::ChangeProcessor; +using browser_sync::DataTypeController; using browser_sync::ModelAssociator; using browser_sync::SyncBackendHost; +using testing::_; +using testing::Return; class TestBookmarkModelAssociator : public TestModelAssociator<BookmarkModelAssociator> { @@ -222,17 +228,22 @@ class ProfileSyncServiceTest : public testing::Test { void StartSyncService() { if (!service_.get()) { - service_.reset(new TestProfileSyncService(profile_.get(), false)); + service_.reset(new TestProfileSyncService(&factory_, + profile_.get(), + false)); // Register the bookmark data type. model_associator_ = new TestBookmarkModelAssociator(service_.get()); change_processor_ = new BookmarkChangeProcessor(model_associator_, service_.get()); - factory_.reset(new TestProfileSyncFactory(NULL, - model_associator_, - change_processor_)); + EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_)). + WillOnce(Return(ProfileSyncFactory::SyncComponents( + model_associator_, change_processor_))); + EXPECT_CALL(factory_, CreateDataTypeManager(_)). + WillOnce(MakeDataTypeManager()); + service_->RegisterDataTypeController( - new browser_sync::BookmarkDataTypeController(factory_.get(), + new browser_sync::BookmarkDataTypeController(&factory_, profile_.get(), service_.get())); service_->Initialize(); @@ -414,7 +425,7 @@ class ProfileSyncServiceTest : public testing::Test { scoped_ptr<TestProfileSyncService> service_; scoped_ptr<TestingProfile> profile_; - scoped_ptr<TestProfileSyncFactory> factory_; + ProfileSyncFactoryMock factory_; BookmarkModel* model_; TestBookmarkModelAssociator* model_associator_; BookmarkChangeProcessor* change_processor_; @@ -1296,17 +1307,21 @@ TEST_F(ProfileSyncServiceTestWithData, MAYBE_TestStartupWithOldSyncData) { LoadBookmarkModel(LOAD_FROM_STORAGE, SAVE_TO_STORAGE); if (!service_.get()) { - service_.reset(new TestProfileSyncService(profile_.get(), false)); + service_.reset( + new TestProfileSyncService(&factory_, profile_.get(), false)); profile_->GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false); model_associator_ = new TestBookmarkModelAssociator(service_.get()); change_processor_ = new BookmarkChangeProcessor(model_associator_, service_.get()); - factory_.reset(new TestProfileSyncFactory(NULL, - model_associator_, - change_processor_)); + EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_)). + WillOnce(Return(ProfileSyncFactory::SyncComponents( + model_associator_, change_processor_))); + EXPECT_CALL(factory_, CreateDataTypeManager(_)). + WillOnce(MakeDataTypeManager()); + service_->RegisterDataTypeController( - new browser_sync::BookmarkDataTypeController(factory_.get(), + new browser_sync::BookmarkDataTypeController(&factory_, profile_.get(), service_.get())); diff --git a/chrome/browser/sync/profile_sync_test_util.h b/chrome/browser/sync/profile_sync_test_util.h index 48632fc..14fcaec 100644 --- a/chrome/browser/sync/profile_sync_test_util.h +++ b/chrome/browser/sync/profile_sync_test_util.h @@ -9,43 +9,17 @@ #include "chrome/browser/sync/glue/bookmark_data_type_controller.h" #include "chrome/browser/sync/glue/bookmark_model_associator.h" #include "chrome/browser/sync/glue/change_processor.h" +#include "chrome/browser/sync/glue/data_type_manager_impl.h" #include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/test/sync/test_http_bridge_factory.h" +#include "testing/gmock/include/gmock/gmock.h" -class TestProfileSyncFactory : public ProfileSyncFactory { - public: - TestProfileSyncFactory(ProfileSyncService* profile_sync_service, - browser_sync::AssociatorInterface* model_associator, - browser_sync::ChangeProcessor* change_processor) - : profile_sync_service_(profile_sync_service), - model_associator_(model_associator), - change_processor_(change_processor) {} - virtual ~TestProfileSyncFactory() {} - - virtual ProfileSyncService* CreateProfileSyncService() { - return profile_sync_service_.release(); - } - - virtual SyncComponents CreateBookmarkSyncComponents( - ProfileSyncService* service) { - return SyncComponents(model_associator_.release(), - change_processor_.release()); - } - - virtual SyncComponents CreatePreferenceSyncComponents( - ProfileSyncService* service) { - return SyncComponents(model_associator_.release(), - change_processor_.release()); - } - - private: - scoped_ptr<ProfileSyncService> profile_sync_service_; - scoped_ptr<browser_sync::AssociatorInterface> model_associator_; - scoped_ptr<browser_sync::ChangeProcessor> change_processor_; - - DISALLOW_COPY_AND_ASSIGN(TestProfileSyncFactory); -}; +// This action is used to mock out the ProfileSyncFactory +// CreateDataTypeManager method. +ACTION(MakeDataTypeManager) { + return new browser_sync::DataTypeManagerImpl(arg0); +} template <class ModelAssociatorImpl> class TestModelAssociator : public ModelAssociatorImpl { diff --git a/chrome/browser/sync/sync_setup_wizard_unittest.cc b/chrome/browser/sync/sync_setup_wizard_unittest.cc index bdb41cb..38cb614 100644 --- a/chrome/browser/sync/sync_setup_wizard_unittest.cc +++ b/chrome/browser/sync/sync_setup_wizard_unittest.cc @@ -11,6 +11,7 @@ #include "chrome/browser/browser_list.h" #include "chrome/browser/google_service_auth_error.h" #include "chrome/browser/pref_service.h" +#include "chrome/browser/sync/profile_sync_factory_mock.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/sync_setup_flow.h" #include "chrome/browser/sync/sync_setup_wizard.h" @@ -29,8 +30,8 @@ typedef GoogleServiceAuthError AuthError; // A PSS subtype to inject. class ProfileSyncServiceForWizardTest : public ProfileSyncService { public: - explicit ProfileSyncServiceForWizardTest(Profile* profile) - : ProfileSyncService(profile, false), + ProfileSyncServiceForWizardTest(ProfileSyncFactory* factory, Profile* profile) + : ProfileSyncService(factory, profile, false), user_accepted_merge_and_sync_(false), user_cancelled_dialog_(false) { RegisterPreferences(); @@ -83,13 +84,14 @@ class ProfileSyncServiceForWizardTest : public ProfileSyncService { class TestingProfileWithSyncService : public TestingProfile { public: TestingProfileWithSyncService() { - sync_service_.reset(new ProfileSyncServiceForWizardTest(this)); + sync_service_.reset(new ProfileSyncServiceForWizardTest(&factory_, this)); } virtual ProfileSyncService* GetProfileSyncService() { return sync_service_.get(); } private: + ProfileSyncFactoryMock factory_; scoped_ptr<ProfileSyncService> sync_service_; }; diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index cb86c49..9efd5d2 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -9,14 +9,16 @@ #include "base/message_loop.h" #include "chrome/browser/profile.h" +#include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/test/sync/test_http_bridge_factory.h" class TestProfileSyncService : public ProfileSyncService { public: - explicit TestProfileSyncService(Profile* profile, + explicit TestProfileSyncService(ProfileSyncFactory* factory, + Profile* profile, bool bootstrap_sync_authentication) - : ProfileSyncService(profile, bootstrap_sync_authentication) { + : ProfileSyncService(factory, profile, bootstrap_sync_authentication) { RegisterPreferences(); SetSyncSetupCompleted(); } diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 626d415..2acdcda 100755 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1774,6 +1774,9 @@ 'browser/sync/glue/change_processor.cc', 'browser/sync/glue/change_processor.h', 'browser/sync/glue/data_type_controller.h', + 'browser/sync/glue/data_type_manager.h', + 'browser/sync/glue/data_type_manager_impl.cc', + 'browser/sync/glue/data_type_manager_impl.h', 'browser/sync/glue/database_model_worker.cc', 'browser/sync/glue/database_model_worker.h', 'browser/sync/glue/http_bridge.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index d58a8d6..70cd9db 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -827,6 +827,8 @@ 'browser/ssl/ssl_host_state_unittest.cc', 'browser/sync/glue/change_processor_mock.h', 'browser/sync/glue/data_type_controller_mock.h', + 'browser/sync/glue/data_type_manager_impl_unittest.cc', + 'browser/sync/glue/data_type_manager_mock.h', 'browser/sync/glue/database_model_worker_unittest.cc', 'browser/sync/glue/http_bridge_unittest.cc', 'browser/sync/glue/ui_model_worker_unittest.cc', diff --git a/chrome/test/testing_profile.cc b/chrome/test/testing_profile.cc index 5770bb9..27708f2 100644 --- a/chrome/test/testing_profile.cc +++ b/chrome/test/testing_profile.cc @@ -5,12 +5,14 @@ #include "chrome/test/testing_profile.h" #include "build/build_config.h" +#include "base/command_line.h" #include "base/string_util.h" #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/dom_ui/ntp_resource_cache.h" #include "chrome/browser/history/history_backend.h" #include "chrome/browser/net/url_request_context_getter.h" #include "chrome/browser/sessions/session_service.h" +#include "chrome/browser/sync/profile_sync_factory_impl.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/notification_service.h" @@ -296,7 +298,11 @@ void TestingProfile::BlockUntilHistoryProcessesPendingRequests() { void TestingProfile::CreateProfileSyncService() { if (!profile_sync_service_.get()) { - profile_sync_service_.reset(new ProfileSyncService(this, false)); + profile_sync_factory_.reset( + new ProfileSyncFactoryImpl(this, + CommandLine::ForCurrentProcess())); + profile_sync_service_.reset( + profile_sync_factory_->CreateProfileSyncService()); profile_sync_service_->Initialize(); } } diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h index 1cc458b..31711fe 100644 --- a/chrome/test/testing_profile.h +++ b/chrome/test/testing_profile.h @@ -21,6 +21,8 @@ #include "chrome/browser/search_engines/template_url_model.h" #include "net/base/cookie_monster.h" +class ProfileSyncFactory; +class ProfileSyncService; class SessionService; class TestingProfile : public Profile { @@ -268,6 +270,9 @@ class TestingProfile : public Profile { // The BookmarkModel. Only created if CreateBookmarkModel is invoked. scoped_ptr<BookmarkModel> bookmark_bar_model_; + // The ProfileSyncFactory. Created by CreateProfileSyncService. + scoped_ptr<ProfileSyncFactory> profile_sync_factory_; + // The ProfileSyncService. Created by CreateProfileSyncService. scoped_ptr<ProfileSyncService> profile_sync_service_; |