PasswordAccountStorageOptInWatcher: Observe SyncService instead of IdentityManager

Reason: The underlying state that's being watched (i.e.
password_manager_util::IsOptedInForAccountStorage) gets the relevant
account-related state from SyncService, not from IdentityManager.
So this is more consistent, and ensures that there are no
order-of-observers problems. (SyncService itself observes the
IdentityManager, so before this CL, the outcome could depend on whether
SyncService or PasswordAccountStorageOptInWatcher get notified first.)

Bug: 1024332
Change-Id: I916c6035961056d46a2061a3eeb9362467ac8d26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151871
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760088}

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/password_manager/bulk_leak_check_service_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "extensions/browser/extension_system_provider.h"
 
@@ -37,6 +38,7 @@ PasswordsPrivateDelegateFactory::PasswordsPrivateDelegateFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(BulkLeakCheckServiceFactory::GetInstance());
   DependsOn(PasswordStoreFactory::GetInstance());
+  DependsOn(ProfileSyncServiceFactory::GetInstance());
 }
 
 PasswordsPrivateDelegateFactory::~PasswordsPrivateDelegateFactory() = default;

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -144,8 +144,8 @@ PasswordsPrivateDelegateImpl::PasswordsPrivateDelegateImpl(Profile* profile)
       password_account_storage_opt_in_watcher_(
           std::make_unique<
               password_manager::PasswordAccountStorageOptInWatcher>(
-              IdentityManagerFactory::GetForProfile(profile_),
               profile_->GetPrefs(),
+              ProfileSyncServiceFactory::GetForProfile(profile_),
               base::BindRepeating(&PasswordsPrivateDelegateImpl::
                                       OnAccountStorageOptInStateChanged,
                                   base::Unretained(this)))),

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc

@@ -308,12 +308,12 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestShouldReauthForOptIn) {
   scoped_feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
 
-  PasswordsPrivateDelegateImpl delegate(&profile_);
-
   auto* test_sync_service = static_cast<syncer::SyncService*>(
       ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
           &profile_, base::BindRepeating(&BuildTestSyncService)));
 
+  PasswordsPrivateDelegateImpl delegate(&profile_);
+
   password_manager_util::SetAccountStorageOptIn(profile_.GetPrefs(),
                                                 test_sync_service, false);
 
@@ -329,12 +329,12 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestShouldNotReauthForOptOut) {
   scoped_feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
 
-  PasswordsPrivateDelegateImpl delegate(&profile_);
-
   auto* test_sync_service = static_cast<syncer::SyncService*>(
       ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
           &profile_, base::BindRepeating(&BuildTestSyncService)));
 
+  PasswordsPrivateDelegateImpl delegate(&profile_);
+
   password_manager_util::SetAccountStorageOptIn(profile_.GetPrefs(),
                                                 test_sync_service, true);
 

components/password_manager/core/browser/password_account_storage_opt_in_watcher.cc

@@ -8,18 +8,19 @@
 
 #include "base/callback.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/sync/driver/sync_service.h"
 
 namespace password_manager {
 
 PasswordAccountStorageOptInWatcher::PasswordAccountStorageOptInWatcher(
-    signin::IdentityManager* identity_manager,
     PrefService* pref_service,
+    syncer::SyncService* sync_service,
     base::RepeatingClosure change_callback)
-    : identity_manager_(identity_manager),
+    : sync_service_(sync_service),
       change_callback_(std::move(change_callback)) {
   // The opt-in state is per-account, so it can change whenever the state of the
-  // signed-in account (aka unconsented primary account) changes.
-  identity_manager_->AddObserver(this);
+  // account used by Sync changes.
+  sync_service_->AddObserver(this);
   // The opt-in state is stored in a pref, so changes to the pref might indicate
   // a change to the opt-in state.
   pref_change_registrar_.Init(pref_service);
@@ -29,11 +30,11 @@ PasswordAccountStorageOptInWatcher::PasswordAccountStorageOptInWatcher(
 }
 
 PasswordAccountStorageOptInWatcher::~PasswordAccountStorageOptInWatcher() {
-  identity_manager_->RemoveObserver(this);
+  sync_service_->RemoveObserver(this);
 }
 
-void PasswordAccountStorageOptInWatcher::OnUnconsentedPrimaryAccountChanged(
-    const CoreAccountInfo& unconsented_primary_account_info) {
+void PasswordAccountStorageOptInWatcher::OnStateChanged(
+    syncer::SyncService* sync_service) {
   change_callback_.Run();
 }
 

components/password_manager/core/browser/password_account_storage_opt_in_watcher.h

@@ -7,32 +7,34 @@
 
 #include "base/callback_forward.h"
 #include "components/prefs/pref_change_registrar.h"
-#include "components/signin/public/identity_manager/identity_manager.h"
+#include "components/sync/driver/sync_service_observer.h"
 
 class PrefService;
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 namespace password_manager {
 
 // Helper class to watch for changes to the opt-in state for the account-scoped
 // password storage (see password_manager_util::IsOptedInForAccountStorage()).
-class PasswordAccountStorageOptInWatcher
-    : public signin::IdentityManager::Observer {
+class PasswordAccountStorageOptInWatcher : public syncer::SyncServiceObserver {
  public:
-  // |identity_manager| and |pref_service| must not be null and must outlive
-  // this object.
+  // |pref_service| and |sync_service| must not be null and must outlive this
+  // object.
   // |change_callback| will be invoked whenever the state of
   // password_manager_util::IsOptedInForAccountStorage() might have changed.
-  PasswordAccountStorageOptInWatcher(signin::IdentityManager* identity_manager,
-                                     PrefService* pref_service,
+  PasswordAccountStorageOptInWatcher(PrefService* pref_service,
+                                     syncer::SyncService* sync_service,
                                      base::RepeatingClosure change_callback);
   ~PasswordAccountStorageOptInWatcher() override;
 
-  // identity::IdentityManager::Observer:
-  void OnUnconsentedPrimaryAccountChanged(
-      const CoreAccountInfo& unconsented_primary_account_info) override;
+  // syncer::SyncServiceObserver:
+  void OnStateChanged(syncer::SyncService* sync_service) override;
 
  private:
-  signin::IdentityManager* const identity_manager_;
+  syncer::SyncService* const sync_service_;
   base::RepeatingClosure change_callback_;
 
   PrefChangeRegistrar pref_change_registrar_;

components/password_manager/core/browser/sync/password_model_type_controller.cc

@@ -32,8 +32,8 @@ PasswordModelTypeController::PasswordModelTypeController(
       sync_service_(sync_service),
       state_changed_callback_(state_changed_callback),
       account_storage_opt_in_watcher_(
-          identity_manager_,
           pref_service_,
+          sync_service_,
           base::BindRepeating(
               &PasswordModelTypeController::OnOptInStateMaybeChanged,
               base::Unretained(this))) {