Simplify ios session sync logic

SessionSyncService's API is sufficient to achieve the desired
behavior, i.e. distinguish the exact state for tab sync: off,
initial download in progress and done. It also issues a
notification every time that state changes, so there's no need to
monitor updates from ProfileSyncService directly.

Bug: 895455
Change-Id: Icf66328fdde97f833e178f0571dedb248c91beca
Reviewed-on: https://chromium-review.googlesource.com/c/1344029Reviewed-by: Sergio Collazos <sczs@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612077}

ios/chrome/browser/sync/sync_setup_service.cc

@@ -73,7 +73,7 @@ void SyncSetupService::SetDataTypeEnabled(syncer::ModelType datatype,
     SetSyncEnabled(false);
 }
 
-bool SyncSetupService::UserActionIsRequiredToHaveSyncWork() {
+bool SyncSetupService::UserActionIsRequiredToHaveTabSyncWork() {
   if (!IsSyncEnabled() || !IsDataTypePreferred(syncer::PROXY_TABS)) {
     return true;
   }

ios/chrome/browser/sync/sync_setup_service.h

@@ -44,7 +44,7 @@ class SyncSetupService : public KeyedService {
     kNumberOfSyncableDatatypes
   };
 
-  SyncSetupService(syncer::SyncService* sync_service);
+  explicit SyncSetupService(syncer::SyncService* sync_service);
   ~SyncSetupService() override;
 
   // Returns the |syncer::ModelType| associated to the given
@@ -72,8 +72,8 @@ class SyncSetupService : public KeyedService {
   void SetDataTypeEnabled(syncer::ModelType datatype, bool enabled);
 
   // Returns whether the user needs to enter a passphrase or enable sync to make
-  // sync work.
-  bool UserActionIsRequiredToHaveSyncWork();
+  // tab sync work.
+  bool UserActionIsRequiredToHaveTabSyncWork();
 
   // Returns whether all datatypes are being synced.
   virtual bool IsSyncingAllDataTypes() const;

ios/chrome/browser/ui/recent_tabs/BUILD.gn

@@ -18,7 +18,6 @@ source_set("recent_tabs") {
     ":recent_tabs_ui",
     "resources:show_history",
     "//base",
-    "//components/browser_sync",
     "//components/sessions",
     "//components/sync",
     "//ios/chrome/app/strings",
@@ -58,7 +57,6 @@ source_set("recent_tabs_ui") {
   ]
   deps = [
     "//base",
-    "//components/browser_sync",
     "//components/sessions",
     "//components/strings",
     "//components/sync",
@@ -97,7 +95,7 @@ source_set("unit_tests") {
     ":recent_tabs_ui",
     "//base",
     "//components/browser_sync",
-    "//components/browser_sync:test_support",
+    "//components/sync:test_support_model",
     "//components/sync_sessions",
     "//ios/chrome/browser/browser_state:test_support",
     "//ios/chrome/browser/signin",

ios/chrome/browser/ui/recent_tabs/recent_tabs_coordinator_unittest.mm

@@ -9,11 +9,12 @@
 #include <memory>
 
 #include "components/browser_sync/profile_sync_service.h"
-#include "components/browser_sync/profile_sync_service_mock.h"
+#include "components/sync/model/fake_model_type_controller_delegate.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
+#include "components/sync_sessions/session_sync_service.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
-#include "ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.h"
 #include "ios/chrome/browser/sync/profile_sync_service_factory.h"
+#include "ios/chrome/browser/sync/session_sync_service_factory.h"
 #include "ios/chrome/browser/sync/sync_setup_service.h"
 #include "ios/chrome/browser/sync/sync_setup_service_factory.h"
 #include "ios/chrome/browser/sync/sync_setup_service_mock.h"
@@ -32,7 +33,6 @@
 #endif
 
 using testing::_;
-using testing::AtLeast;
 using testing::Return;
 
 namespace {
@@ -43,26 +43,35 @@ std::unique_ptr<KeyedService> CreateSyncSetupService(
       ios::ChromeBrowserState::FromBrowserState(context);
   syncer::SyncService* sync_service =
       ProfileSyncServiceFactory::GetForBrowserState(chrome_browser_state);
-  return std::make_unique<SyncSetupServiceMock>(sync_service);
+  return std::make_unique<testing::NiceMock<SyncSetupServiceMock>>(
+      sync_service);
 }
 
-class ProfileSyncServiceMockForRecentTabsTableCoordinator
-    : public browser_sync::ProfileSyncServiceMock {
+class SessionSyncServiceMockForRecentTabsTableCoordinator
+    : public sync_sessions::SessionSyncService {
  public:
-  explicit ProfileSyncServiceMockForRecentTabsTableCoordinator(
-      InitParams init_params)
-      : browser_sync::ProfileSyncServiceMock(std::move(init_params)) {}
-  ~ProfileSyncServiceMockForRecentTabsTableCoordinator() override {}
+  SessionSyncServiceMockForRecentTabsTableCoordinator() {}
+  ~SessionSyncServiceMockForRecentTabsTableCoordinator() override {}
 
+  MOCK_CONST_METHOD0(GetGlobalIdMapper, syncer::GlobalIdMapper*());
   MOCK_METHOD0(GetOpenTabsUIDelegate, sync_sessions::OpenTabsUIDelegate*());
+  MOCK_METHOD1(SubscribeToForeignSessionsChanged,
+               std::unique_ptr<base::CallbackList<void()>::Subscription>(
+                   const base::RepeatingClosure& cb));
+  MOCK_METHOD0(ScheduleGarbageCollection, void());
+  MOCK_METHOD0(GetControllerDelegate,
+               base::WeakPtr<syncer::ModelTypeControllerDelegate>());
+  MOCK_METHOD0(GetFaviconCache, sync_sessions::FaviconCache*());
+  MOCK_METHOD1(ProxyTabsStateChanged,
+               void(syncer::DataTypeController::State state));
+  MOCK_METHOD1(SetSyncSessionsGUID, void(const std::string& guid));
 };
 
 std::unique_ptr<KeyedService>
-BuildMockProfileSyncServiceForRecentTabsTableCoordinator(
+BuildMockSessionSyncServiceForRecentTabsTableCoordinator(
     web::BrowserState* context) {
-  return std::make_unique<ProfileSyncServiceMockForRecentTabsTableCoordinator>(
-      CreateProfileSyncServiceParamsForTest(
-          nullptr, ios::ChromeBrowserState::FromBrowserState(context)));
+  return std::make_unique<
+      testing::NiceMock<SessionSyncServiceMockForRecentTabsTableCoordinator>>();
 }
 
 class OpenTabsUIDelegateMock : public sync_sessions::OpenTabsUIDelegate {
@@ -93,7 +102,9 @@ class OpenTabsUIDelegateMock : public sync_sessions::OpenTabsUIDelegate {
 
 class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
  public:
-  RecentTabsTableCoordinatorTest() : no_error_(GoogleServiceAuthError::NONE) {}
+  RecentTabsTableCoordinatorTest()
+      : no_error_(GoogleServiceAuthError::NONE),
+        fake_controller_delegate_(syncer::SESSIONS) {}
 
  protected:
   void SetUp() override {
@@ -104,24 +115,10 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
         SyncSetupServiceFactory::GetInstance(),
         base::BindRepeating(&CreateSyncSetupService));
     test_cbs_builder.AddTestingFactory(
-        ProfileSyncServiceFactory::GetInstance(),
+        SessionSyncServiceFactory::GetInstance(),
         base::BindRepeating(
-            &BuildMockProfileSyncServiceForRecentTabsTableCoordinator));
+            &BuildMockSessionSyncServiceForRecentTabsTableCoordinator));
     chrome_browser_state_ = test_cbs_builder.Build();
-
-    ProfileSyncServiceMockForRecentTabsTableCoordinator* sync_service =
-        static_cast<ProfileSyncServiceMockForRecentTabsTableCoordinator*>(
-            ProfileSyncServiceFactory::GetForBrowserState(
-                chrome_browser_state_.get()));
-    EXPECT_CALL(*sync_service, GetAuthError())
-        .WillRepeatedly(::testing::ReturnRef(no_error_));
-    ON_CALL(*sync_service, GetRegisteredDataTypes())
-        .WillByDefault(Return(syncer::ModelTypeSet()));
-    sync_service->Initialize();
-    EXPECT_CALL(*sync_service, GetTransportState())
-        .WillRepeatedly(Return(syncer::SyncService::TransportState::ACTIVE));
-    EXPECT_CALL(*sync_service, GetOpenTabsUIDelegate())
-        .WillRepeatedly(Return(nullptr));
   }
 
   void TearDown() override {
@@ -133,6 +130,7 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
 
   void SetupSyncState(BOOL signedIn,
                       BOOL syncEnabled,
+                      BOOL syncCompleted,
                       BOOL hasForeignSessions) {
     if (signedIn) {
       identity_test_env_.MakePrimaryAccountAvailable("test@test.com");
@@ -143,39 +141,35 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
           signin_metrics::SignoutDelete::IGNORE_METRIC);
     }
 
+    SessionSyncServiceMockForRecentTabsTableCoordinator* session_sync_service =
+        static_cast<SessionSyncServiceMockForRecentTabsTableCoordinator*>(
+            SessionSyncServiceFactory::GetForBrowserState(
+                chrome_browser_state_.get()));
+
+    // Needed by ProfileSyncService's initialization, triggered during
+    // initialization of SyncSetupServiceMock.
+    ON_CALL(*session_sync_service, GetControllerDelegate())
+        .WillByDefault(Return(fake_controller_delegate_.GetWeakPtr()));
+
     SyncSetupServiceMock* syncSetupService = static_cast<SyncSetupServiceMock*>(
         SyncSetupServiceFactory::GetForBrowserState(
             chrome_browser_state_.get()));
-    EXPECT_CALL(*syncSetupService, IsSyncEnabled())
-        .WillRepeatedly(Return(syncEnabled));
-    EXPECT_CALL(*syncSetupService, IsDataTypePreferred(syncer::PROXY_TABS))
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*syncSetupService, GetSyncServiceState())
-        .WillRepeatedly(Return(SyncSetupService::kNoSyncServiceError));
-
-    if (syncEnabled) {
-      ProfileSyncServiceMockForRecentTabsTableCoordinator* sync_service =
-          static_cast<ProfileSyncServiceMockForRecentTabsTableCoordinator*>(
-              ProfileSyncServiceFactory::GetForBrowserState(
-                  chrome_browser_state_.get()));
-      open_tabs_ui_delegate_.reset(new OpenTabsUIDelegateMock());
-      EXPECT_CALL(*sync_service, GetOpenTabsUIDelegate())
-          .WillRepeatedly(Return(open_tabs_ui_delegate_.get()));
-      EXPECT_CALL(*open_tabs_ui_delegate_, GetAllForeignSessions(_))
-          .WillRepeatedly(Return(hasForeignSessions));
+    ON_CALL(*syncSetupService, IsSyncEnabled())
+        .WillByDefault(Return(syncEnabled));
+    ON_CALL(*syncSetupService, IsDataTypePreferred(syncer::PROXY_TABS))
+        .WillByDefault(Return(true));
+    ON_CALL(*syncSetupService, GetSyncServiceState())
+        .WillByDefault(Return(SyncSetupService::kNoSyncServiceError));
+
+    if (syncCompleted) {
+      ON_CALL(*session_sync_service, GetOpenTabsUIDelegate())
+          .WillByDefault(Return(&open_tabs_ui_delegate_));
+      ON_CALL(open_tabs_ui_delegate_, GetAllForeignSessions(_))
+          .WillByDefault(Return(hasForeignSessions));
     }
   }
 
   void CreateController() {
-    // Sets up the test expectations for the Sync Service Observer Bridge.
-    // RecentTabsTableCoordinator must be added as an observer of
-    // ProfileSyncService changes and removed when it is destroyed.
-    browser_sync::ProfileSyncServiceMock* sync_service =
-        static_cast<browser_sync::ProfileSyncServiceMock*>(
-            ProfileSyncServiceFactory::GetForBrowserState(
-                chrome_browser_state_.get()));
-    EXPECT_CALL(*sync_service, AddObserver(_)).Times(AtLeast(1));
-    EXPECT_CALL(*sync_service, RemoveObserver(_)).Times(AtLeast(1));
     coordinator_ = [[RecentTabsCoordinator alloc]
         initWithBaseViewController:nil
                       browserState:chrome_browser_state_.get()];
@@ -188,8 +182,9 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
   IOSChromeScopedTestingLocalState local_state_;
   identity::IdentityTestEnvironment identity_test_env_;
 
+  syncer::FakeModelTypeControllerDelegate fake_controller_delegate_;
+  testing::NiceMock<OpenTabsUIDelegateMock> open_tabs_ui_delegate_;
   std::unique_ptr<TestChromeBrowserState> chrome_browser_state_;
-  std::unique_ptr<OpenTabsUIDelegateMock> open_tabs_ui_delegate_;
 
   // Must be declared *after* |chrome_browser_state_| so it can outlive it.
   RecentTabsCoordinator* coordinator_;
@@ -201,22 +196,31 @@ TEST_F(RecentTabsTableCoordinatorTest, TestConstructorDestructor) {
 }
 
 TEST_F(RecentTabsTableCoordinatorTest, TestUserSignedOut) {
-  SetupSyncState(NO, NO, NO);
+  // TODO(crbug.com/907495): Actual test expectations are missing below.
+  SetupSyncState(NO, NO, NO, NO);
   CreateController();
 }
 
 TEST_F(RecentTabsTableCoordinatorTest, TestUserSignedInSyncOff) {
-  SetupSyncState(YES, NO, NO);
+  // TODO(crbug.com/907495): Actual test expectations are missing below.
+  SetupSyncState(YES, NO, NO, NO);
   CreateController();
 }
 
 TEST_F(RecentTabsTableCoordinatorTest, TestUserSignedInSyncInProgress) {
-  SetupSyncState(YES, YES, NO);
+  // TODO(crbug.com/907495): Actual test expectations are missing below.
+  SetupSyncState(YES, YES, NO, NO);
+  CreateController();
+}
+TEST_F(RecentTabsTableCoordinatorTest, TestUserSignedInSyncOnWithoutSessions) {
+  // TODO(crbug.com/907495): Actual test expectations are missing below.
+  SetupSyncState(YES, YES, YES, NO);
   CreateController();
 }
 
 TEST_F(RecentTabsTableCoordinatorTest, TestUserSignedInSyncOnWithSessions) {
-  SetupSyncState(YES, YES, YES);
+  // TODO(crbug.com/907495): Actual test expectations are missing below.
+  SetupSyncState(YES, YES, YES, YES);
   CreateController();
 }
 

ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm

@@ -139,7 +139,7 @@ const CGFloat kFaviconMinWidthHeight = 16;
   DCHECK([self isSignedIn]);
   SyncSetupService* service =
       SyncSetupServiceFactory::GetForBrowserState(_browserState);
-  return !service->UserActionIsRequiredToHaveSyncWork();
+  return !service->UserActionIsRequiredToHaveTabSyncWork();
 }
 
 // Returns whether this profile has any foreign sessions to sync.
@@ -148,25 +148,29 @@ const CGFloat kFaviconMinWidthHeight = 16;
     return SessionsSyncUserState::USER_SIGNED_OUT;
   if (![self isSyncTabsEnabled])
     return SessionsSyncUserState::USER_SIGNED_IN_SYNC_OFF;
-  if ([self hasForeignSessions])
-    return SessionsSyncUserState::USER_SIGNED_IN_SYNC_ON_WITH_SESSIONS;
   if (![self isSyncCompleted])
     return SessionsSyncUserState::USER_SIGNED_IN_SYNC_IN_PROGRESS;
+  if ([self hasForeignSessions])
+    return SessionsSyncUserState::USER_SIGNED_IN_SYNC_ON_WITH_SESSIONS;
   return SessionsSyncUserState::USER_SIGNED_IN_SYNC_ON_NO_SESSIONS;
 }
 
+- (BOOL)isSyncCompleted {
+  sync_sessions::SessionSyncService* service =
+      SessionSyncServiceFactory::GetForBrowserState(_browserState);
+  DCHECK(service);
+  return service->GetOpenTabsUIDelegate() != nullptr;
+}
+
 - (BOOL)hasForeignSessions {
   sync_sessions::SessionSyncService* service =
       SessionSyncServiceFactory::GetForBrowserState(_browserState);
   DCHECK(service);
   sync_sessions::OpenTabsUIDelegate* openTabs =
       service->GetOpenTabsUIDelegate();
+  DCHECK(openTabs);
   std::vector<const sync_sessions::SyncedSession*> sessions;
-  return openTabs ? openTabs->GetAllForeignSessions(&sessions) : NO;
-}
-
-- (BOOL)isSyncCompleted {
-  return _syncedSessionsObserver->IsFirstSyncCycleCompleted();
+  return openTabs->GetAllForeignSessions(&sessions);
 }
 
 #pragma mark - RecentTabsTableViewControllerDelegate

ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.h

@@ -11,8 +11,6 @@
 
 #include "base/callback_list.h"
 #include "base/macros.h"
-#include "components/sync/driver/sync_service_observer.h"
-#import "ios/chrome/browser/sync/sync_observer_bridge.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
 namespace ios {
@@ -23,7 +21,7 @@ namespace identity {
 class IdentityManager;
 }
 
-@protocol SyncedSessionsObserver<SyncObserverModelBridge>
+@protocol SyncedSessionsObserver
 // Reloads the session data.
 - (void)reloadSessions;
 @end
@@ -33,21 +31,15 @@ namespace synced_sessions {
 // Bridge class that will notify the panel when the remote sessions content
 // change.
 class SyncedSessionsObserverBridge
-    : public SyncObserverBridge,
-      public identity::IdentityManager::Observer {
+    : public identity::IdentityManager::Observer {
  public:
   SyncedSessionsObserverBridge(id<SyncedSessionsObserver> owner,
                                ios::ChromeBrowserState* browserState);
   ~SyncedSessionsObserverBridge() override;
-  // SyncObserverBridge implementation.
-  void OnSyncConfigurationCompleted(syncer::SyncService* sync) override;
   // identity::IdentityManager::Observer implementation.
   void OnPrimaryAccountCleared(
       const AccountInfo& previous_primary_account_info) override;
 
-  // Returns true if the first sync cycle that contains session information is
-  // completed. Returns false otherwise.
-  bool IsFirstSyncCycleCompleted();
   // Returns true if user is signed in.
   bool IsSignedIn();
 
@@ -56,7 +48,6 @@ class SyncedSessionsObserverBridge
 
   __weak id<SyncedSessionsObserver> owner_ = nil;
   identity::IdentityManager* identity_manager_ = nullptr;
-  ios::ChromeBrowserState* browser_state_;
   ScopedObserver<identity::IdentityManager, identity::IdentityManager::Observer>
       identity_manager_observer_;
   std::unique_ptr<base::CallbackList<void()>::Subscription>
@@ -67,4 +58,4 @@ class SyncedSessionsObserverBridge
 
 }  // namespace synced_sessions
 
-#endif  // IOS_CHROME_BROWSER_SYNC_SYNCED_SESSIONS_BRIDGE_H_
+#endif  // IOS_CHROME_BROWSER_UI_RECENT_TABS_SYNCED_SESSIONS_BRIDGE_H_

ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.mm

@@ -4,14 +4,10 @@
 
 #import "ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.h"
 
-#include "components/browser_sync/profile_sync_service.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/signin/identity_manager_factory.h"
-#include "ios/chrome/browser/sync/profile_sync_service_factory.h"
 #include "ios/chrome/browser/sync/session_sync_service_factory.h"
-#include "ios/chrome/browser/sync/sync_setup_service.h"
-#include "ios/chrome/browser/sync/sync_setup_service_factory.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -25,13 +21,9 @@ namespace synced_sessions {
 SyncedSessionsObserverBridge::SyncedSessionsObserverBridge(
     id<SyncedSessionsObserver> owner,
     ios::ChromeBrowserState* browserState)
-    : SyncObserverBridge(
-          owner,
-          ProfileSyncServiceFactory::GetForBrowserState(browserState)),
-      owner_(owner),
+    : owner_(owner),
       identity_manager_(
           IdentityManagerFactory::GetForBrowserState(browserState)),
-      browser_state_(browserState),
       identity_manager_observer_(this) {
   identity_manager_observer_.Add(identity_manager_);
 
@@ -46,24 +38,6 @@ SyncedSessionsObserverBridge::SyncedSessionsObserverBridge(
 
 SyncedSessionsObserverBridge::~SyncedSessionsObserverBridge() {}
 
-#pragma mark - SyncObserverBridge
-
-void SyncedSessionsObserverBridge::OnSyncConfigurationCompleted(
-    syncer::SyncService* sync) {
-  // TODO(crbug.com/895455): This notification seems redundant because
-  // OnForeignSessionChanged() should be called when the initial sync is
-  // completed.
-  [owner_ reloadSessions];
-}
-
-bool SyncedSessionsObserverBridge::IsFirstSyncCycleCompleted() {
-  // TODO(crbug.com/895455): This could probably be implemented via
-  // SessionSyncService directly and possibly remove all dependencies to
-  // SyncService/SyncSetupService.
-  return SyncSetupServiceFactory::GetForBrowserState(browser_state_)
-      ->IsDataTypeActive(syncer::SESSIONS);
-}
-
 #pragma mark - identity::IdentityManager::Observer
 
 void SyncedSessionsObserverBridge::OnPrimaryAccountCleared(