diff options
author | stanisc@chromium.org <stanisc@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-22 00:36:20 +0000 |
---|---|---|
committer | stanisc@chromium.org <stanisc@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-22 00:36:20 +0000 |
commit | d85f0ac40f155b13ff9f67f85937112f6a2074de (patch) | |
tree | 8784996593344007e3337720e7d31f8cb93b253d | |
parent | 8b7e37e4ddf9c62eb5e3f01e5d68ab2a77abe091 (diff) | |
download | chromium_src-d85f0ac40f155b13ff9f67f85937112f6a2074de.zip chromium_src-d85f0ac40f155b13ff9f67f85937112f6a2074de.tar.gz chromium_src-d85f0ac40f155b13ff9f67f85937112f6a2074de.tar.bz2 |
Sync: Refactoring of DEVICE_INFO syncable type - Part 1
This reintroduces change 283461 (issue 367153005) that has been reverted due to
memory leaks in some unit tests.
The memory leak was in a mock implementation of ProfileSyncComponentsFactory.
The factory is responsible for creating an instance of LocalDeviceInfoProvider,
which it does on demand and immediately transfer ownership of
LocalDeviceInfoProvider to the caller.
But due to the way mocking was implemented in ProfileSyncComponentsFactoryMock
a mock instance of LocalDeviceInfoProvider was created in the constructor and
if a test never called CreateLocalDeviceInfoProvider(), it would be leaked.
In this change I moved away from using ON_CALL mocking mechanism that resulted
in the leak to a custom implementation that avoids that problem.
See original changes in the patch #1 and the fix in the patch #2.
The original description copied from issue 367153005:
This change introduces a new class LocalDeviceInfoProvider that is responsible
for providing the local device specific DeviceInfo/cache_guid. It initializes
the data asynchronously and allows consumers to get notified when the data
becomes available.
LocalDeviceInfoProvider will allow to remove these responsibilities from
SyncedDeviceTracker / ProfileSyncService and loose coupling between
ProfileSyncService and a number of SyncableService and DataTypeController
derived classes.
LocalDeviceInfoProvider is hosted on the frontend thread. Since it needs
cache_guid to initialize DeviceInfo, which is currently available only on the
backend, I updated SyncBackendHostCore and SyncBackendHostImpl to pass
cache_guid to the frontend via OnBackendInitialized.
For the time being SyncedDeviceTracker remains unchanged
and continues to initialize the local device info too. The entire class will be
removed in the Part 2 of the change which will move SyncedDeviceTracker
functionality to a new SyncableService.
LocalDeviceInfoProvider also replaces SessionsSyncManager::SyncInternalApiDelegate
which was previously used to decouple SessionsSyncManager from the specifics of
of the local device info implementation.
SessionsSyncManager is changed to consume LocalDeviceInfoProvider instead of
SessionsSyncManager::SyncInternalApiDelegate.
SessionDataTypeController now also consumes LocalDeviceInfoProvider to make sure
that SESSIONS type model doesn't start before local device info becomes available.
A very similar approach will be used in the upcoming second part of the change
for DEVICE_INFO DataTypeController and SyncableType.
The change includes new unit tests for LocalDeviceInfoProvider and
SessionDataTypeController.
TBR=maniscalco@chromium.org,sky@chromium.org,zea@chromium.org
BUG=395349
Review URL: https://codereview.chromium.org/398423002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284561 0039d316-1c4b-4281-b951-d872f2087c98
43 files changed, 928 insertions, 194 deletions
diff --git a/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc b/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc index f64288b..2d2b165 100644 --- a/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc +++ b/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc @@ -19,6 +19,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/supervised_user_signin_manager_wrapper.h" @@ -42,7 +43,8 @@ class FakeProfileSyncService : public ProfileSyncService { public: explicit FakeProfileSyncService(Profile* profile) : ProfileSyncService( - NULL, + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), profile, make_scoped_ptr<SupervisedUserSigninManagerWrapper>(NULL), ProfileOAuth2TokenServiceFactory::GetForProfile(profile), diff --git a/chrome/browser/extensions/api/sessions/sessions_apitest.cc b/chrome/browser/extensions/api/sessions/sessions_apitest.cc index fd2a8d6..f733c8a 100644 --- a/chrome/browser/extensions/api/sessions/sessions_apitest.cc +++ b/chrome/browser/extensions/api/sessions/sessions_apitest.cc @@ -11,7 +11,9 @@ #include "chrome/browser/extensions/extension_apitest.h" #include "chrome/browser/extensions/extension_function_test_utils.h" #include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" #include "chrome/browser/sync/open_tabs_ui_delegate.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/profile_sync_service_mock.h" @@ -85,6 +87,9 @@ class ExtensionSessionsTest : public InProcessBrowserTest { virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE; virtual void SetUpOnMainThread() OVERRIDE; protected: + static KeyedService* BuildProfileSyncService( + content::BrowserContext* profile); + void CreateTestProfileSyncService(); void CreateTestExtension(); void CreateSessionModels(); @@ -113,6 +118,27 @@ void ExtensionSessionsTest::SetUpOnMainThread() { CreateTestExtension(); } +KeyedService* ExtensionSessionsTest::BuildProfileSyncService( + content::BrowserContext* profile) { + + ProfileSyncComponentsFactoryMock* factory = + new ProfileSyncComponentsFactoryMock(); + + factory->SetLocalDeviceInfoProvider( + scoped_ptr<LocalDeviceInfoProvider>( + new browser_sync::LocalDeviceInfoProviderMock( + kSessionTags[0], + "machine name", + "Chromium 10k", + "Chrome 10k", + sync_pb::SyncEnums_DeviceType_TYPE_LINUX, + "device_id"))); + + return new ProfileSyncServiceMock( + scoped_ptr<ProfileSyncComponentsFactory>(factory), + static_cast<Profile*>(profile)); +} + void ExtensionSessionsTest::CreateTestProfileSyncService() { ProfileManager* profile_manager = g_browser_process->profile_manager(); base::FilePath path; @@ -125,7 +151,7 @@ void ExtensionSessionsTest::CreateTestProfileSyncService() { profile_manager->RegisterTestingProfile(profile, true, false); ProfileSyncServiceMock* service = static_cast<ProfileSyncServiceMock*>( ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse( - profile, &ProfileSyncServiceMock::BuildMockProfileSyncService)); + profile, &ExtensionSessionsTest::BuildProfileSyncService)); browser_ = new Browser(Browser::CreateParams( profile, chrome::HOST_DESKTOP_TYPE_NATIVE)); @@ -142,16 +168,7 @@ void ExtensionSessionsTest::CreateTestProfileSyncService() { testing::ReturnRef(no_error)); ON_CALL(*service, GetActiveDataTypes()).WillByDefault( testing::Return(preferred_types)); - ON_CALL(*service, GetLocalDeviceInfoMock()).WillByDefault( - testing::Return(new browser_sync::DeviceInfo( - std::string(kSessionTags[0]), - "machine name", - "Chromium 10k", - "Chrome 10k", - sync_pb::SyncEnums_DeviceType_TYPE_LINUX, - "device_id"))); - ON_CALL(*service, GetLocalSyncCacheGUID()).WillByDefault( - testing::Return(std::string(kSessionTags[0]))); + EXPECT_CALL(*service, AddObserver(testing::_)).Times(testing::AnyNumber()); EXPECT_CALL(*service, RemoveObserver(testing::_)).Times(testing::AnyNumber()); diff --git a/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc b/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc index a7a1d8a..d48cc0c 100644 --- a/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc +++ b/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc @@ -10,6 +10,7 @@ #include "chrome/browser/extensions/api/signed_in_devices/id_mapping_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/glue/device_info.h" +#include "chrome/browser/sync/glue/local_device_info_provider.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/common/extensions/api/signed_in_devices.h" @@ -17,6 +18,7 @@ using base::DictionaryValue; using browser_sync::DeviceInfo; +using browser_sync::LocalDeviceInfoProvider; namespace extensions { @@ -92,7 +94,10 @@ scoped_ptr<DeviceInfo> GetLocalDeviceInfo(const std::string& extension_id, if (!pss) { return scoped_ptr<DeviceInfo>(); } - std::string guid = pss->GetLocalSyncCacheGUID(); + + LocalDeviceInfoProvider* local_device = pss->GetLocalDeviceInfoProvider(); + DCHECK(local_device); + std::string guid = local_device->GetLocalSyncCacheGUID(); scoped_ptr<DeviceInfo> device = GetDeviceInfoForClientId(guid, extension_id, profile); diff --git a/chrome/browser/sync/glue/local_device_info_provider.h b/chrome/browser/sync/glue/local_device_info_provider.h new file mode 100644 index 0000000..4194c8e --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider.h @@ -0,0 +1,50 @@ +// Copyright 2014 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_LOCAL_DEVICE_INFO_PROVIDER_H_ +#define CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_H_ + +#include <string> +#include "base/callback_list.h" + +namespace browser_sync { + +class DeviceInfo; + +// Interface for providing sync specific informaiton about the +// local device. +class LocalDeviceInfoProvider { + public: + typedef base::CallbackList<void(void)>::Subscription Subscription; + + virtual ~LocalDeviceInfoProvider() {} + + // Returns sync's representation of the local device info; + // NULL if the device info is unavailable. + // The returned object is fully owned by LocalDeviceInfoProvider (must not + // be freed by the caller). It remains valid until LocalDeviceInfoProvider + // is destroyed. + virtual const DeviceInfo* GetLocalDeviceInfo() const = 0; + + // Returns a GUID string used for creation of the machine tag for + // this local session; an empty sting if LocalDeviceInfoProvider hasn't been + // initialized yet. + virtual std::string GetLocalSyncCacheGUID() const = 0; + + // Starts initializing local device info. + virtual void Initialize( + const std::string& cache_guid, + const std::string& signin_scoped_device_id) = 0; + + // Registers a callback to be called when local device info becomes available. + // The callback will remain registered until the + // returned Subscription is destroyed, which must occur before the + // CallbackList is destroyed. + virtual scoped_ptr<Subscription> RegisterOnInitializedCallback( + const base::Closure& callback) = 0; +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_H_ diff --git a/chrome/browser/sync/glue/local_device_info_provider_impl.cc b/chrome/browser/sync/glue/local_device_info_provider_impl.cc new file mode 100644 index 0000000..711e895 --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider_impl.cc @@ -0,0 +1,61 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/glue/local_device_info_provider_impl.h" + +namespace browser_sync { + +LocalDeviceInfoProviderImpl::LocalDeviceInfoProviderImpl() + : weak_factory_(this) { +} + +LocalDeviceInfoProviderImpl::~LocalDeviceInfoProviderImpl() { +} + +const DeviceInfo* +LocalDeviceInfoProviderImpl::GetLocalDeviceInfo() const { + return local_device_info_.get(); +} + +std::string LocalDeviceInfoProviderImpl::GetLocalSyncCacheGUID() const { + return cache_guid_; +} + +scoped_ptr<LocalDeviceInfoProvider::Subscription> +LocalDeviceInfoProviderImpl::RegisterOnInitializedCallback( + const base::Closure& callback) { + DCHECK(!local_device_info_.get()); + return callback_list_.Add(callback); +} + +void LocalDeviceInfoProviderImpl::Initialize( + const std::string& cache_guid, const std::string& signin_scoped_device_id) { + DCHECK(!cache_guid.empty()); + cache_guid_ = cache_guid; + DeviceInfo::CreateLocalDeviceInfo( + cache_guid_, + signin_scoped_device_id, + base::Bind(&LocalDeviceInfoProviderImpl::InitializeContinuation, + weak_factory_.GetWeakPtr())); +} + +void LocalDeviceInfoProviderImpl::InitializeContinuation( + const DeviceInfo& local_info) { + // Copy constructor is disallowed in DeviceInfo, construct a new one from + // the fields passed in local_info. + local_device_info_.reset( + new DeviceInfo( + local_info.guid(), + local_info.client_name(), + local_info.chrome_version(), + local_info.sync_user_agent(), + local_info.device_type(), + local_info.signin_scoped_device_id())); + + // Notify observers. + callback_list_.Notify(); +} + +} // namespace browser_sync + diff --git a/chrome/browser/sync/glue/local_device_info_provider_impl.h b/chrome/browser/sync/glue/local_device_info_provider_impl.h new file mode 100644 index 0000000..752a663 --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider_impl.h @@ -0,0 +1,41 @@ +// Copyright 2014 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_LOCAL_DEVICE_INFO_PROVIDER_IMPL_H_ +#define CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_IMPL_H_ + +#include "base/memory/weak_ptr.h" +#include "chrome/browser/sync/glue/device_info.h" +#include "chrome/browser/sync/glue/local_device_info_provider.h" + +namespace browser_sync { + +class LocalDeviceInfoProviderImpl : public LocalDeviceInfoProvider { + public: + LocalDeviceInfoProviderImpl(); + virtual ~LocalDeviceInfoProviderImpl(); + + // LocalDeviceInfoProvider implementation. + virtual const DeviceInfo* GetLocalDeviceInfo() const OVERRIDE; + virtual std::string GetLocalSyncCacheGUID() const OVERRIDE; + virtual void Initialize( + const std::string& cache_guid, + const std::string& signin_scoped_device_id) OVERRIDE; + virtual scoped_ptr<Subscription> RegisterOnInitializedCallback( + const base::Closure& callback) OVERRIDE; + + private: + void InitializeContinuation(const DeviceInfo& local_info); + + std::string cache_guid_; + scoped_ptr<DeviceInfo> local_device_info_; + base::CallbackList<void(void)> callback_list_; + base::WeakPtrFactory<LocalDeviceInfoProviderImpl> weak_factory_; + + DISALLOW_COPY_AND_ASSIGN(LocalDeviceInfoProviderImpl); +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_IMPL_H_ diff --git a/chrome/browser/sync/glue/local_device_info_provider_mock.cc b/chrome/browser/sync/glue/local_device_info_provider_mock.cc new file mode 100644 index 0000000..863ca73 --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider_mock.cc @@ -0,0 +1,61 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" + +namespace browser_sync { + +LocalDeviceInfoProviderMock::LocalDeviceInfoProviderMock() + : is_initialized_(false) {} + +LocalDeviceInfoProviderMock::LocalDeviceInfoProviderMock( + const std::string& guid, + const std::string& client_name, + const std::string& chrome_version, + const std::string& sync_user_agent, + const sync_pb::SyncEnums::DeviceType device_type, + const std::string& signin_scoped_device_id) + : is_initialized_(true) { + local_device_info_.reset( + new DeviceInfo( + guid, + client_name, + chrome_version, + sync_user_agent, + device_type, + signin_scoped_device_id)); +} + +LocalDeviceInfoProviderMock::~LocalDeviceInfoProviderMock() {} + +const DeviceInfo* +LocalDeviceInfoProviderMock::GetLocalDeviceInfo() const { + return is_initialized_ ? local_device_info_.get() : NULL; +} + +std::string LocalDeviceInfoProviderMock::GetLocalSyncCacheGUID() const { + return local_device_info_.get() ? local_device_info_->guid() : ""; +} + +void LocalDeviceInfoProviderMock::Initialize( + const std::string& cache_guid, const std::string& signin_scoped_device_id) { + // Ignored for the mock provider. +} + +scoped_ptr<LocalDeviceInfoProvider::Subscription> +LocalDeviceInfoProviderMock::RegisterOnInitializedCallback( + const base::Closure& callback) { + DCHECK(!is_initialized_); + return callback_list_.Add(callback); +} + +void LocalDeviceInfoProviderMock::SetInitialized(bool is_initialized) { + is_initialized_ = is_initialized; + if (is_initialized_) { + callback_list_.Notify(); + } +} + +} // namespace browser_sync + diff --git a/chrome/browser/sync/glue/local_device_info_provider_mock.h b/chrome/browser/sync/glue/local_device_info_provider_mock.h new file mode 100644 index 0000000..a795aea --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider_mock.h @@ -0,0 +1,46 @@ +// Copyright 2014 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_LOCAL_DEVICE_INFO_PROVIDER_MOCK_H_ +#define CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_MOCK_H_ + +#include "chrome/browser/sync/glue/device_info.h" +#include "chrome/browser/sync/glue/local_device_info_provider.h" + +namespace browser_sync { + +class LocalDeviceInfoProviderMock : public LocalDeviceInfoProvider { + public: + // Creates uninitialized provider. + LocalDeviceInfoProviderMock(); + // Creates initialized provider with the specified device info. + LocalDeviceInfoProviderMock( + const std::string& guid, + const std::string& client_name, + const std::string& chrome_version, + const std::string& sync_user_agent, + const sync_pb::SyncEnums::DeviceType device_type, + const std::string& signin_scoped_device_id); + virtual ~LocalDeviceInfoProviderMock(); + + virtual const DeviceInfo* GetLocalDeviceInfo() const OVERRIDE; + virtual std::string GetLocalSyncCacheGUID() const OVERRIDE; + virtual void Initialize( + const std::string& cache_guid, + const std::string& signin_scoped_device_id) OVERRIDE; + virtual scoped_ptr<Subscription> RegisterOnInitializedCallback( + const base::Closure& callback) OVERRIDE; + + void SetInitialized(bool is_initialized); + + private: + bool is_initialized_; + + scoped_ptr<DeviceInfo> local_device_info_; + base::CallbackList<void(void)> callback_list_; +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_MOCK_H_ diff --git a/chrome/browser/sync/glue/local_device_info_provider_unittest.cc b/chrome/browser/sync/glue/local_device_info_provider_unittest.cc new file mode 100644 index 0000000..d1fb4eb --- /dev/null +++ b/chrome/browser/sync/glue/local_device_info_provider_unittest.cc @@ -0,0 +1,85 @@ +// Copyright 2014 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/message_loop/message_loop.h" +#include "base/run_loop.h" +#include "chrome/browser/sync/glue/local_device_info_provider_impl.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace browser_sync { + +const char kLocalDeviceGuid[] = "foo"; +const char kSigninScopedDeviceId[] = "device_id"; + +class LocalDeviceInfoProviderTest : public testing::Test { + public: + LocalDeviceInfoProviderTest() + : called_back_(false) {} + virtual ~LocalDeviceInfoProviderTest() {} + + virtual void SetUp() OVERRIDE { + provider_.reset(new LocalDeviceInfoProviderImpl()); + } + + virtual void TearDown() OVERRIDE { + provider_.reset(); + called_back_ = false; + } + + protected: + void InitializeProvider() { + // Start initialization. + provider_->Initialize(kLocalDeviceGuid, kSigninScopedDeviceId); + + // Subscribe to the notification and wait until the callback + // is called. The callback will quit the loop. + base::RunLoop run_loop; + scoped_ptr<LocalDeviceInfoProvider::Subscription> subscription( + provider_->RegisterOnInitializedCallback( + base::Bind(&LocalDeviceInfoProviderTest::QuitLoopOnInitialized, + base::Unretained(this), &run_loop))); + run_loop.Run(); + } + + void QuitLoopOnInitialized(base::RunLoop* loop) { + called_back_ = true; + loop->Quit(); + } + + scoped_ptr<LocalDeviceInfoProviderImpl> provider_; + + bool called_back_; + + private: + base::MessageLoop message_loop_; +}; + +TEST_F(LocalDeviceInfoProviderTest, OnInitializedCallback) { + ASSERT_FALSE(called_back_); + + InitializeProvider(); + EXPECT_TRUE(called_back_); +} + +TEST_F(LocalDeviceInfoProviderTest, GetLocalDeviceInfo) { + ASSERT_EQ(NULL, provider_->GetLocalDeviceInfo()); + + InitializeProvider(); + + const DeviceInfo* local_device_info = provider_->GetLocalDeviceInfo(); + EXPECT_TRUE(!!local_device_info); + EXPECT_EQ(std::string(kLocalDeviceGuid), local_device_info->guid()); + EXPECT_EQ(std::string(kSigninScopedDeviceId), + local_device_info->signin_scoped_device_id()); +} + +TEST_F(LocalDeviceInfoProviderTest, GetLocalSyncCacheGUID) { + ASSERT_EQ(std::string(), provider_->GetLocalSyncCacheGUID()); + + InitializeProvider(); + + EXPECT_EQ(std::string(kLocalDeviceGuid), provider_->GetLocalSyncCacheGUID()); +} + +} // namespace browser_sync diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc index 6799f9c..d7494ff 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc @@ -120,8 +120,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { virtual void SetUp() { db_thread_.Start(); - profile_sync_factory_.reset( - new StrictMock<ProfileSyncComponentsFactoryMock>()); + profile_sync_factory_.reset(new ProfileSyncComponentsFactoryMock()); // All of these are refcounted, so don't need to be released. dtc_mock_ = new StrictMock<NonFrontendDataTypeControllerMock>(); diff --git a/chrome/browser/sync/glue/sync_backend_host_core.cc b/chrome/browser/sync/glue/sync_backend_host_core.cc index 0243dd0..d528ec4 100644 --- a/chrome/browser/sync/glue/sync_backend_host_core.cc +++ b/chrome/browser/sync/glue/sync_backend_host_core.cc @@ -526,7 +526,8 @@ void SyncBackendHostCore::DoFinishInitialProcessControlTypes() { &SyncBackendHostImpl::HandleInitializationSuccessOnFrontendLoop, js_backend_, debug_info_listener_, - sync_manager_->GetSyncContextProxy()); + sync_manager_->GetSyncContextProxy(), + sync_manager_->cache_guid()); js_backend_.Reset(); debug_info_listener_.Reset(); diff --git a/chrome/browser/sync/glue/sync_backend_host_impl.cc b/chrome/browser/sync/glue/sync_backend_host_impl.cc index 1f500ee..ea2a399 100644 --- a/chrome/browser/sync/glue/sync_backend_host_impl.cc +++ b/chrome/browser/sync/glue/sync_backend_host_impl.cc @@ -651,7 +651,8 @@ void SyncBackendHostImpl::HandleInitializationSuccessOnFrontendLoop( const syncer::WeakHandle<syncer::JsBackend> js_backend, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener> debug_info_listener, - syncer::SyncContextProxy* sync_context_proxy) { + syncer::SyncContextProxy* sync_context_proxy, + const std::string& cache_guid) { DCHECK_EQ(base::MessageLoop::current(), frontend_loop_); if (sync_context_proxy) @@ -681,6 +682,7 @@ void SyncBackendHostImpl::HandleInitializationSuccessOnFrontendLoop( AddExperimentalTypes(); frontend_->OnBackendInitialized(js_backend, debug_info_listener, + cache_guid, true); } @@ -692,6 +694,7 @@ void SyncBackendHostImpl::HandleInitializationFailureOnFrontendLoop() { frontend_->OnBackendInitialized( syncer::WeakHandle<syncer::JsBackend>(), syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + "", false); } diff --git a/chrome/browser/sync/glue/sync_backend_host_impl.h b/chrome/browser/sync/glue/sync_backend_host_impl.h index 629722a..4d53c7c 100644 --- a/chrome/browser/sync/glue/sync_backend_host_impl.h +++ b/chrome/browser/sync/glue/sync_backend_host_impl.h @@ -177,7 +177,8 @@ class SyncBackendHostImpl const syncer::WeakHandle<syncer::JsBackend> js_backend, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener> debug_info_listener, - syncer::SyncContextProxy* sync_context_proxy); + syncer::SyncContextProxy* sync_context_proxy, + const std::string& cache_guid); // Downloading of control types failed and will be retried. Invokes the // frontend's sync configure retry method. diff --git a/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc index a41e111..ed3827e 100644 --- a/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc @@ -76,10 +76,11 @@ class MockSyncFrontend : public SyncFrontend { public: virtual ~MockSyncFrontend() {} - MOCK_METHOD3( + MOCK_METHOD4( OnBackendInitialized, void(const syncer::WeakHandle<syncer::JsBackend>&, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&, + const std::string&, bool)); MOCK_METHOD0(OnSyncCycleCompleted, void()); MOCK_METHOD1(OnConnectionStatusChange, @@ -207,7 +208,7 @@ class SyncBackendHostTest : public testing::Test { // Synchronously initializes the backend. void InitializeBackend(bool expect_success) { - EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, _, expect_success)). + EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, _, _, expect_success)). WillOnce(InvokeWithoutArgs(QuitMessageLoop)); backend_->Initialize( &mock_frontend_, diff --git a/chrome/browser/sync/glue/sync_backend_host_mock.cc b/chrome/browser/sync/glue/sync_backend_host_mock.cc index 7a0eeeb..38f3ced 100644 --- a/chrome/browser/sync/glue/sync_backend_host_mock.cc +++ b/chrome/browser/sync/glue/sync_backend_host_mock.cc @@ -8,6 +8,8 @@ namespace browser_sync { +const char kTestCacheGuid[] = "test-guid"; + SyncBackendHostMock::SyncBackendHostMock() : fail_initial_download_(false) {} SyncBackendHostMock::~SyncBackendHostMock() {} @@ -26,6 +28,7 @@ void SyncBackendHostMock::Initialize( frontend->OnBackendInitialized( syncer::WeakHandle<syncer::JsBackend>(), syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + kTestCacheGuid, !fail_initial_download_); } diff --git a/chrome/browser/sync/profile_sync_components_factory.h b/chrome/browser/sync/profile_sync_components_factory.h index db958f6..94c38ab 100644 --- a/chrome/browser/sync/profile_sync_components_factory.h +++ b/chrome/browser/sync/profile_sync_components_factory.h @@ -29,6 +29,7 @@ class DataTypeManager; class DataTypeManagerObserver; class FailedDataTypesHandler; class GenericChangeProcessor; +class LocalDeviceInfoProvider; class SyncBackendHost; class DataTypeErrorHandler; } // namespace browser_sync @@ -97,6 +98,10 @@ class ProfileSyncComponentsFactory const base::WeakPtr<sync_driver::SyncPrefs>& sync_prefs, const base::FilePath& sync_folder) = 0; + // Creating this in the factory helps us mock it out in testing. + virtual scoped_ptr<browser_sync::LocalDeviceInfoProvider> + CreateLocalDeviceInfoProvider() = 0; + // Legacy datatypes that need to be converted to the SyncableService API. virtual SyncComponents CreateBookmarkSyncComponents( ProfileSyncService* profile_sync_service, diff --git a/chrome/browser/sync/profile_sync_components_factory_impl.cc b/chrome/browser/sync/profile_sync_components_factory_impl.cc index 8cc64a3..c26ee51 100644 --- a/chrome/browser/sync/profile_sync_components_factory_impl.cc +++ b/chrome/browser/sync/profile_sync_components_factory_impl.cc @@ -27,6 +27,7 @@ #include "chrome/browser/sync/glue/extension_backed_data_type_controller.h" #include "chrome/browser/sync/glue/extension_data_type_controller.h" #include "chrome/browser/sync/glue/extension_setting_data_type_controller.h" +#include "chrome/browser/sync/glue/local_device_info_provider_impl.h" #include "chrome/browser/sync/glue/password_data_type_controller.h" #include "chrome/browser/sync/glue/search_engine_data_type_controller.h" #include "chrome/browser/sync/glue/sync_backend_host.h" @@ -252,7 +253,11 @@ void ProfileSyncComponentsFactoryImpl::RegisterCommonDataTypes( syncer::PROXY_TABS)); pss->RegisterDataTypeController( new SessionDataTypeController( - this, profile_, MakeDisableCallbackFor(syncer::SESSIONS))); + this, + profile_, + pss->GetSyncedWindowDelegatesGetter(), + pss->GetLocalDeviceInfoProvider(), + MakeDisableCallbackFor(syncer::SESSIONS))); } // Favicon sync is enabled by default. Register unless explicitly disabled. @@ -469,6 +474,12 @@ ProfileSyncComponentsFactoryImpl::CreateSyncBackendHost( sync_prefs, sync_folder); } +scoped_ptr<browser_sync::LocalDeviceInfoProvider> +ProfileSyncComponentsFactoryImpl::CreateLocalDeviceInfoProvider() { + return scoped_ptr<browser_sync::LocalDeviceInfoProvider>( + new browser_sync::LocalDeviceInfoProviderImpl()); +} + base::WeakPtr<syncer::SyncableService> ProfileSyncComponentsFactoryImpl:: GetSyncableServiceForType(syncer::ModelType type) { if (!profile_) { // For tests. diff --git a/chrome/browser/sync/profile_sync_components_factory_impl.h b/chrome/browser/sync/profile_sync_components_factory_impl.h index 1307a38..20eafa3 100644 --- a/chrome/browser/sync/profile_sync_components_factory_impl.h +++ b/chrome/browser/sync/profile_sync_components_factory_impl.h @@ -61,6 +61,9 @@ class ProfileSyncComponentsFactoryImpl : public ProfileSyncComponentsFactory { const base::WeakPtr<sync_driver::SyncPrefs>& sync_prefs, const base::FilePath& sync_folder) OVERRIDE; + virtual scoped_ptr<browser_sync::LocalDeviceInfoProvider> + CreateLocalDeviceInfoProvider() OVERRIDE; + virtual base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType( syncer::ModelType type) OVERRIDE; virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService( diff --git a/chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc b/chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc index ea0b1d9..f016ae4 100644 --- a/chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc +++ b/chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc @@ -106,12 +106,13 @@ class ProfileSyncComponentsFactoryImplTest : public testing::Test { ProfileOAuth2TokenService* token_service = ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()); scoped_ptr<ProfileSyncService> pss(new ProfileSyncService( - new ProfileSyncComponentsFactoryImpl( - profile_.get(), - command_line_.get(), - ProfileSyncService::GetSyncServiceURL(*command_line_), - token_service, - profile_->GetRequestContext()), + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryImpl( + profile_.get(), + command_line_.get(), + ProfileSyncService::GetSyncServiceURL(*command_line_), + token_service, + profile_->GetRequestContext())), profile_.get(), make_scoped_ptr<SupervisedUserSigninManagerWrapper>(NULL), token_service, @@ -133,12 +134,13 @@ TEST_F(ProfileSyncComponentsFactoryImplTest, CreatePSSDefault) { ProfileOAuth2TokenService* token_service = ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()); scoped_ptr<ProfileSyncService> pss(new ProfileSyncService( - new ProfileSyncComponentsFactoryImpl( - profile_.get(), - command_line_.get(), - ProfileSyncService::GetSyncServiceURL(*command_line_), - token_service, - profile_->GetRequestContext()), + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryImpl( + profile_.get(), + command_line_.get(), + ProfileSyncService::GetSyncServiceURL(*command_line_), + token_service, + profile_->GetRequestContext())), profile_.get(), make_scoped_ptr<SupervisedUserSigninManagerWrapper>(NULL), token_service, diff --git a/chrome/browser/sync/profile_sync_components_factory_mock.cc b/chrome/browser/sync/profile_sync_components_factory_mock.cc index 2ed6f14..b3f576e 100644 --- a/chrome/browser/sync/profile_sync_components_factory_mock.cc +++ b/chrome/browser/sync/profile_sync_components_factory_mock.cc @@ -3,6 +3,8 @@ // found in the LICENSE file. #include "chrome/browser/sync/profile_sync_components_factory_mock.h" + +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" #include "components/sync_driver/change_processor.h" #include "components/sync_driver/model_associator.h" #include "content/public/browser/browser_thread.h" @@ -13,13 +15,17 @@ using browser_sync::AssociatorInterface; using browser_sync::ChangeProcessor; using testing::_; using testing::InvokeWithoutArgs; +using testing::Return; -ProfileSyncComponentsFactoryMock::ProfileSyncComponentsFactoryMock() {} +ProfileSyncComponentsFactoryMock::ProfileSyncComponentsFactoryMock() + : local_device_(new browser_sync::LocalDeviceInfoProviderMock()) { +} ProfileSyncComponentsFactoryMock::ProfileSyncComponentsFactoryMock( AssociatorInterface* model_associator, ChangeProcessor* change_processor) : model_associator_(model_associator), - change_processor_(change_processor) { + change_processor_(change_processor), + local_device_(new browser_sync::LocalDeviceInfoProviderMock()) { ON_CALL(*this, CreateBookmarkSyncComponents(_, _)). WillByDefault( InvokeWithoutArgs( @@ -41,3 +47,13 @@ ProfileSyncComponentsFactoryMock::MakeSyncComponents() { return SyncComponents(model_associator_.release(), change_processor_.release()); } + +scoped_ptr<browser_sync::LocalDeviceInfoProvider> +ProfileSyncComponentsFactoryMock::CreateLocalDeviceInfoProvider() { + return local_device_.Pass(); +} + +void ProfileSyncComponentsFactoryMock::SetLocalDeviceInfoProvider( + scoped_ptr<browser_sync::LocalDeviceInfoProvider> local_device) { + local_device_ = local_device.Pass(); +} diff --git a/chrome/browser/sync/profile_sync_components_factory_mock.h b/chrome/browser/sync/profile_sync_components_factory_mock.h index 8a7f767..b5a68cd 100644 --- a/chrome/browser/sync/profile_sync_components_factory_mock.h +++ b/chrome/browser/sync/profile_sync_components_factory_mock.h @@ -44,6 +44,12 @@ class ProfileSyncComponentsFactoryMock : public ProfileSyncComponentsFactory { invalidation::InvalidationService* invalidator, const base::WeakPtr<sync_driver::SyncPrefs>& sync_prefs, const base::FilePath& sync_folder)); + + virtual scoped_ptr<browser_sync::LocalDeviceInfoProvider> + CreateLocalDeviceInfoProvider() OVERRIDE; + void SetLocalDeviceInfoProvider( + scoped_ptr<browser_sync::LocalDeviceInfoProvider> local_device); + MOCK_METHOD1(GetSyncableServiceForType, base::WeakPtr<syncer::SyncableService>(syncer::ModelType)); virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService( @@ -68,6 +74,9 @@ class ProfileSyncComponentsFactoryMock : public ProfileSyncComponentsFactory { scoped_ptr<browser_sync::AssociatorInterface> model_associator_; scoped_ptr<browser_sync::ChangeProcessor> change_processor_; + // LocalDeviceInfoProvider is initially owned by this class, + // transferred to caller when CreateLocalDeviceInfoProvider is called. + scoped_ptr<browser_sync::LocalDeviceInfoProvider> local_device_; }; #endif // CHROME_BROWSER_SYNC_PROFILE_SYNC_COMPONENTS_FACTORY_MOCK_H__ diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index bb6d595..636d61f 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -38,6 +38,7 @@ #include "chrome/browser/services/gcm/gcm_profile_service.h" #include "chrome/browser/services/gcm/gcm_profile_service_factory.h" #include "chrome/browser/signin/about_signin_internals_factory.h" +#include "chrome/browser/signin/chrome_signin_client_factory.h" #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/sync/backend_migrator.h" @@ -188,7 +189,7 @@ bool ShouldShowActionOnUI( } ProfileSyncService::ProfileSyncService( - ProfileSyncComponentsFactory* factory, + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile, scoped_ptr<SupervisedUserSigninManagerWrapper> signin_wrapper, ProfileOAuth2TokenService* oauth2_token_service, @@ -196,7 +197,7 @@ ProfileSyncService::ProfileSyncService( : OAuth2TokenService::Consumer("sync"), last_auth_error_(AuthError::AuthErrorNone()), passphrase_required_reason_(syncer::REASON_PASSPHRASE_NOT_REQUIRED), - factory_(factory), + factory_(factory.Pass()), profile_(profile), sync_prefs_(profile_->GetPrefs()), sync_service_url_(GetSyncServiceURL(*CommandLine::ForCurrentProcess())), @@ -243,8 +244,11 @@ ProfileSyncService::ProfileSyncService( sync_start_util::GetFlareForSyncableService(profile->GetPath())); scoped_ptr<browser_sync::LocalSessionEventRouter> router( new NotificationServiceSessionsRouter(profile, flare)); + + DCHECK(factory_.get()); + local_device_ = factory_->CreateLocalDeviceInfoProvider(); sessions_sync_manager_.reset( - new SessionsSyncManager(profile, this, router.Pass())); + new SessionsSyncManager(profile, local_device_.get(), router.Pass())); } ProfileSyncService::~ProfileSyncService() { @@ -424,15 +428,14 @@ browser_sync::FaviconCache* ProfileSyncService::GetFaviconCache() { return sessions_sync_manager_->GetFaviconCache(); } -scoped_ptr<browser_sync::DeviceInfo> -ProfileSyncService::GetLocalDeviceInfo() const { - if (HasSyncingBackend()) { - browser_sync::SyncedDeviceTracker* device_tracker = - backend_->GetSyncedDeviceTracker(); - if (device_tracker) - return device_tracker->ReadLocalDeviceInfo(); - } - return scoped_ptr<browser_sync::DeviceInfo>(); +browser_sync::SyncedWindowDelegatesGetter* +ProfileSyncService::GetSyncedWindowDelegatesGetter() const { + return sessions_sync_manager_->GetSyncedWindowDelegatesGetter(); +} + +browser_sync::LocalDeviceInfoProvider* +ProfileSyncService::GetLocalDeviceInfoProvider() { + return local_device_.get(); } scoped_ptr<browser_sync::DeviceInfo> @@ -460,17 +463,6 @@ ScopedVector<browser_sync::DeviceInfo> return devices.Pass(); } -std::string ProfileSyncService::GetLocalSyncCacheGUID() const { - if (HasSyncingBackend()) { - browser_sync::SyncedDeviceTracker* device_tracker = - backend_->GetSyncedDeviceTracker(); - if (device_tracker) { - return device_tracker->cache_guid(); - } - } - return std::string(); -} - // Notifies the observer of any device info changes. void ProfileSyncService::AddObserverForDeviceInfoChange( browser_sync::SyncedDeviceTracker::Observer* observer) { @@ -1071,6 +1063,7 @@ void ProfileSyncService::OnBackendInitialized( const syncer::WeakHandle<syncer::JsBackend>& js_backend, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>& debug_info_listener, + const std::string& cache_guid, bool success) { UpdateBackendInitUMA(success); @@ -1099,6 +1092,15 @@ void ProfileSyncService::OnBackendInitialized( sync_js_controller_.AttachJsBackend(js_backend); debug_info_listener_ = debug_info_listener; + SigninClient* signin_client = + ChromeSigninClientFactory::GetForProfile(profile_); + DCHECK(signin_client); + std::string signin_scoped_device_id = + signin_client->GetSigninScopedDeviceId(); + + // Initialize local device info. + local_device_->Initialize(cache_guid, signin_scoped_device_id); + // Give the DataTypeControllers a handle to the now initialized backend // as a UserShare. for (DataTypeController::TypeMap::iterator it = diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 90a9af5..a37d9a4 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -22,6 +22,7 @@ #include "base/timer/timer.h" #include "chrome/browser/sync/backend_unrecoverable_error_handler.h" #include "chrome/browser/sync/backup_rollback_controller.h" +#include "chrome/browser/sync/glue/local_device_info_provider.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/glue/synced_device_tracker.h" #include "chrome/browser/sync/profile_sync_service_base.h" @@ -88,6 +89,7 @@ namespace sync_pb { class EncryptedData; } // namespace sync_pb +using browser_sync::LocalDeviceInfoProvider; using browser_sync::SessionsSyncManager; // ProfileSyncService is the layer between browser subsystems like bookmarks, @@ -182,7 +184,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, public browser_sync::DataTypeEncryptionHandler, public OAuth2TokenService::Consumer, public OAuth2TokenService::Observer, - public SessionsSyncManager::SyncInternalApiDelegate, public SigninManagerBase::Observer { public: typedef browser_sync::SyncBackendHost::Status Status; @@ -265,7 +266,7 @@ class ProfileSyncService : public ProfileSyncServiceBase, // Takes ownership of |factory| and |signin_wrapper|. ProfileSyncService( - ProfileSyncComponentsFactory* factory, + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile, scoped_ptr<SupervisedUserSigninManagerWrapper> signin_wrapper, ProfileOAuth2TokenService* oauth2_token_service, @@ -351,22 +352,15 @@ class ProfileSyncService : public ProfileSyncServiceBase, // currently syncing, returns NULL. virtual browser_sync::OpenTabsUIDelegate* GetOpenTabsUIDelegate(); + // Returns the SyncedWindowDelegatesGetter from the embedded sessions manager. + virtual browser_sync::SyncedWindowDelegatesGetter* + GetSyncedWindowDelegatesGetter() const; + // Returns the SyncableService for syncer::SESSIONS. virtual syncer::SyncableService* GetSessionsSyncableService(); - // SyncInternalApiDelegate implementation. - // - // Returns sync's representation of the local device info. - // Return value is an empty scoped_ptr if the device info is unavailable. - virtual scoped_ptr<browser_sync::DeviceInfo> GetLocalDeviceInfo() - const OVERRIDE; - - // Gets the guid for the local device. Can be used by other layers to - // to distinguish sync data that belongs to the local device vs data - // that belongs to remote devices. Returns empty string if sync is not - // initialized. The GUID is not persistent across Chrome signout/signin. - // If you sign out of Chrome and sign in, a new GUID is generated. - virtual std::string GetLocalSyncCacheGUID() const OVERRIDE; + // Returns DeviceInfo provider for the local device. + virtual browser_sync::LocalDeviceInfoProvider* GetLocalDeviceInfoProvider(); // Returns sync's representation of the device info for a client identified // by |client_id|. Return value is an empty scoped ptr if the device info @@ -402,6 +396,7 @@ class ProfileSyncService : public ProfileSyncServiceBase, const syncer::WeakHandle<syncer::JsBackend>& js_backend, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>& debug_info_listener, + const std::string& cache_guid, bool success) OVERRIDE; virtual void OnSyncCycleCompleted() OVERRIDE; virtual void OnProtocolEvent(const syncer::ProtocolEvent& event) OVERRIDE; @@ -1113,6 +1108,8 @@ class ProfileSyncService : public ProfileSyncServiceBase, GoogleServiceAuthError last_get_token_error_; base::Time next_token_request_time_; + scoped_ptr<LocalDeviceInfoProvider> local_device_; + // Locally owned SyncableService implementations. scoped_ptr<SessionsSyncManager> sessions_sync_manager_; diff --git a/chrome/browser/sync/profile_sync_service_factory.cc b/chrome/browser/sync/profile_sync_service_factory.cc index 95c911a..b4a0f8a 100644 --- a/chrome/browser/sync/profile_sync_service_factory.cc +++ b/chrome/browser/sync/profile_sync_service_factory.cc @@ -133,11 +133,13 @@ KeyedService* ProfileSyncServiceFactory::BuildServiceInstanceFor( browser_defaults::kSyncAutoStarts ? browser_sync::AUTO_START : browser_sync::MANUAL_START; ProfileSyncService* pss = new ProfileSyncService( - new ProfileSyncComponentsFactoryImpl(profile, - CommandLine::ForCurrentProcess(), - sync_service_url, - token_service, - url_request_context_getter), + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryImpl( + profile, + CommandLine::ForCurrentProcess(), + sync_service_url, + token_service, + url_request_context_getter)), profile, signin_wrapper.Pass(), token_service, diff --git a/chrome/browser/sync/profile_sync_service_mock.cc b/chrome/browser/sync/profile_sync_service_mock.cc index 3ab3903..7364111 100644 --- a/chrome/browser/sync/profile_sync_service_mock.cc +++ b/chrome/browser/sync/profile_sync_service_mock.cc @@ -8,6 +8,7 @@ #include "base/prefs/testing_pref_store.h" #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/supervised_user_signin_manager_wrapper.h" #include "chrome/common/pref_names.h" #include "chrome/common/url_constants.h" @@ -17,7 +18,20 @@ ProfileSyncServiceMock::ProfileSyncServiceMock(Profile* profile) : ProfileSyncService( - NULL, + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), + profile, + make_scoped_ptr(new SupervisedUserSigninManagerWrapper( + profile, + SigninManagerFactory::GetForProfile(profile))), + ProfileOAuth2TokenServiceFactory::GetForProfile(profile), + browser_sync::MANUAL_START) { +} + +ProfileSyncServiceMock::ProfileSyncServiceMock( + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile) + : ProfileSyncService( + factory.Pass(), profile, make_scoped_ptr(new SupervisedUserSigninManagerWrapper( profile, @@ -50,8 +64,3 @@ ScopedVector<browser_sync::DeviceInfo> devices.get() = *device_vector; return devices.Pass(); } - -scoped_ptr<browser_sync::DeviceInfo> - ProfileSyncServiceMock::GetLocalDeviceInfo() const { - return scoped_ptr<browser_sync::DeviceInfo>(GetLocalDeviceInfoMock()).Pass(); -} diff --git a/chrome/browser/sync/profile_sync_service_mock.h b/chrome/browser/sync/profile_sync_service_mock.h index 7eb01f0..b4795ae 100644 --- a/chrome/browser/sync/profile_sync_service_mock.h +++ b/chrome/browser/sync/profile_sync_service_mock.h @@ -25,6 +25,8 @@ using ::testing::Invoke; class ProfileSyncServiceMock : public ProfileSyncService { public: explicit ProfileSyncServiceMock(Profile* profile); + ProfileSyncServiceMock( + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile); virtual ~ProfileSyncServiceMock(); // A utility used by sync tests to create a TestingProfile with a Google @@ -37,9 +39,10 @@ class ProfileSyncServiceMock : public ProfileSyncService { content::BrowserContext* profile); MOCK_METHOD0(DisableForUser, void()); - MOCK_METHOD3(OnBackendInitialized, + MOCK_METHOD4(OnBackendInitialized, void(const syncer::WeakHandle<syncer::JsBackend>&, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&, + const std::string&, bool)); MOCK_METHOD0(OnSyncCycleCompleted, void()); MOCK_METHOD0(OnAuthError, void()); @@ -98,12 +101,6 @@ class ProfileSyncServiceMock : public ProfileSyncService { virtual ScopedVector<browser_sync::DeviceInfo> GetAllSignedInDevices() const OVERRIDE; - virtual scoped_ptr<browser_sync::DeviceInfo> GetLocalDeviceInfo() - const OVERRIDE; - MOCK_CONST_METHOD0(GetLocalDeviceInfoMock, - browser_sync::DeviceInfo*()); - MOCK_CONST_METHOD0(GetLocalSyncCacheGUID, std::string()); - // DataTypeManagerObserver mocks. MOCK_METHOD0(OnConfigureBlocked, void()); MOCK_METHOD1(OnConfigureDone, diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index 91ed7f8..c00315e 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -99,7 +99,8 @@ class ProfileSyncServiceStartupTest : public testing::Test { static KeyedService* BuildService(content::BrowserContext* browser_context) { Profile* profile = static_cast<Profile*>(browser_context); return new ProfileSyncService( - new ProfileSyncComponentsFactoryMock(), + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), profile, make_scoped_ptr(new SupervisedUserSigninManagerWrapper( profile, SigninManagerFactory::GetForProfile(profile))), @@ -183,7 +184,8 @@ class ProfileSyncServiceStartupCrosTest : public ProfileSyncServiceStartupTest { ProfileOAuth2TokenServiceFactory::GetForProfile(profile); EXPECT_FALSE(signin->GetAuthenticatedUsername().empty()); return new ProfileSyncService( - new ProfileSyncComponentsFactoryMock(), + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), profile, make_scoped_ptr(new SupervisedUserSigninManagerWrapper(profile, signin)), diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index f4879fb..412be89 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -193,9 +193,9 @@ class ProfileSyncServiceTest : public ::testing::Test { signin->SetAuthenticatedUsername("test"); ProfileOAuth2TokenService* oauth2_token_service = ProfileOAuth2TokenServiceFactory::GetForProfile(profile_); - components_factory_ = new StrictMock<ProfileSyncComponentsFactoryMock>(); + components_factory_ = new ProfileSyncComponentsFactoryMock(); service_.reset(new ProfileSyncService( - components_factory_, + scoped_ptr<ProfileSyncComponentsFactory>(components_factory_), profile_, make_scoped_ptr(new SupervisedUserSigninManagerWrapper(profile_, signin)), diff --git a/chrome/browser/sync/sessions/session_data_type_controller.cc b/chrome/browser/sync/sessions/session_data_type_controller.cc index 25578b6..6717f75 100644 --- a/chrome/browser/sync/sessions/session_data_type_controller.cc +++ b/chrome/browser/sync/sessions/session_data_type_controller.cc @@ -8,6 +8,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h" #include "chrome/browser/sync/glue/synced_window_delegate.h" +#include "chrome/browser/sync/sessions/synced_window_delegates_getter.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_service.h" @@ -20,6 +21,8 @@ namespace browser_sync { SessionDataTypeController::SessionDataTypeController( SyncApiComponentFactory* sync_factory, Profile* profile, + SyncedWindowDelegatesGetter* synced_window_getter, + LocalDeviceInfoProvider* local_device, const DisableTypeCallback& disable_callback) : UIDataTypeController( BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), @@ -27,7 +30,12 @@ SessionDataTypeController::SessionDataTypeController( disable_callback, syncer::SESSIONS, sync_factory), - profile_(profile) { + profile_(profile), + synced_window_getter_(synced_window_getter), + local_device_(local_device), + waiting_on_session_restore_(false), + waiting_on_local_device_info_(false) { + DCHECK(local_device_); } SessionDataTypeController::~SessionDataTypeController() {} @@ -35,7 +43,7 @@ SessionDataTypeController::~SessionDataTypeController() {} bool SessionDataTypeController::StartModels() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); std::set<browser_sync::SyncedWindowDelegate*> window = - browser_sync::SyncedWindowDelegate::GetSyncedWindowDelegates(); + synced_window_getter_->GetSyncedWindowDelegates(); for (std::set<browser_sync::SyncedWindowDelegate*>::const_iterator i = window.begin(); i != window.end(); ++i) { if ((*i)->IsSessionRestoreInProgress()) { @@ -43,16 +51,35 @@ bool SessionDataTypeController::StartModels() { this, chrome::NOTIFICATION_SESSION_RESTORE_COMPLETE, content::Source<Profile>(profile_)); - return false; + waiting_on_session_restore_ = true; + break; } } - return true; + + if (!local_device_->GetLocalDeviceInfo()) { + subscription_ = local_device_->RegisterOnInitializedCallback( + base::Bind(&SessionDataTypeController::OnLocalDeviceInfoInitialized, + this)); + waiting_on_local_device_info_ = true; + } + + return !IsWaiting(); } void SessionDataTypeController::StopModels() { notification_registrar_.RemoveAll(); } +bool SessionDataTypeController::IsWaiting() { + return waiting_on_session_restore_ || waiting_on_local_device_info_; +} + +void SessionDataTypeController::MaybeCompleteLoading() { + if (state_ == MODEL_STARTING && !IsWaiting()) { + OnModelLoaded(); + } +} + void SessionDataTypeController::Observe( int type, const content::NotificationSource& source, @@ -61,7 +88,17 @@ void SessionDataTypeController::Observe( DCHECK_EQ(chrome::NOTIFICATION_SESSION_RESTORE_COMPLETE, type); DCHECK_EQ(profile_, content::Source<Profile>(source).ptr()); notification_registrar_.RemoveAll(); - OnModelLoaded(); + + waiting_on_session_restore_ = false; + MaybeCompleteLoading(); +} + +void SessionDataTypeController::OnLocalDeviceInfoInitialized() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + subscription_.reset(); + + waiting_on_local_device_info_ = false; + MaybeCompleteLoading(); } } // namespace browser_sync diff --git a/chrome/browser/sync/sessions/session_data_type_controller.h b/chrome/browser/sync/sessions/session_data_type_controller.h index d5ede14..846c23f 100644 --- a/chrome/browser/sync/sessions/session_data_type_controller.h +++ b/chrome/browser/sync/sessions/session_data_type_controller.h @@ -5,6 +5,7 @@ #ifndef CHROME_BROWSER_SYNC_SESSIONS_SESSION_DATA_TYPE_CONTROLLER_H_ #define CHROME_BROWSER_SYNC_SESSIONS_SESSION_DATA_TYPE_CONTROLLER_H_ +#include "chrome/browser/sync/glue/local_device_info_provider.h" #include "components/sync_driver/ui_data_type_controller.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" @@ -13,13 +14,18 @@ class Profile; namespace browser_sync { +class SyncedWindowDelegatesGetter; + // Overrides StartModels to avoid sync contention with sessions during -// a session restore operation at startup. +// a session restore operation at startup and to wait for the local +// device info to become available. class SessionDataTypeController : public UIDataTypeController, public content::NotificationObserver { public: SessionDataTypeController(SyncApiComponentFactory* factory, Profile* profile, + SyncedWindowDelegatesGetter* synced_window_getter, + LocalDeviceInfoProvider* local_device, const DisableTypeCallback& disable_callback); // NotificationObserver interface. @@ -33,8 +39,22 @@ class SessionDataTypeController : public UIDataTypeController, virtual void StopModels() OVERRIDE; private: + bool IsWaiting(); + void MaybeCompleteLoading(); + void OnLocalDeviceInfoInitialized(); + Profile* const profile_; + + SyncedWindowDelegatesGetter* synced_window_getter_; content::NotificationRegistrar notification_registrar_; + + LocalDeviceInfoProvider* const local_device_; + scoped_ptr<LocalDeviceInfoProvider::Subscription> subscription_; + + // Flags that indicate the reason for pending loading models. + bool waiting_on_session_restore_; + bool waiting_on_local_device_info_; + DISALLOW_COPY_AND_ASSIGN(SessionDataTypeController); }; diff --git a/chrome/browser/sync/sessions/session_data_type_controller_unittest.cc b/chrome/browser/sync/sessions/session_data_type_controller_unittest.cc new file mode 100644 index 0000000..90f08d0 --- /dev/null +++ b/chrome/browser/sync/sessions/session_data_type_controller_unittest.cc @@ -0,0 +1,234 @@ +// Copyright 2014 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/bind.h" +#include "base/callback.h" +#include "base/memory/weak_ptr.h" +#include "base/run_loop.h" +#include "chrome/browser/chrome_notification_types.h" +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" +#include "chrome/browser/sync/glue/synced_window_delegate.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" +#include "chrome/browser/sync/sessions/session_data_type_controller.h" +#include "chrome/browser/sync/sessions/synced_window_delegates_getter.h" +#include "chrome/test/base/testing_profile.h" +#include "content/public/browser/notification_service.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace browser_sync { + +namespace { + +class MockSyncedWindowDelegate : public SyncedWindowDelegate { + public: + explicit MockSyncedWindowDelegate(Profile* profile) + : is_restore_in_progress_(false), + profile_(profile) {} + virtual ~MockSyncedWindowDelegate() {} + + virtual bool HasWindow() const OVERRIDE { return false; } + virtual SessionID::id_type GetSessionId() const OVERRIDE { return 0; } + virtual int GetTabCount() const OVERRIDE { return 0; } + virtual int GetActiveIndex() const OVERRIDE { return 0; } + virtual bool IsApp() const OVERRIDE { return false; } + virtual bool IsTypeTabbed() const OVERRIDE { return false; } + virtual bool IsTypePopup() const OVERRIDE { return false; } + virtual bool IsTabPinned(const SyncedTabDelegate* tab) const OVERRIDE { + return false; + } + virtual SyncedTabDelegate* GetTabAt(int index) const OVERRIDE { return NULL; } + virtual SessionID::id_type GetTabIdAt(int index) const OVERRIDE { return 0; } + + virtual bool IsSessionRestoreInProgress() const OVERRIDE { + return is_restore_in_progress_; + } + + void SetSessionRestoreInProgress(bool is_restore_in_progress) { + is_restore_in_progress_ = is_restore_in_progress; + + if (!is_restore_in_progress_) { + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_SESSION_RESTORE_COMPLETE, + content::Source<Profile>(profile_), + content::NotificationService::NoDetails()); + } + } + + private: + bool is_restore_in_progress_; + Profile* profile_; +}; + +class MockSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter { + public: + virtual const std::set<SyncedWindowDelegate*> + GetSyncedWindowDelegates() OVERRIDE { + return delegates_; + } + + void Add(SyncedWindowDelegate* delegate) { + delegates_.insert(delegate); + } + + private: + std::set<SyncedWindowDelegate*> delegates_; +}; + +class SessionDataTypeControllerTest + : public testing::Test { + public: + SessionDataTypeControllerTest() + : load_finished_(false), + thread_bundle_(content::TestBrowserThreadBundle::DEFAULT), + weak_ptr_factory_(this), + last_type_(syncer::UNSPECIFIED) {} + virtual ~SessionDataTypeControllerTest() {} + + virtual void SetUp() OVERRIDE { + synced_window_delegate_.reset(new MockSyncedWindowDelegate(&profile_)); + synced_window_getter_.reset(new MockSyncedWindowDelegatesGetter()); + synced_window_getter_->Add(synced_window_delegate_.get()); + + local_device_.reset(new LocalDeviceInfoProviderMock( + "cache_guid", + "Wayne Gretzky's Hacking Box", + "Chromium 10k", + "Chrome 10k", + sync_pb::SyncEnums_DeviceType_TYPE_LINUX, + "device_id")); + + controller_ = new SessionDataTypeController( + &profile_sync_factory_, + &profile_, + synced_window_getter_.get(), + local_device_.get(), + DataTypeController::DisableTypeCallback()); + + load_finished_ = false; + last_type_ = syncer::UNSPECIFIED; + last_error_ = syncer::SyncError(); + } + + virtual void TearDown() OVERRIDE { + controller_ = NULL; + local_device_.reset(); + synced_window_getter_.reset(); + synced_window_delegate_.reset(); + } + + void Start() { + controller_->LoadModels( + base::Bind(&SessionDataTypeControllerTest::OnLoadFinished, + weak_ptr_factory_.GetWeakPtr())); + } + + void OnLoadFinished(syncer::ModelType type, syncer::SyncError error) { + load_finished_ = true; + last_type_ = type; + last_error_ = error; + } + + testing::AssertionResult LoadResult() { + if (!load_finished_) { + return testing::AssertionFailure() << + "OnLoadFinished wasn't called"; + } + + if (last_error_.IsSet()) { + return testing::AssertionFailure() << + "OnLoadFinished was called with a SyncError: " << + last_error_.ToString(); + } + + if (last_type_ != syncer::SESSIONS) { + return testing::AssertionFailure() << + "OnLoadFinished was called with a wrong sync type: " << + last_type_; + } + + return testing::AssertionSuccess(); + } + + protected: + scoped_refptr<SessionDataTypeController> controller_; + scoped_ptr<MockSyncedWindowDelegatesGetter> synced_window_getter_; + scoped_ptr<LocalDeviceInfoProviderMock> local_device_; + scoped_ptr<MockSyncedWindowDelegate> synced_window_delegate_; + bool load_finished_; + + private: + content::TestBrowserThreadBundle thread_bundle_; + ProfileSyncComponentsFactoryMock profile_sync_factory_; + TestingProfile profile_; + base::WeakPtrFactory<SessionDataTypeControllerTest> weak_ptr_factory_; + syncer::ModelType last_type_; + syncer::SyncError last_error_; +}; + +TEST_F(SessionDataTypeControllerTest, StartModels) { + Start(); + EXPECT_EQ(DataTypeController::MODEL_LOADED, controller_->state()); + EXPECT_TRUE(LoadResult()); +} + +TEST_F(SessionDataTypeControllerTest, StartModelsDelayedByLocalDevice) { + local_device_->SetInitialized(false); + Start(); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + local_device_->SetInitialized(true); + EXPECT_EQ(DataTypeController::MODEL_LOADED, controller_->state()); + EXPECT_TRUE(LoadResult()); +} + +TEST_F(SessionDataTypeControllerTest, StartModelsDelayedByRestoreInProgress) { + synced_window_delegate_->SetSessionRestoreInProgress(true); + Start(); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + synced_window_delegate_->SetSessionRestoreInProgress(false); + EXPECT_EQ(DataTypeController::MODEL_LOADED, controller_->state()); + EXPECT_TRUE(LoadResult()); +} + +TEST_F(SessionDataTypeControllerTest, + StartModelsDelayedByLocalDeviceThenRestoreInProgress) { + local_device_->SetInitialized(false); + synced_window_delegate_->SetSessionRestoreInProgress(true); + Start(); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + local_device_->SetInitialized(true); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + synced_window_delegate_->SetSessionRestoreInProgress(false); + EXPECT_EQ(DataTypeController::MODEL_LOADED, controller_->state()); + EXPECT_TRUE(LoadResult()); +} + +TEST_F(SessionDataTypeControllerTest, + StartModelsDelayedByRestoreInProgressThenLocalDevice) { + local_device_->SetInitialized(false); + synced_window_delegate_->SetSessionRestoreInProgress(true); + Start(); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + synced_window_delegate_->SetSessionRestoreInProgress(false); + EXPECT_FALSE(load_finished_); + EXPECT_EQ(DataTypeController::MODEL_STARTING, controller_->state()); + + local_device_->SetInitialized(true); + EXPECT_EQ(DataTypeController::MODEL_LOADED, controller_->state()); + EXPECT_TRUE(LoadResult()); +} + +} // namespace + +} // namespace browser_sync diff --git a/chrome/browser/sync/sessions/sessions_sync_manager.cc b/chrome/browser/sync/sessions/sessions_sync_manager.cc index 72a01d2..069f1f2 100644 --- a/chrome/browser/sync/sessions/sessions_sync_manager.cc +++ b/chrome/browser/sync/sessions/sessions_sync_manager.cc @@ -6,6 +6,7 @@ #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/glue/local_device_info_provider.h" #include "chrome/browser/sync/glue/synced_tab_delegate.h" #include "chrome/browser/sync/glue/synced_window_delegate.h" #include "chrome/browser/sync/sessions/sessions_util.h" @@ -44,15 +45,17 @@ static const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs"; // stale and becomes a candidate for garbage collection. static const size_t kDefaultStaleSessionThresholdDays = 14; // 2 weeks. +// |local_device| is owned by ProfileSyncService, its lifetime exceeds +// lifetime of SessionSyncManager. SessionsSyncManager::SessionsSyncManager( Profile* profile, - SyncInternalApiDelegate* delegate, + LocalDeviceInfoProvider* local_device, scoped_ptr<LocalSessionEventRouter> router) : favicon_cache_(profile, kMaxSyncFavicons), local_tab_pool_out_of_sync_(true), sync_prefs_(profile->GetPrefs()), profile_(profile), - delegate_(delegate), + local_device_(local_device), local_session_header_node_id_(TabNodePool::kInvalidTabNodeID), stale_session_threshold_days_(kDefaultStaleSessionThresholdDays), local_event_router_(router.Pass()), @@ -85,24 +88,31 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing( sync_processor_ = sync_processor.Pass(); local_session_header_node_id_ = TabNodePool::kInvalidTabNodeID; - scoped_ptr<DeviceInfo> local_device_info(delegate_->GetLocalDeviceInfo()); - syncer::SyncChangeList new_changes; // Make sure we have a machine tag. We do this now (versus earlier) as it's // a conveniently safe time to assert sync is ready and the cache_guid is // initialized. - if (current_machine_tag_.empty()) + if (current_machine_tag_.empty()) { InitializeCurrentMachineTag(); + } + + // SessionDataTypeController ensures that the local device info + // is available before activating this datatype. + DCHECK(local_device_); + const DeviceInfo* local_device_info = local_device_->GetLocalDeviceInfo(); if (local_device_info) { current_session_name_ = local_device_info->client_name(); } else { merge_result.set_error(error_handler_->CreateAndUploadError( FROM_HERE, - "Failed to get device info for machine tag.")); + "Failed to get local device info.")); return merge_result; } + session_tracker_.SetLocalSessionTag(current_machine_tag_); + syncer::SyncChangeList new_changes; + // First, we iterate over sync data to update our session_tracker_. syncer::SyncDataList restored_tabs; if (!InitFromSyncModel(initial_sync_data, &restored_tabs, &new_changes)) { @@ -121,7 +131,7 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing( #if defined(OS_ANDROID) std::string sync_machine_tag(BuildMachineTag( - delegate_->GetLocalSyncCacheGUID())); + local_device_->GetLocalSyncCacheGUID())); if (current_machine_tag_.compare(sync_machine_tag) != 0) DeleteForeignSessionInternal(sync_machine_tag, &new_changes); #endif @@ -639,7 +649,10 @@ void SessionsSyncManager::InitializeCurrentMachineTag() { current_machine_tag_ = persisted_guid; DVLOG(1) << "Restoring persisted session sync guid: " << persisted_guid; } else { - current_machine_tag_ = BuildMachineTag(delegate_->GetLocalSyncCacheGUID()); + DCHECK(local_device_); + std::string cache_guid = local_device_->GetLocalSyncCacheGUID(); + DCHECK(!cache_guid.empty()); + current_machine_tag_ = BuildMachineTag(cache_guid); DVLOG(1) << "Creating session sync guid: " << current_machine_tag_; sync_prefs_.SetSyncSessionsGUID(current_machine_tag_); } @@ -954,6 +967,11 @@ FaviconCache* SessionsSyncManager::GetFaviconCache() { return &favicon_cache_; } +SyncedWindowDelegatesGetter* +SessionsSyncManager::GetSyncedWindowDelegatesGetter() const { + return synced_window_getter_.get(); +} + void SessionsSyncManager::DoGarbageCollection() { std::vector<const SyncedSession*> sessions; if (!GetAllForeignSessions(&sessions)) diff --git a/chrome/browser/sync/sessions/sessions_sync_manager.h b/chrome/browser/sync/sessions/sessions_sync_manager.h index 51d9a1e..d94df57 100644 --- a/chrome/browser/sync/sessions/sessions_sync_manager.h +++ b/chrome/browser/sync/sessions/sessions_sync_manager.h @@ -43,6 +43,7 @@ class TabNavigation; namespace browser_sync { class DataTypeErrorHandler; +class LocalDeviceInfoProvider; class SyncedTabDelegate; class SyncedWindowDelegate; class SyncedWindowDelegatesGetter; @@ -82,21 +83,8 @@ class SessionsSyncManager : public syncer::SyncableService, public OpenTabsUIDelegate, public LocalSessionEventHandler { public: - // Isolates SessionsSyncManager from having to depend on sync internals. - class SyncInternalApiDelegate { - public: - virtual ~SyncInternalApiDelegate() {} - - // Returns sync's representation of the local device info. - // Return value is an empty scoped_ptr if the device info is unavailable. - virtual scoped_ptr<DeviceInfo> GetLocalDeviceInfo() const = 0; - - // Used for creation of the machine tag for this local session. - virtual std::string GetLocalSyncCacheGUID() const = 0; - }; - SessionsSyncManager(Profile* profile, - SyncInternalApiDelegate* delegate, + LocalDeviceInfoProvider* local_device, scoped_ptr<LocalSessionEventRouter> router); virtual ~SessionsSyncManager(); @@ -148,6 +136,8 @@ class SessionsSyncManager : public syncer::SyncableService, FaviconCache* GetFaviconCache(); + SyncedWindowDelegatesGetter* GetSyncedWindowDelegatesGetter() const; + // Triggers garbage collection of stale sessions (as defined by // |stale_session_threshold_days_|). This is called automatically every // time we start up (via AssociateModels) and when new sessions data is @@ -354,7 +344,8 @@ class SessionsSyncManager : public syncer::SyncableService, scoped_ptr<syncer::SyncErrorFactory> error_handler_; scoped_ptr<syncer::SyncChangeProcessor> sync_processor_; - const SyncInternalApiDelegate* const delegate_; + // Local device info provider, owned by ProfileSyncService. + const LocalDeviceInfoProvider* const local_device_; // Unique client tag. std::string current_machine_tag_; diff --git a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc index e06ad8c..ccac778 100644 --- a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc +++ b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc @@ -10,6 +10,7 @@ #include "chrome/browser/sessions/session_tab_helper.h" #include "chrome/browser/sessions/session_types.h" #include "chrome/browser/sync/glue/device_info.h" +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" #include "chrome/browser/sync/glue/session_sync_test_helper.h" #include "chrome/browser/sync/glue/synced_tab_delegate.h" #include "chrome/browser/sync/glue/synced_window_delegate.h" @@ -228,17 +229,25 @@ scoped_ptr<LocalSessionEventRouter> NewDummyRouter() { } // namespace class SessionsSyncManagerTest - : public BrowserWithTestWindowTest, - public SessionsSyncManager::SyncInternalApiDelegate { + : public BrowserWithTestWindowTest { public: - SessionsSyncManagerTest() : test_processor_(NULL) {} + SessionsSyncManagerTest() + : test_processor_(NULL) { + local_device_.reset(new LocalDeviceInfoProviderMock( + "cache_guid", + "Wayne Gretzky's Hacking Box", + "Chromium 10k", + "Chrome 10k", + sync_pb::SyncEnums_DeviceType_TYPE_LINUX, + "device_id")); + } virtual void SetUp() OVERRIDE { BrowserWithTestWindowTest::SetUp(); browser_sync::NotificationServiceSessionsRouter* router( new browser_sync::NotificationServiceSessionsRouter( profile(), syncer::SyncableService::StartSyncFlare())); - manager_.reset(new SessionsSyncManager(profile(), this, + manager_.reset(new SessionsSyncManager(profile(), local_device_.get(), scoped_ptr<LocalSessionEventRouter>(router))); } @@ -249,22 +258,13 @@ class SessionsSyncManagerTest BrowserWithTestWindowTest::TearDown(); } - virtual scoped_ptr<DeviceInfo> GetLocalDeviceInfo() const OVERRIDE { - return scoped_ptr<DeviceInfo>( - new DeviceInfo(GetLocalSyncCacheGUID(), - "Wayne Gretzky's Hacking Box", - "Chromium 10k", - "Chrome 10k", - sync_pb::SyncEnums_DeviceType_TYPE_LINUX, - "device_id")); - } - - virtual std::string GetLocalSyncCacheGUID() const OVERRIDE { - return "cache_guid"; + const DeviceInfo* GetLocalDeviceInfo() { + return local_device_->GetLocalDeviceInfo(); } SessionsSyncManager* manager() { return manager_.get(); } SessionSyncTestHelper* helper() { return &helper_; } + LocalDeviceInfoProvider* local_device() { return local_device_.get(); } void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data, syncer::SyncChangeList* output) { @@ -314,6 +314,7 @@ class SessionsSyncManagerTest scoped_ptr<SessionsSyncManager> manager_; SessionSyncTestHelper helper_; TestSyncProcessorStub* test_processor_; + scoped_ptr<LocalDeviceInfoProviderMock> local_device_; }; // Test that the SyncSessionManager can properly fill in a SessionHeader. @@ -686,7 +687,7 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionNoTabs) { syncer::AttachmentServiceProxyForTest::Create())); syncer::SyncDataList in(&d, &d + 1); out.clear(); - SessionsSyncManager manager2(profile(), this, NewDummyRouter()); + SessionsSyncManager manager2(profile(), local_device(), NewDummyRouter()); syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing( syncer::SESSIONS, in, scoped_ptr<syncer::SyncChangeProcessor>( @@ -1274,7 +1275,7 @@ TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) { syncer::AttachmentServiceProxyForTest::Create())); syncer::SyncDataList in(&d, &d + 1); changes.clear(); - SessionsSyncManager manager2(profile(), this, NewDummyRouter()); + SessionsSyncManager manager2(profile(), local_device(), NewDummyRouter()); syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing( syncer::SESSIONS, in, scoped_ptr<syncer::SyncChangeProcessor>( diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 3af8872..81a9220 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -103,13 +103,13 @@ TestProfileSyncService::GetJsEventHandler() { } TestProfileSyncService::TestProfileSyncService( - ProfileSyncComponentsFactory* factory, + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile, SigninManagerBase* signin, ProfileOAuth2TokenService* oauth2_token_service, browser_sync::ProfileSyncServiceStartBehavior behavior) : ProfileSyncService( - factory, + factory.Pass(), profile, make_scoped_ptr(new SupervisedUserSigninManagerWrapper(profile, signin)), @@ -129,13 +129,13 @@ KeyedService* TestProfileSyncService::TestFactoryFunction( SigninManagerFactory::GetForProfile(profile); ProfileOAuth2TokenService* oauth2_token_service = ProfileOAuth2TokenServiceFactory::GetForProfile(profile); - ProfileSyncComponentsFactoryMock* factory = - new ProfileSyncComponentsFactoryMock(); - return new TestProfileSyncService(factory, - profile, - signin, - oauth2_token_service, - browser_sync::AUTO_START); + return new TestProfileSyncService( + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), + profile, + signin, + oauth2_token_service, + browser_sync::AUTO_START); } // static diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index 733bd32..09dc1c6 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -73,7 +73,7 @@ class TestProfileSyncService : public ProfileSyncService { // TODO(tim): Add ability to inject TokenService alongside SigninManager. // TODO(rogerta): what does above comment mean? TestProfileSyncService( - ProfileSyncComponentsFactory* factory, + scoped_ptr<ProfileSyncComponentsFactory> factory, Profile* profile, SigninManagerBase* signin, ProfileOAuth2TokenService* oauth2_token_service, diff --git a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm index 3b94945..b90ab91 100644 --- a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm +++ b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm @@ -8,6 +8,7 @@ #include "base/strings/utf_string_conversions.h" #include "chrome/app/chrome_command_ids.h" #include "chrome/browser/sync/glue/device_info.h" +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/sessions/sessions_sync_manager.h" #include "chrome/browser/ui/cocoa/cocoa_profile_test.h" @@ -57,9 +58,20 @@ class DummyRouter : public browser_sync::LocalSessionEventRouter { }; class WrenchMenuControllerTest - : public CocoaProfileTest, - public browser_sync::SessionsSyncManager::SyncInternalApiDelegate { + : public CocoaProfileTest { public: + WrenchMenuControllerTest() + : local_device_(new browser_sync::LocalDeviceInfoProviderMock( + "WrenchMenuControllerTest", + "Test Machine", + "Chromium 10k", + "Chrome 10k", + sync_pb::SyncEnums_DeviceType_TYPE_LINUX, + "device_id")) { + } + + virtual ~WrenchMenuControllerTest() {} + virtual void SetUp() OVERRIDE { CocoaProfileTest::SetUp(); ASSERT_TRUE(browser()); @@ -69,7 +81,7 @@ class WrenchMenuControllerTest manager_.reset(new browser_sync::SessionsSyncManager( profile(), - this, + local_device_.get(), scoped_ptr<browser_sync::LocalSessionEventRouter>( new DummyRouter()))); manager_->MergeDataAndStartSyncing( @@ -81,21 +93,6 @@ class WrenchMenuControllerTest new syncer::SyncErrorFactoryMock)); } - virtual scoped_ptr<browser_sync::DeviceInfo> GetLocalDeviceInfo() - const OVERRIDE { - return scoped_ptr<browser_sync::DeviceInfo>( - new browser_sync::DeviceInfo(GetLocalSyncCacheGUID(), - "Test Machine", - "Chromium 10k", - "Chrome 10k", - sync_pb::SyncEnums_DeviceType_TYPE_LINUX, - "device_id")); - } - - virtual std::string GetLocalSyncCacheGUID() const OVERRIDE { - return "WrenchMenuControllerTest"; - } - void RegisterRecentTabs(RecentTabsBuilderTestHelper* helper) { helper->ExportToSessionsSyncManager(manager_.get()); } @@ -121,6 +118,7 @@ class WrenchMenuControllerTest private: scoped_ptr<browser_sync::SessionsSyncManager> manager_; + scoped_ptr<browser_sync::LocalDeviceInfoProviderMock> local_device_; }; TEST_F(WrenchMenuControllerTest, Initialized) { diff --git a/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc b/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc index 924cea5..8b9387a 100644 --- a/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc +++ b/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc @@ -23,6 +23,7 @@ #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/signin/signin_names_io_thread.h" #include "chrome/browser/signin/signin_promo.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/test_profile_sync_service.h" #include "chrome/browser/ui/sync/one_click_signin_helper.h" @@ -202,7 +203,8 @@ class OneClickTestProfileSyncService : public TestProfileSyncService { private: explicit OneClickTestProfileSyncService(Profile* profile) : TestProfileSyncService( - NULL, + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), profile, SigninManagerFactory::GetForProfile(profile), ProfileOAuth2TokenServiceFactory::GetForProfile(profile), diff --git a/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc b/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc index 8c00b55..5b7750a 100644 --- a/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc +++ b/chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc @@ -9,6 +9,7 @@ #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/signin/signin_promo.h" +#include "chrome/browser/sync/profile_sync_components_factory_mock.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/startup_controller.h" #include "chrome/browser/sync/test_profile_sync_service.h" @@ -67,7 +68,8 @@ class OneClickTestProfileSyncService : public TestProfileSyncService { private: explicit OneClickTestProfileSyncService(Profile* profile) : TestProfileSyncService( - NULL, + scoped_ptr<ProfileSyncComponentsFactory>( + new ProfileSyncComponentsFactoryMock()), profile, SigninManagerFactory::GetForProfile(profile), ProfileOAuth2TokenServiceFactory::GetForProfile(profile), diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc index 6875d40..fdc3d3e 100644 --- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc +++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc @@ -12,6 +12,7 @@ #include "chrome/browser/sessions/session_types.h" #include "chrome/browser/sessions/persistent_tab_restore_service.h" #include "chrome/browser/sessions/tab_restore_service_factory.h" +#include "chrome/browser/sync/glue/local_device_info_provider_mock.h" #include "chrome/browser/sync/glue/synced_session.h" #include "chrome/browser/sync/profile_sync_service_mock.h" #include "chrome/browser/sync/sessions/sessions_sync_manager.h" @@ -109,14 +110,20 @@ class DummyRouter : public browser_sync::LocalSessionEventRouter { } // namespace class RecentTabsSubMenuModelTest - : public BrowserWithTestWindowTest, - public browser_sync::SessionsSyncManager::SyncInternalApiDelegate { + : public BrowserWithTestWindowTest { public: RecentTabsSubMenuModelTest() - : sync_service_(&testing_profile_) { + : sync_service_(&testing_profile_), + local_device_(new browser_sync::LocalDeviceInfoProviderMock( + "RecentTabsSubMenuModelTest", + "Test Machine", + "Chromium 10k", + "Chrome 10k", + sync_pb::SyncEnums_DeviceType_TYPE_LINUX, + "device_id")) { manager_.reset(new browser_sync::SessionsSyncManager( &testing_profile_, - this, + local_device_.get(), scoped_ptr<browser_sync::LocalSessionEventRouter>( new DummyRouter()))); manager_->MergeDataAndStartSyncing( @@ -141,7 +148,6 @@ class RecentTabsSubMenuModelTest Profile::FromBrowserContext(browser_context), NULL); } - browser_sync::OpenTabsUIDelegate* GetOpenTabsDelegate() { return manager_.get(); } @@ -150,26 +156,12 @@ class RecentTabsSubMenuModelTest helper->ExportToSessionsSyncManager(manager_.get()); } - virtual scoped_ptr<browser_sync::DeviceInfo> GetLocalDeviceInfo() - const OVERRIDE { - return scoped_ptr<browser_sync::DeviceInfo>( - new browser_sync::DeviceInfo(GetLocalSyncCacheGUID(), - "Test Machine", - "Chromium 10k", - "Chrome 10k", - sync_pb::SyncEnums_DeviceType_TYPE_LINUX, - "device_id")); - } - - virtual std::string GetLocalSyncCacheGUID() const OVERRIDE { - return "RecentTabsSubMenuModelTest"; - } - private: TestingProfile testing_profile_; testing::NiceMock<ProfileSyncServiceMock> sync_service_; scoped_ptr<browser_sync::SessionsSyncManager> manager_; + scoped_ptr<browser_sync::LocalDeviceInfoProviderMock> local_device_; }; // Test disabled "Recently closed" header with no foreign tabs. diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 641db2c..b499b50 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1304,6 +1304,9 @@ 'browser/sync/glue/history_model_worker.h', 'browser/sync/glue/invalidation_adapter.cc', 'browser/sync/glue/invalidation_adapter.h', + 'browser/sync/glue/local_device_info_provider.h', + 'browser/sync/glue/local_device_info_provider_impl.cc', + 'browser/sync/glue/local_device_info_provider_impl.h', 'browser/sync/glue/non_frontend_data_type_controller.cc', 'browser/sync/glue/non_frontend_data_type_controller.h', 'browser/sync/glue/password_data_type_controller.cc', diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 4016abe..76ba1f4 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -191,6 +191,10 @@ 'browser/signin/fake_signin_manager.h', 'browser/ssl/ssl_client_auth_requestor_mock.cc', 'browser/ssl/ssl_client_auth_requestor_mock.h', + 'browser/sync/glue/local_device_info_provider_mock.cc', + 'browser/sync/glue/local_device_info_provider_mock.h', + 'browser/sync/profile_sync_components_factory_mock.cc', + 'browser/sync/profile_sync_components_factory_mock.h', 'browser/sync/profile_sync_service_mock.cc', 'browser/sync/profile_sync_service_mock.h', 'browser/ui/app_list/test/chrome_app_list_test_support.cc', @@ -1359,6 +1363,7 @@ 'browser/sync/glue/frontend_data_type_controller_mock.cc', 'browser/sync/glue/frontend_data_type_controller_mock.h', 'browser/sync/glue/frontend_data_type_controller_unittest.cc', + 'browser/sync/glue/local_device_info_provider_unittest.cc', 'browser/sync/glue/non_frontend_data_type_controller_mock.cc', 'browser/sync/glue/non_frontend_data_type_controller_mock.h', 'browser/sync/glue/non_frontend_data_type_controller_unittest.cc', @@ -1373,8 +1378,6 @@ 'browser/sync/glue/ui_model_worker_unittest.cc', 'browser/sync/profile_sync_auth_provider_unittest.cc', 'browser/sync/profile_sync_components_factory_impl_unittest.cc', - 'browser/sync/profile_sync_components_factory_mock.cc', - 'browser/sync/profile_sync_components_factory_mock.h', 'browser/sync/profile_sync_service_autofill_unittest.cc', 'browser/sync/profile_sync_service_bookmark_unittest.cc', 'browser/sync/profile_sync_service_startup_unittest.cc', @@ -1382,6 +1385,7 @@ 'browser/sync/profile_sync_service_unittest.cc', 'browser/sync/profile_sync_test_util.cc', 'browser/sync/profile_sync_test_util.h', + 'browser/sync/sessions/session_data_type_controller_unittest.cc', 'browser/sync/sessions/sessions_sync_manager_unittest.cc', 'browser/sync/sessions/tab_node_pool_unittest.cc', 'browser/sync/startup_controller_unittest.cc', diff --git a/components/sync_driver/sync_frontend.h b/components/sync_driver/sync_frontend.h index 0a360a8..247d215 100644 --- a/components/sync_driver/sync_frontend.h +++ b/components/sync_driver/sync_frontend.h @@ -48,6 +48,7 @@ class SyncFrontend { const syncer::WeakHandle<syncer::JsBackend>& js_backend, const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>& debug_info_listener, + const std::string& cache_guid, bool success) = 0; // The backend queried the server recently and received some updates. |