[PhoneHub] Subscribe to sync state changes

CrosapiSessionSyncNotifier becomes a SyncServiceObserver and notifies
Ash of changes to the enabled state for tab sync.

Test: Added unit tests and manually verified.
Bug: b/260599791
Change-Id: I12e7648ef6e954014f90deae1b6405e9b092a6aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4267816
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Michael Hansen <hansenmichael@google.com>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Auto-Submit: Michael Hansen <hansenmichael@google.com>
Cr-Commit-Position: refs/heads/main@{#1118217}

chrome/browser/lacros/sync/crosapi_session_sync_notifier.cc

@@ -8,6 +8,7 @@
 
 #include "base/functional/bind.h"
 #include "chromeos/lacros/lacros_service.h"
+#include "components/sync/driver/sync_user_settings.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_sessions/sync_sessions_client.h"
@@ -86,13 +87,20 @@ std::vector<crosapi::mojom::SyncedSessionPtr> ConstructSyncedPhoneSessions(
   return crosapi_synced_phone_sessions;
 }
 
+bool IsTabSyncEnabled(syncer::SyncService* sync_service) {
+  return sync_service->GetUserSettings()->GetSelectedTypes().Has(
+      syncer::UserSelectableType::kTabs);
+}
+
 }  // namespace
 
 CrosapiSessionSyncNotifier::CrosapiSessionSyncNotifier(
     sync_sessions::SessionSyncService* session_sync_service,
     mojo::PendingRemote<crosapi::mojom::SyncedSessionClient>
-        synced_session_client)
-    : session_sync_service_(std::move(session_sync_service)),
+        synced_session_client,
+    syncer::SyncService* sync_service)
+    : is_tab_sync_enabled_(IsTabSyncEnabled(sync_service)),
+      session_sync_service_(session_sync_service),
       synced_session_client_(std::move(synced_session_client)) {
   if (synced_session_client_.version() >=
       static_cast<int>(crosapi::mojom::SyncedSessionClient::
@@ -103,12 +111,33 @@ CrosapiSessionSyncNotifier::CrosapiSessionSyncNotifier(
                 &CrosapiSessionSyncNotifier::OnForeignSyncedSessionsUpdated,
                 base::Unretained(this)));
   }
+
+  sync_service_observation_.Observe(sync_service);
+
+  // Broadcast the initial value for |is_tab_sync_enabled_|.
+  NotifySyncEnabledChanged();
 }
 
 CrosapiSessionSyncNotifier::~CrosapiSessionSyncNotifier() = default;
 
