Split SessionSyncService and Impl

This will allow tests to introduce doubles in future patches.

Bug: 895455
Change-Id: I33b99a8a90f00ab47678a852e4521e90deee1390
Reviewed-on: https://chromium-review.googlesource.com/c/1346453Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610181}

chrome/browser/sync/session_sync_service_factory.cc

@@ -22,7 +22,7 @@
 #include "components/sync/device_info/local_device_info_provider.h"
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync_sessions/session_sync_prefs.h"
-#include "components/sync_sessions/session_sync_service.h"
+#include "components/sync_sessions/session_sync_service_impl.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 
 #if defined(OS_ANDROID)
@@ -156,6 +156,6 @@ SessionSyncServiceFactory::~SessionSyncServiceFactory() {}
 KeyedService* SessionSyncServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = Profile::FromBrowserContext(context);
-  return new sync_sessions::SessionSyncService(
+  return new sync_sessions::SessionSyncServiceImpl(
       chrome::GetChannel(), std::make_unique<SyncSessionsClientImpl>(profile));
 }

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

@@ -39,7 +39,7 @@
 #include "components/sync/driver/data_type_controller.h"
 #include "components/sync/engine/data_type_activation_response.h"
 #include "components/sync/model/data_type_activation_request.h"
-#include "components/sync_sessions/session_sync_service.h"
+#include "components/sync_sessions/session_sync_service_impl.h"
 #include "components/sync_sessions/synced_session.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_utils.h"
@@ -167,7 +167,9 @@ class RecentTabsSubMenuModelTest
   void RegisterRecentTabs(RecentTabsBuilderTestHelper* helper) {
     helper->ExportToSessionSync(
         sync_processor_.get(),
-        session_sync_service_->GetUnderlyingOpenTabsUIDelegateForTest());
+        static_cast<sync_sessions::SessionSyncServiceImpl*>(
+            session_sync_service_)
+            ->GetUnderlyingOpenTabsUIDelegateForTest());
   }
 
  private:

components/sync_sessions/BUILD.gn

@@ -25,6 +25,8 @@ static_library("sync_sessions") {
     "session_sync_prefs.h",
     "session_sync_service.cc",
     "session_sync_service.h",
+    "session_sync_service_impl.cc",
+    "session_sync_service_impl.h",
     "sessions_global_id_mapper.cc",
     "sessions_global_id_mapper.h",
     "sync_sessions_client.cc",

components/sync_sessions/session_sync_service.cc

@@ -4,85 +4,10 @@
 
 #include "components/sync_sessions/session_sync_service.h"
 
-#include <utility>
-
-#include "base/bind_helpers.h"
-#include "components/sync/base/report_unrecoverable_error.h"
-#include "components/sync/model_impl/client_tag_based_model_type_processor.h"
-#include "components/sync_sessions/favicon_cache.h"
-#include "components/sync_sessions/session_sync_bridge.h"
-#include "components/sync_sessions/session_sync_prefs.h"
-#include "components/sync_sessions/sync_sessions_client.h"
-
 namespace sync_sessions {
 
-SessionSyncService::SessionSyncService(
-    version_info::Channel channel,
-    std::unique_ptr<SyncSessionsClient> sessions_client)
-    : sessions_client_(std::move(sessions_client)) {
-  DCHECK(sessions_client_);
-
-  bridge_ = std::make_unique<sync_sessions::SessionSyncBridge>(
-      base::BindRepeating(&SessionSyncService::NotifyForeignSessionUpdated,
-                          base::Unretained(this)),
-      sessions_client_.get(),
-      std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
-          syncer::SESSIONS,
-          base::BindRepeating(&syncer::ReportUnrecoverableError, channel)));
-}
-
-SessionSyncService::~SessionSyncService() {}
-
-syncer::GlobalIdMapper* SessionSyncService::GetGlobalIdMapper() const {
-  return bridge_->GetGlobalIdMapper();
-}
-
-OpenTabsUIDelegate* SessionSyncService::GetOpenTabsUIDelegate() {
-  if (!proxy_tabs_running_) {
-    return nullptr;
-  }
-  return bridge_->GetOpenTabsUIDelegate();
-}
-
-std::unique_ptr<base::CallbackList<void()>::Subscription>
-SessionSyncService::SubscribeToForeignSessionsChanged(
-    const base::RepeatingClosure& cb) {
-  return foreign_sessions_changed_callback_list_.Add(cb);
-}
-
-void SessionSyncService::ScheduleGarbageCollection() {
-  bridge_->ScheduleGarbageCollection();
-}
-
-base::WeakPtr<syncer::ModelTypeControllerDelegate>
-SessionSyncService::GetControllerDelegate() {
-  return bridge_->change_processor()->GetControllerDelegate();
-}
-
-FaviconCache* SessionSyncService::GetFaviconCache() {
-  return bridge_->GetFaviconCache();
-}
-
-void SessionSyncService::ProxyTabsStateChanged(
-    syncer::DataTypeController::State state) {
-  const bool was_proxy_tabs_running = proxy_tabs_running_;
-  proxy_tabs_running_ = (state == syncer::DataTypeController::RUNNING);
-  if (proxy_tabs_running_ != was_proxy_tabs_running) {
-    NotifyForeignSessionUpdated();
-  }
-}
-
-void SessionSyncService::SetSyncSessionsGUID(const std::string& guid) {
-  sessions_client_->GetSessionSyncPrefs()->SetSyncSessionsGUID(guid);
-}
-
-OpenTabsUIDelegate*
-SessionSyncService::GetUnderlyingOpenTabsUIDelegateForTest() {
-  return bridge_->GetOpenTabsUIDelegate();
-}
+SessionSyncService::SessionSyncService() = default;
 
-void SessionSyncService::NotifyForeignSessionUpdated() {
-  foreign_sessions_changed_callback_list_.Notify();
-}
+SessionSyncService::~SessionSyncService() = default;
 
 }  // namespace sync_sessions