+void CrosapiSessionSyncNotifier::OnStateChanged(
+    syncer::SyncService* sync_service) {
+  bool is_tab_sync_enabled = IsTabSyncEnabled(sync_service);
+  if (is_tab_sync_enabled == is_tab_sync_enabled_) {
+    return;
+  }
+
+  is_tab_sync_enabled_ = is_tab_sync_enabled;
+  NotifySyncEnabledChanged();
+}
+
+void CrosapiSessionSyncNotifier::NotifySyncEnabledChanged() {
+  synced_session_client_->OnSessionSyncEnabledChanged(is_tab_sync_enabled_);
+}
+
 void CrosapiSessionSyncNotifier::OnForeignSyncedSessionsUpdated() {
-  // Fetch sessions
+  // Fetch sessions. Ensure |open_tabs| is not null since
+  // GetOpenTabsUIDelegate() can return null if session sync is not running.
   sync_sessions::OpenTabsUIDelegate* open_tabs =
       session_sync_service_->GetOpenTabsUIDelegate();
   if (!open_tabs) {

chrome/browser/lacros/sync/crosapi_session_sync_notifier.h

@@ -7,7 +7,10 @@
 
 #include "base/callback_list.h"
 #include "base/memory/raw_ptr.h"
+#include "base/scoped_observation.h"
 #include "chromeos/crosapi/mojom/synced_session_client.mojom.h"
+#include "components/sync/driver/sync_service.h"
+#include "components/sync/driver/sync_service_observer.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
@@ -17,24 +20,33 @@ class SessionSyncService;
 
 // This class is responsible for sending browser window data to Ash upon changes
 // to foreign browser sessions.
-class CrosapiSessionSyncNotifier {
+class CrosapiSessionSyncNotifier : public syncer::SyncServiceObserver {
  public:
   // `session_sync_service` should not be null and should outlive `this`.
+  // `sync_service` should not be null and should outlive `this`.
   CrosapiSessionSyncNotifier(
       sync_sessions::SessionSyncService* session_sync_service,
       mojo::PendingRemote<crosapi::mojom::SyncedSessionClient>
-          synced_session_client);
+          synced_session_client,
+      syncer::SyncService* sync_service);
   CrosapiSessionSyncNotifier(const CrosapiSessionSyncNotifier&) = delete;
   CrosapiSessionSyncNotifier& operator=(const CrosapiSessionSyncNotifier&) =
       delete;
-  ~CrosapiSessionSyncNotifier();
+  ~CrosapiSessionSyncNotifier() override;
 
  private:
+  // syncer::SyncServiceObserver:
+  void OnStateChanged(syncer::SyncService* sync_service) override;
+
+  void NotifySyncEnabledChanged();
   void OnForeignSyncedSessionsUpdated();
 
+  bool is_tab_sync_enabled_ = false;
   base::raw_ptr<sync_sessions::SessionSyncService> session_sync_service_;
   mojo::Remote<crosapi::mojom::SyncedSessionClient> synced_session_client_;
   base::CallbackListSubscription session_updated_subscription_;
+  base::ScopedObservation<syncer::SyncService, syncer::SyncServiceObserver>
+      sync_service_observation_{this};
 };
 
 #endif  // CHROME_BROWSER_LACROS_SYNC_CROSAPI_SESSION_SYNC_NOTIFIER_H_

chrome/browser/lacros/sync/crosapi_session_sync_notifier_unittest.cc

@@ -14,12 +14,12 @@
 #include "chromeos/crosapi/mojom/synced_session_client.mojom.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "components/sync/test/fake_synced_session_client_ash.h"
+#include "components/sync/test/test_sync_service.h"
 #include "components/sync_sessions/mock_sync_sessions_client.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_sessions/synced_session.h"
 #include "components/sync_sessions/synced_session_tracker.h"
-#include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -121,12 +121,16 @@ class CrosapiSessionSyncNotifierTest : public testing::Test {
     ON_CALL(mock_sync_sessions_client_, ShouldSyncURL(_))
         .WillByDefault(Return(true));
 
+    test_sync_service_ = std::make_unique<syncer::TestSyncService>();
+    test_sync_service_->GetUserSettings()->SetSelectedTypes(
+        /*sync_everything=*/false, /*types=*/{});
+
     // Create object under test.
     crosapi_session_sync_notifier_ =
         std::make_unique<CrosapiSessionSyncNotifier>(
             &mock_session_sync_service_,
-            synced_session_client_receiver_
-                .BindNewPipeAndPassRemoteWithVersion());
+            fake_synced_session_client_ash_.CreateRemote(),
+            test_sync_service_.get());
   }
 
   base::CallbackListSubscription SubscribeToForeignSessionsChanged(
@@ -208,6 +212,18 @@ class CrosapiSessionSyncNotifierTest : public testing::Test {
         .SetOnForeignSyncedPhoneSessionsUpdatedCallback(std::move(callback));
   }
 
+  syncer::TestSyncService* test_sync_service() {
+    return test_sync_service_.get();
+  }
+
+  syncer::FakeSyncedSessionClientAsh* fake_synced_session_client_ash() {
+    return &fake_synced_session_client_ash_;
+  }
+
+  CrosapiSessionSyncNotifier* crosapi_session_sync_notifier() {
+    return crosapi_session_sync_notifier_.get();
+  }
+
  private:
   // Helper to `CreateForeignPhonePresentableTabInSession()`, keeps all promises
   // made by that function. Finds the SessionWindow with id `window_id` to
@@ -304,20 +320,15 @@ class CrosapiSessionSyncNotifierTest : public testing::Test {
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
+  std::unique_ptr<syncer::TestSyncService> test_sync_service_;
   std::unique_ptr<CrosapiSessionSyncNotifier> crosapi_session_sync_notifier_;
-
   syncer::FakeSyncedSessionClientAsh fake_synced_session_client_ash_;
-  mojo::Receiver<crosapi::mojom::SyncedSessionClient>
-      synced_session_client_receiver_{&fake_synced_session_client_ash_};
 
   sync_sessions::SyncedSessionTracker synced_session_tracker_;
-
   testing::NiceMock<MockSessionSyncService> mock_session_sync_service_;
   testing::NiceMock<MockOpenTabsUIDelegate> mock_open_tabs_ui_delegate_;
-
   testing::NiceMock<sync_sessions::MockSyncSessionsClient>
       mock_sync_sessions_client_;
-
   std::vector<const sync_sessions::SyncedSession*> foreign_sessions_;
   base::RepeatingClosure foreign_sessions_changed_callback_;
 };
@@ -389,3 +400,27 @@ TEST_F(CrosapiSessionSyncNotifierTest,
   run_loop.Run();
   ValidateSentSessions();
 }
+
+TEST_F(CrosapiSessionSyncNotifierTest, SyncServiceObserverAdded) {
+  EXPECT_TRUE(
+      test_sync_service()->HasObserver(crosapi_session_sync_notifier()));
+}
+
+TEST_F(CrosapiSessionSyncNotifierTest, OnStateChanged_NoChangeToStartingValue) {
+  // OnStateChanged() is called from the CrosapiSessionSyncNotifier constructor.
+  // The "tab sync enabled" value should remain |false|
+  test_sync_service()->GetUserSettings()->SetSelectedTypes(
+      /*sync_everything=*/false, /*types=*/{});
+  test_sync_service()->FireStateChanged();
+  EXPECT_FALSE(fake_synced_session_client_ash()->is_session_sync_enabled());
+}
+
+TEST_F(CrosapiSessionSyncNotifierTest,
+       OnStateChanged_TabSyncEnabledStateChanged) {
+  // OnStateChange() is called if the "tab sync enabled" value changes
+  test_sync_service()->GetUserSettings()->SetSelectedTypes(
+      /*sync_everything=*/true, /*types=*/{});
+  test_sync_service()->FireStateChanged();
+  fake_synced_session_client_ash()->FlushMojoForTesting();
+  EXPECT_TRUE(fake_synced_session_client_ash()->is_session_sync_enabled());
+}

chrome/browser/lacros/sync/sync_crosapi_manager_lacros.cc

@@ -110,7 +110,7 @@ void SyncCrosapiManagerLacros::PostProfileInit(Profile* profile) {
 
   DCHECK(!crosapi_session_sync_notifier_);
   profile->AddObserver(this);
-  MaybeCreateCrosapiSessionSyncNotifier(lacros_service, profile);
+  MaybeCreateCrosapiSessionSyncNotifier(lacros_service, profile, sync_service);
 
   DCHECK(!sync_explicit_passphrase_client_);
   sync_explicit_passphrase_client_ =
@@ -135,7 +135,8 @@ void SyncCrosapiManagerLacros::OnSyncShutdown(
 
 void SyncCrosapiManagerLacros::MaybeCreateCrosapiSessionSyncNotifier(
     chromeos::LacrosService* lacros_service,
-    Profile* profile) {
+    Profile* profile,
+    syncer::SyncService* sync_service) {
   if (!base::FeatureList::IsEnabled(syncer::kChromeOSSyncedSessionSharing)) {
     return;
   }
@@ -154,14 +155,17 @@ void SyncCrosapiManagerLacros::MaybeCreateCrosapiSessionSyncNotifier(
   }
 
   lacros_service->GetRemote<crosapi::mojom::SyncService>()
-      ->CreateSyncedSessionClient(
-          base::BindOnce(&SyncCrosapiManagerLacros::OnCreateSyncedSessionClient,
-                         weak_ptr_factory_.GetWeakPtr(), session_sync_service));
+      ->CreateSyncedSessionClient(base::BindOnce(
+          &SyncCrosapiManagerLacros::OnCreateSyncedSessionClient,
+          weak_ptr_factory_.GetWeakPtr(), session_sync_service, sync_service));
 }
 
 void SyncCrosapiManagerLacros::OnCreateSyncedSessionClient(
     sync_sessions::SessionSyncService* session_sync_service,
+    syncer::SyncService* sync_service,
     mojo::PendingRemote<crosapi::mojom::SyncedSessionClient> pending_remote) {
+  // TODO(b/260599791): Handle the potential case where the profile or these
+  // passed-in services may be invalid by the time we receive the remote.
   if (!pending_remote) {
     return;
   }
@@ -174,5 +178,5 @@ void SyncCrosapiManagerLacros::OnCreateSyncedSessionClient(
 
   DCHECK(!crosapi_session_sync_notifier_);
   crosapi_session_sync_notifier_ = std::make_unique<CrosapiSessionSyncNotifier>(
-      session_sync_service, std::move(pending_remote));
+      session_sync_service, std::move(pending_remote), sync_service);
 }

chrome/browser/lacros/sync/sync_crosapi_manager_lacros.h

@@ -56,9 +56,11 @@ class SyncCrosapiManagerLacros : public syncer::SyncServiceObserver,
   //  - session sync service for the user's profile cannot be found.
   void MaybeCreateCrosapiSessionSyncNotifier(
       chromeos::LacrosService* lacros_service,
-      Profile* profile);
+      Profile* profile,
+      syncer::SyncService* sync_service);
   void OnCreateSyncedSessionClient(
       sync_sessions::SessionSyncService* session_sync_service,
+      syncer::SyncService* sync_service,
       mojo::PendingRemote<crosapi::mojom::SyncedSessionClient> pending_remote);
 
   // The objects below are created for main profile PostProfileInit() and

components/sync/test/fake_synced_session_client_ash.cc

@@ -20,6 +20,16 @@ void FakeSyncedSessionClientAsh::BindReceiver(
   receivers_.Add(this, std::move(receiver));
 }
 
+mojo::PendingRemote<crosapi::mojom::SyncedSessionClient>
+FakeSyncedSessionClientAsh::CreateRemote() {
+  mojo::PendingReceiver<crosapi::mojom::SyncedSessionClient> pending_receiver;
+  mojo::PendingRemote<crosapi::mojom::SyncedSessionClient> pending_remote =
+      pending_receiver.InitWithNewPipeAndPassRemote();
+  receivers_.Add(this, std::move(pending_receiver));
+  return {pending_remote.PassPipe(),
+          crosapi::mojom::SyncedSessionClient::Version_};
+}
+
 void FakeSyncedSessionClientAsh::OnForeignSyncedPhoneSessionsUpdated(
     std::vector<crosapi::mojom::SyncedSessionPtr> sessions) {
   last_foreign_synced_phone_sessions_ = std::move(sessions);
@@ -41,7 +51,11 @@ FakeSyncedSessionClientAsh::LookupForeignSyncedPhoneSessions() {
 }
 
 void FakeSyncedSessionClientAsh::OnSessionSyncEnabledChanged(bool enabled) {
-  NOTIMPLEMENTED();
+  is_session_sync_enabled_ = enabled;
+}
+
+void FakeSyncedSessionClientAsh::FlushMojoForTesting() {
+  receivers_.FlushForTesting();
 }
 
 }  // namespace syncer

components/sync/test/fake_synced_session_client_ash.h

@@ -24,6 +24,7 @@ class FakeSyncedSessionClientAsh : public crosapi::mojom::SyncedSessionClient {
   // crosapi::mojom::SyncedSessionClient:
   void OnForeignSyncedPhoneSessionsUpdated(
       std::vector<crosapi::mojom::SyncedSessionPtr> sessions) override;
+  void OnSessionSyncEnabledChanged(bool enabled) override;
 
   void SetOnForeignSyncedPhoneSessionsUpdatedCallback(
       base::RepeatingClosure callback);
@@ -34,17 +35,21 @@ class FakeSyncedSessionClientAsh : public crosapi::mojom::SyncedSessionClient {
   const std::vector<crosapi::mojom::SyncedSessionPtr>&
   LookupForeignSyncedPhoneSessions();
 
-  void OnSessionSyncEnabledChanged(bool enabled) override;
-
   void BindReceiver(
       mojo::PendingReceiver<crosapi::mojom::SyncedSessionClient> receiver);
+  mojo::PendingRemote<crosapi::mojom::SyncedSessionClient> CreateRemote();
+
+  bool is_session_sync_enabled() { return is_session_sync_enabled_; }
+
+  void FlushMojoForTesting();
 
  private:
   mojo::ReceiverSet<crosapi::mojom::SyncedSessionClient> receivers_;
   std::vector<crosapi::mojom::SyncedSessionPtr>
       last_foreign_synced_phone_sessions_;
   base::RepeatingClosure
       on_foreign_synced_phone_sessions_updated_complete_callback_;
+  bool is_session_sync_enabled_ = false;
 };
 
 }  // namespace syncer