components/sync_sessions/session_sync_service.h

@@ -13,8 +13,6 @@
 #include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/driver/data_type_controller.h"
-#include "components/sync/model/model_type_store.h"
-#include "components/version_info/channel.h"
 
 namespace syncer {
 class GlobalIdMapper;
@@ -25,8 +23,6 @@ namespace sync_sessions {
 
 class FaviconCache;
 class OpenTabsUIDelegate;
-class SessionSyncBridge;
-class SyncSessionsClient;
 
 // KeyedService responsible session sync (aka tab sync), including favicon sync.
 // This powers things like the history UI, where "Tabs from other devices"
@@ -34,53 +30,40 @@ class SyncSessionsClient;
 // local tabs.
 class SessionSyncService : public KeyedService {
  public:
-  SessionSyncService(version_info::Channel channel,
-                     std::unique_ptr<SyncSessionsClient> sessions_client);
+  SessionSyncService();
   ~SessionSyncService() override;
 
-  syncer::GlobalIdMapper* GetGlobalIdMapper() const;
+  virtual syncer::GlobalIdMapper* GetGlobalIdMapper() const = 0;
 
   // Return the active OpenTabsUIDelegate. If open/proxy tabs is not enabled or
   // not currently syncing, returns nullptr.
-  OpenTabsUIDelegate* GetOpenTabsUIDelegate();
+  virtual OpenTabsUIDelegate* GetOpenTabsUIDelegate() = 0;
 
   // Allows client code to be notified when foreign sessions change.
-  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  virtual std::unique_ptr<base::CallbackList<void()>::Subscription>
   SubscribeToForeignSessionsChanged(const base::RepeatingClosure& cb)
-      WARN_UNUSED_RESULT;
+      WARN_UNUSED_RESULT = 0;
 
   // Schedules garbage collection of foreign sessions.
-  void ScheduleGarbageCollection();
+  virtual void ScheduleGarbageCollection() = 0;
 
   // For ProfileSyncService to initialize the controller for SESSIONS.
-  base::WeakPtr<syncer::ModelTypeControllerDelegate> GetControllerDelegate();
+  virtual base::WeakPtr<syncer::ModelTypeControllerDelegate>
+  GetControllerDelegate() = 0;
 
   // For ProfileSyncService to initialize the controller for FAVICON_IMAGES and
   // FAVICON_TRACKING.
-  FaviconCache* GetFaviconCache();
+  virtual FaviconCache* GetFaviconCache() = 0;
 
   // Intended to be used by ProxyDataTypeController: influences whether
   // GetOpenTabsUIDelegate() returns null or not.
-  void ProxyTabsStateChanged(syncer::DataTypeController::State state);
+  virtual void ProxyTabsStateChanged(
+      syncer::DataTypeController::State state) = 0;
 
   // Used on Android only, to override the machine tag.
-  void SetSyncSessionsGUID(const std::string& guid);
-
-  // Returns OpenTabsUIDelegate regardless of sync being enabled or disabled,
-  // useful for tests.
-  OpenTabsUIDelegate* GetUnderlyingOpenTabsUIDelegateForTest();
+  virtual void SetSyncSessionsGUID(const std::string& guid) = 0;
 
  private:
-  void NotifyForeignSessionUpdated();
-
-  std::unique_ptr<SyncSessionsClient> sessions_client_;
-
-  bool proxy_tabs_running_ = false;
-
-  std::unique_ptr<SessionSyncBridge> bridge_;
-
-  base::CallbackList<void()> foreign_sessions_changed_callback_list_;
-
   DISALLOW_COPY_AND_ASSIGN(SessionSyncService);
 };
 

components/sync_sessions/session_sync_service_impl.cc

+// Copyright 2018 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 "components/sync_sessions/session_sync_service_impl.h"
+
+#include <utility>
+
+#include "base/bind_helpers.h"
+#include "components/sync/base/report_unrecoverable_error.h"
+#include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync_sessions/favicon_cache.h"
+#include "components/sync_sessions/session_sync_bridge.h"
+#include "components/sync_sessions/session_sync_prefs.h"
+#include "components/sync_sessions/sync_sessions_client.h"
+
+namespace sync_sessions {
+
+SessionSyncServiceImpl::SessionSyncServiceImpl(
+    version_info::Channel channel,
+    std::unique_ptr<SyncSessionsClient> sessions_client)
+    : sessions_client_(std::move(sessions_client)) {
+  DCHECK(sessions_client_);
+
+  bridge_ = std::make_unique<sync_sessions::SessionSyncBridge>(
+      base::BindRepeating(&SessionSyncServiceImpl::NotifyForeignSessionUpdated,
+                          base::Unretained(this)),
+      sessions_client_.get(),
+      std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
+          syncer::SESSIONS,
+          base::BindRepeating(&syncer::ReportUnrecoverableError, channel)));
+}
+
+SessionSyncServiceImpl::~SessionSyncServiceImpl() {}
+
+syncer::GlobalIdMapper* SessionSyncServiceImpl::GetGlobalIdMapper() const {
+  return bridge_->GetGlobalIdMapper();
+}
+
+OpenTabsUIDelegate* SessionSyncServiceImpl::GetOpenTabsUIDelegate() {
+  if (!proxy_tabs_running_) {
+    return nullptr;
+  }
+  return bridge_->GetOpenTabsUIDelegate();
+}
+
+std::unique_ptr<base::CallbackList<void()>::Subscription>
+SessionSyncServiceImpl::SubscribeToForeignSessionsChanged(
+    const base::RepeatingClosure& cb) {
+  return foreign_sessions_changed_callback_list_.Add(cb);
+}
+
+void SessionSyncServiceImpl::ScheduleGarbageCollection() {
+  bridge_->ScheduleGarbageCollection();
+}
+
+base::WeakPtr<syncer::ModelTypeControllerDelegate>
+SessionSyncServiceImpl::GetControllerDelegate() {
+  return bridge_->change_processor()->GetControllerDelegate();
+}
+
+FaviconCache* SessionSyncServiceImpl::GetFaviconCache() {
+  return bridge_->GetFaviconCache();
+}
+
+void SessionSyncServiceImpl::ProxyTabsStateChanged(
+    syncer::DataTypeController::State state) {
+  const bool was_proxy_tabs_running = proxy_tabs_running_;
+  proxy_tabs_running_ = (state == syncer::DataTypeController::RUNNING);
+  if (proxy_tabs_running_ != was_proxy_tabs_running) {
+    NotifyForeignSessionUpdated();
+  }
+}
+
+void SessionSyncServiceImpl::SetSyncSessionsGUID(const std::string& guid) {
+  sessions_client_->GetSessionSyncPrefs()->SetSyncSessionsGUID(guid);
+}
+
+OpenTabsUIDelegate*
+SessionSyncServiceImpl::GetUnderlyingOpenTabsUIDelegateForTest() {
+  return bridge_->GetOpenTabsUIDelegate();
+}
+
+void SessionSyncServiceImpl::NotifyForeignSessionUpdated() {
+  foreign_sessions_changed_callback_list_.Notify();
+}
+
+}  // namespace sync_sessions

components/sync_sessions/session_sync_service_impl.h

+// Copyright 2018 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 COMPONENTS_SYNC_SESSIONS_SESSION_SYNC_SERVICE_IMPL_H_
+#define COMPONENTS_SYNC_SESSIONS_SESSION_SYNC_SERVICE_IMPL_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback_list.h"
+#include "base/memory/weak_ptr.h"
+#include "components/sync/model/model_type_store.h"
+#include "components/sync_sessions/session_sync_service.h"
+#include "components/version_info/channel.h"
+
+namespace sync_sessions {
+
+class SessionSyncBridge;
+class SyncSessionsClient;
+
+// Single non-test implementation of SessionSyncService.
+class SessionSyncServiceImpl : public SessionSyncService {
+ public:
+  SessionSyncServiceImpl(version_info::Channel channel,
+                         std::unique_ptr<SyncSessionsClient> sessions_client);
+  ~SessionSyncServiceImpl() override;
+
+  syncer::GlobalIdMapper* GetGlobalIdMapper() const override;
+
+  // Return the active OpenTabsUIDelegate. If open/proxy tabs is not enabled or
+  // not currently syncing, returns nullptr.
+  OpenTabsUIDelegate* GetOpenTabsUIDelegate() override;
+
+  // Allows client code to be notified when foreign sessions change.
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  SubscribeToForeignSessionsChanged(const base::RepeatingClosure& cb) override
+      WARN_UNUSED_RESULT;
+
+  // Schedules garbage collection of foreign sessions.
+  void ScheduleGarbageCollection() override;
+
+  // For ProfileSyncService to initialize the controller for SESSIONS.
+  base::WeakPtr<syncer::ModelTypeControllerDelegate> GetControllerDelegate()
+      override;
+
+  // For ProfileSyncService to initialize the controller for FAVICON_IMAGES and
+  // FAVICON_TRACKING.
+  FaviconCache* GetFaviconCache() override;
+
+  // Intended to be used by ProxyDataTypeController: influences whether
+  // GetOpenTabsUIDelegate() returns null or not.
+  void ProxyTabsStateChanged(syncer::DataTypeController::State state) override;
+
+  // Used on Android only, to override the machine tag.
+  void SetSyncSessionsGUID(const std::string& guid) override;
+
+  // Returns OpenTabsUIDelegate regardless of sync being enabled or disabled,
+  // useful for tests.
+  OpenTabsUIDelegate* GetUnderlyingOpenTabsUIDelegateForTest();
+
+ private:
+  void NotifyForeignSessionUpdated();
+
+  std::unique_ptr<SyncSessionsClient> sessions_client_;
+
+  bool proxy_tabs_running_ = false;
+
+  std::unique_ptr<SessionSyncBridge> bridge_;
+
+  base::CallbackList<void()> foreign_sessions_changed_callback_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(SessionSyncServiceImpl);
+};
+
+}  // namespace sync_sessions
+
+#endif  // COMPONENTS_SYNC_SESSIONS_SESSION_SYNC_SERVICE_IMPL_H_

ios/chrome/browser/sync/session_sync_service_factory.cc

@@ -16,7 +16,7 @@
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync_sessions/local_session_event_router.h"
 #include "components/sync_sessions/session_sync_prefs.h"
-#include "components/sync_sessions/session_sync_service.h"
+#include "components/sync_sessions/session_sync_service_impl.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_window_delegates_getter.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
@@ -158,6 +158,6 @@ SessionSyncServiceFactory::BuildServiceInstanceFor(
     web::BrowserState* context) const {
   ios::ChromeBrowserState* browser_state =
       ios::ChromeBrowserState::FromBrowserState(context);
-  return std::make_unique<sync_sessions::SessionSyncService>(
+  return std::make_unique<sync_sessions::SessionSyncServiceImpl>(
       ::GetChannel(), std::make_unique<SyncSessionsClientImpl>(browser_state));
 }