[sync] Refactor to add TrustedVaultClient observer interface

No behavioral changes: this class groups the two preexisting observing
functions in a single interface to improve readability.

On iOS, some temporary code is introduced to avoid breaking the build or
the behavior for downstream implementations of
ChromeTrustedVaultService.

Change-Id: I28e8a78148ce65f69e07ff4782e3570e266d3c10
Bug: 1081649
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461007
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#815640}

chrome/browser/sync/test/integration/encryption_helper.cc

@@ -89,18 +89,14 @@ bool TrustedVaultKeyRequiredStateChecker::IsExitConditionSatisfied(
 
 TrustedVaultKeysChangedStateChecker::TrustedVaultKeysChangedStateChecker(
     syncer::ProfileSyncService* service)
-    : keys_changed_(false) {
-  // base::Unretained() is safe here, because callback won't be called once
-  // |subscription_| is destroyed.
-  subscription_ = service->GetSyncClientForTest()
-                      ->GetTrustedVaultClient()
-                      ->AddKeysChangedObserver(base::BindRepeating(
-                          &TrustedVaultKeysChangedStateChecker::OnKeysChanged,
-                          base::Unretained(this)));
+    : service_(service), keys_changed_(false) {
+  service->GetSyncClientForTest()->GetTrustedVaultClient()->AddObserver(this);
 }
 
-TrustedVaultKeysChangedStateChecker::~TrustedVaultKeysChangedStateChecker() =
-    default;
+TrustedVaultKeysChangedStateChecker::~TrustedVaultKeysChangedStateChecker() {
+  service_->GetSyncClientForTest()->GetTrustedVaultClient()->RemoveObserver(
+      this);
+}
 
 bool TrustedVaultKeysChangedStateChecker::IsExitConditionSatisfied(
     std::ostream* os) {
@@ -108,10 +104,14 @@ bool TrustedVaultKeysChangedStateChecker::IsExitConditionSatisfied(
   return keys_changed_;
 }
 
-void TrustedVaultKeysChangedStateChecker::OnKeysChanged() {
+void TrustedVaultKeysChangedStateChecker::OnTrustedVaultKeysChanged() {
   keys_changed_ = true;
+  CheckExitCondition();
 }
 
+void TrustedVaultKeysChangedStateChecker::
+    OnTrustedVaultRecoverabilityChanged() {}
+
 ScopedScryptFeatureToggler::ScopedScryptFeatureToggler(
     bool force_disabled,
     bool use_for_new_passphrases) {

chrome/browser/sync/test/integration/encryption_helper.h

@@ -72,19 +72,24 @@ class TrustedVaultKeyRequiredStateChecker
 };
 
 // Checker used to block until trusted vault keys are changed.
-class TrustedVaultKeysChangedStateChecker : public StatusChangeChecker {
+class TrustedVaultKeysChangedStateChecker
+    : public StatusChangeChecker,
+      syncer::TrustedVaultClient::Observer {
  public:
   explicit TrustedVaultKeysChangedStateChecker(
       syncer::ProfileSyncService* service);
   ~TrustedVaultKeysChangedStateChecker() override;
 
+  // StatusChangeChecker overrides.
   bool IsExitConditionSatisfied(std::ostream* os) override;
 
- private:
-  void OnKeysChanged();
+  // TrustedVaultClient::Observer overrides.
+  void OnTrustedVaultKeysChanged() override;
+  void OnTrustedVaultRecoverabilityChanged() override;
 
+ private:
+  syncer::ProfileSyncService* const service_;
   bool keys_changed_;
-  std::unique_ptr<syncer::TrustedVaultClient::Subscription> subscription_;
 };
 
 // Helper for setting scrypt-related feature flags.

chrome/browser/sync/trusted_vault_client_android.cc

@@ -64,13 +64,17 @@ void TrustedVaultClientAndroid::MarkKeysAsStaleCompleted(JNIEnv* env,
 }
 
 void TrustedVaultClientAndroid::NotifyKeysChanged(JNIEnv* env) {
-  observer_list_.Notify();
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultKeysChanged();
+  }
 }
 
-std::unique_ptr<TrustedVaultClientAndroid::Subscription>
-TrustedVaultClientAndroid::AddKeysChangedObserver(
-    const base::RepeatingClosure& cb) {
-  return observer_list_.Add(cb);
+void TrustedVaultClientAndroid::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void TrustedVaultClientAndroid::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
 }
 
 void TrustedVaultClientAndroid::FetchKeys(
@@ -138,14 +142,6 @@ void TrustedVaultClientAndroid::GetIsRecoverabilityDegraded(
   std::move(cb).Run(false);
 }
 
-std::unique_ptr<TrustedVaultClientAndroid::Subscription>
-TrustedVaultClientAndroid::AddRecoverabilityObserver(
-    const base::RepeatingClosure& cb) {
-  // TODO(crbug.com/1100279): Needs implementation.
-  NOTIMPLEMENTED();
-  return nullptr;
-}
-
 void TrustedVaultClientAndroid::AddTrustedRecoveryMethod(
     const std::string& gaia_id,
     const std::vector<uint8_t>& public_key,

chrome/browser/sync/trusted_vault_client_android.h

@@ -13,6 +13,7 @@
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/callback.h"
+#include "base/observer_list.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/driver/trusted_vault_client.h"
 
@@ -46,8 +47,8 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
   void NotifyKeysChanged(JNIEnv* env);
 
   // TrustedVaultClient implementation.
-  std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) override;
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
   void FetchKeys(
       const CoreAccountInfo& account_info,
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb)
@@ -60,8 +61,6 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
                        base::OnceCallback<void(bool)> cb) override;
   void GetIsRecoverabilityDegraded(const CoreAccountInfo& account_info,
                                    base::OnceCallback<void(bool)> cb) override;
-  std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& cb) override;
   void AddTrustedRecoveryMethod(const std::string& gaia_id,
                                 const std::vector<uint8_t>& public_key,
                                 base::OnceClosure cb) override;
@@ -86,7 +85,7 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
   // C++.
   base::OnceCallback<void(bool)> ongoing_mark_keys_as_stale_;
 
-  CallbackList observer_list_;
+  base::ObserverList<Observer> observer_list_;
 };
 
 #endif  // CHROME_BROWSER_SYNC_TRUSTED_VAULT_CLIENT_ANDROID_H_

components/sync/driver/sync_service_crypto.cc

@@ -38,10 +38,9 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
   ~EmptyTrustedVaultClient() override = default;
 
   // TrustedVaultClient implementation.
-  std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) override {
-    return nullptr;
-  }
+  void AddObserver(Observer* observer) override {}
+
+  void RemoveObserver(Observer* observer) override {}
 
   void FetchKeys(
       const CoreAccountInfo& account_info,
@@ -72,11 +71,6 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
     std::move(cb).Run(false);
   }
 
-  std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& cb) override {
-    return nullptr;
-  }
-
   void AddTrustedRecoveryMethod(const std::string& gaia_id,
                                 const std::vector<uint8_t>& public_key,
                                 base::OnceClosure cb) override {
@@ -232,18 +226,12 @@ SyncServiceCrypto::SyncServiceCrypto(
   DCHECK(sync_prefs_);
   DCHECK(trusted_vault_client_);
 
-  trusted_vault_client_keys_subscription_ =
-      trusted_vault_client_->AddKeysChangedObserver(base::BindRepeating(
-          &SyncServiceCrypto::OnTrustedVaultClientKeysChanged,
-          weak_factory_.GetWeakPtr()));
-
-  trusted_vault_client_recoverability_subscription_ =
-      trusted_vault_client_->AddRecoverabilityObserver(base::BindRepeating(
-          &SyncServiceCrypto::OnTrustedVaultClientRecoverabilityChanged,
-          weak_factory_.GetWeakPtr()));
+  trusted_vault_client_->AddObserver(this);
 }
 
-SyncServiceCrypto::~SyncServiceCrypto() = default;
+SyncServiceCrypto::~SyncServiceCrypto() {
+  trusted_vault_client_->RemoveObserver(this);
+}
 
 void SyncServiceCrypto::Reset() {
   state_ = State();
@@ -414,6 +402,34 @@ PassphraseType SyncServiceCrypto::GetPassphraseType() const {
   return state_.cached_passphrase_type;
 }
 
+void SyncServiceCrypto::SetSyncEngine(const CoreAccountInfo& account_info,
+                                      SyncEngine* engine) {
+  DCHECK(engine);
+  state_.account_info = account_info;
+  state_.engine = engine;
+
+  // Since there was no state changes during engine initialization, now the
+  // state is known and no user action required.
+  if (state_.required_user_action ==
+      RequiredUserAction::kUnknownDuringInitialization) {
+    UpdateRequiredUserActionAndNotify(RequiredUserAction::kNone);
+  }
+
+  // This indicates OnTrustedVaultKeyRequired() was called as part of the
+  // engine's initialization.
+  if (state_.required_user_action ==
+      RequiredUserAction::kFetchingTrustedVaultKeys) {
+    FetchTrustedVaultKeys(/*is_second_fetch_attempt=*/false);
+  }
+}
+
+std::unique_ptr<SyncEncryptionHandler::Observer>
+SyncServiceCrypto::GetEncryptionObserverProxy() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return std::make_unique<SyncEncryptionObserverProxy>(
+      weak_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get());
+}
+
 ModelTypeSet SyncServiceCrypto::GetEncryptedDataTypes() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(state_.encrypted_types.Has(PASSWORDS));
@@ -606,35 +622,7 @@ void SyncServiceCrypto::OnPassphraseTypeChanged(PassphraseType type,
   notify_observers_.Run();
 }
 
-void SyncServiceCrypto::SetSyncEngine(const CoreAccountInfo& account_info,
-                                      SyncEngine* engine) {
-  DCHECK(engine);
-  state_.account_info = account_info;
-  state_.engine = engine;
-
-  // Since there was no state changes during engine initialization, now the
-  // state is known and no user action required.
-  if (state_.required_user_action ==
-      RequiredUserAction::kUnknownDuringInitialization) {
-    UpdateRequiredUserActionAndNotify(RequiredUserAction::kNone);
-  }
-
-  // This indicates OnTrustedVaultKeyRequired() was called as part of the
-  // engine's initialization.
-  if (state_.required_user_action ==
-      RequiredUserAction::kFetchingTrustedVaultKeys) {
-    FetchTrustedVaultKeys(/*is_second_fetch_attempt=*/false);
-  }
-}
-
-std::unique_ptr<SyncEncryptionHandler::Observer>
-SyncServiceCrypto::GetEncryptionObserverProxy() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return std::make_unique<SyncEncryptionObserverProxy>(
-      weak_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get());
-}
-
-void SyncServiceCrypto::OnTrustedVaultClientKeysChanged() {
+void SyncServiceCrypto::OnTrustedVaultKeysChanged() {
   switch (state_.required_user_action) {
     case RequiredUserAction::kUnknownDuringInitialization:
     case RequiredUserAction::kNone:
@@ -662,7 +650,7 @@ void SyncServiceCrypto::OnTrustedVaultClientKeysChanged() {
   FetchTrustedVaultKeys(/*is_second_fetch_attempt=*/false);
 }
 
-void SyncServiceCrypto::OnTrustedVaultClientRecoverabilityChanged() {
+void SyncServiceCrypto::OnTrustedVaultRecoverabilityChanged() {
   RefreshIsRecoverabilityDegraded();
 }
 

components/sync/driver/sync_service_crypto.h

@@ -28,7 +28,8 @@ class CryptoSyncPrefs;
 // handles things related to encryption, including holding lots of state and
 // encryption communications with the sync thread.
 class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
-                          public DataTypeEncryptionHandler {
+                          public DataTypeEncryptionHandler,
+                          public TrustedVaultClient::Observer {
  public:
   // |sync_prefs| must not be null and must outlive this object.
   // |trusted_vault_client| may be null, but if non-null, the pointee must
@@ -62,6 +63,12 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
   // Returns the actual passphrase type being used for encryption.
   PassphraseType GetPassphraseType() const;
 
+  // Used to provide the engine when it is initialized.
+  void SetSyncEngine(const CoreAccountInfo& account_info, SyncEngine* engine);
+
+  // Creates a proxy observer object that will post calls to this thread.
+  std::unique_ptr<SyncEncryptionHandler::Observer> GetEncryptionObserverProxy();
+
   // SyncEncryptionHandler::Observer implementation.
   void OnPassphraseRequired(
       PassphraseRequiredReason reason,
@@ -84,11 +91,9 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
   bool HasCryptoError() const override;
   ModelTypeSet GetEncryptedDataTypes() const override;
 
-  // Used to provide the engine when it is initialized.
-  void SetSyncEngine(const CoreAccountInfo& account_info, SyncEngine* engine);
-
-  // Creates a proxy observer object that will post calls to this thread.
-  std::unique_ptr<SyncEncryptionHandler::Observer> GetEncryptionObserverProxy();
+  // TrustedVaultClient::Observer implementation.
+  void OnTrustedVaultKeysChanged() override;
+  void OnTrustedVaultRecoverabilityChanged() override;
 
   bool encryption_pending() const { return state_.encryption_pending; }
 
@@ -113,10 +118,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
     kTrustedVaultRecoverabilityDegraded,
   };
 
-  // Observer methods invoked by TrustedVaultClient when its content changes.
-  void OnTrustedVaultClientKeysChanged();
-  void OnTrustedVaultClientRecoverabilityChanged();
-
   // Reads trusted vault keys from the client and feeds them to the sync engine.
   void FetchTrustedVaultKeys(bool is_second_fetch_attempt);
 
@@ -157,12 +158,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
   // Never null and guaranteed to outlive us.
   TrustedVaultClient* const trusted_vault_client_;
 
-  // Subscriptions to observe changes in |*trusted_vault_client_|.
-  std::unique_ptr<TrustedVaultClient::Subscription>
-      trusted_vault_client_keys_subscription_;
-  std::unique_ptr<TrustedVaultClient::Subscription>
-      trusted_vault_client_recoverability_subscription_;
-
   // All the mutable state is wrapped in a struct so that it can be easily
   // reset to its default values.
   struct State {

components/sync/driver/sync_service_crypto_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/bind_helpers.h"
+#include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/test/mock_callback.h"
@@ -151,13 +152,18 @@ class TestTrustedVaultClient : public TrustedVaultClient {
 
   void SetIsRecoverabilityDegraded(bool is_recoverability_degraded) {
     is_recoverability_degraded_ = is_recoverability_degraded;
-    recoverability_observer_list_.Notify();
+    for (Observer& observer : observer_list_) {
+      observer.OnTrustedVaultRecoverabilityChanged();
+    }
   }
 
   // TrustedVaultClient implementation.
-  std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) override {
-    return keys_observer_list_.Add(cb);
+  void AddObserver(Observer* observer) override {
+    observer_list_.AddObserver(observer);
+  }
+
+  void RemoveObserver(Observer* observer) override {
+    observer_list_.RemoveObserver(observer);
   }
 
   void FetchKeys(
@@ -203,12 +209,16 @@ class TestTrustedVaultClient : public TrustedVaultClient {
     CachedKeysPerUser& cached_keys = gaia_id_to_cached_keys_[gaia_id];
     cached_keys.keys = keys;
     cached_keys.marked_as_stale = false;
-    keys_observer_list_.Notify();
+    for (Observer& observer : observer_list_) {
+      observer.OnTrustedVaultKeysChanged();
+    }
   }
 
   void RemoveAllStoredKeys() override {
     gaia_id_to_cached_keys_.clear();
-    keys_observer_list_.Notify();
+    for (Observer& observer : observer_list_) {
+      observer.OnTrustedVaultKeysChanged();
+    }
   }
 
   void MarkKeysAsStale(const CoreAccountInfo& account_info,
@@ -237,11 +247,6 @@ class TestTrustedVaultClient : public TrustedVaultClient {
     std::move(cb).Run(is_recoverability_degraded_);
   }
 
-  std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& cb) override {
-    return recoverability_observer_list_.Add(cb);
-  }
-
   void AddTrustedRecoveryMethod(const std::string& gaia_id,
                                 const std::vector<uint8_t>& public_key,
                                 base::OnceClosure cb) override {
@@ -258,8 +263,7 @@ class TestTrustedVaultClient : public TrustedVaultClient {
   const TestTrustedVaultServer* const server_;
 
   std::map<std::string, CachedKeysPerUser> gaia_id_to_cached_keys_;
-  CallbackList keys_observer_list_;
-  CallbackList recoverability_observer_list_;
+  base::ObserverList<Observer> observer_list_;
   int fetch_count_ = 0;
   int keys_marked_as_stale_count_ = 0;
   int get_is_recoverablity_degraded_call_count_ = 0;
@@ -294,7 +298,7 @@ class SyncServiceCryptoTest : public testing::Test {
 
   bool VerifyAndClearExpectations() {
     return testing::Mock::VerifyAndClearExpectations(&notify_observers_cb_) &&
-           testing::Mock::VerifyAndClearExpectations(&notify_observers_cb_) &&
+           testing::Mock::VerifyAndClearExpectations(&reconfigure_cb_) &&
            testing::Mock::VerifyAndClearExpectations(&trusted_vault_client_) &&
            testing::Mock::VerifyAndClearExpectations(&engine_);
   }

components/sync/driver/trusted_vault_client.h

@@ -10,8 +10,9 @@
 #include <vector>
 
 #include "base/callback_forward.h"
-#include "base/callback_list.h"
 #include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 
 struct CoreAccountInfo;
 
@@ -21,17 +22,26 @@ namespace syncer {
 // available trusted vault encryption keys.
 class TrustedVaultClient {
  public:
+  class Observer : public base::CheckedObserver {
+   public:
+    Observer() = default;
+    Observer(const Observer&) = delete;
+    Observer& operator=(const Observer&) = delete;
+    ~Observer() override = default;
+
+    // Invoked when the keys inside the vault have changed.
+    virtual void OnTrustedVaultKeysChanged() = 0;
+
+    // Invoked when the recoverability of the keys has changed.
+    virtual void OnTrustedVaultRecoverabilityChanged() = 0;
+  };
+
   TrustedVaultClient() = default;
   virtual ~TrustedVaultClient() = default;
 
-  using CallbackList = base::CallbackList<void()>;
-  using Subscription = CallbackList::Subscription;
-
-  // Registers an observer-like callback that will be invoked when the content
-  // of the vault has changed (e.g. new keys added). The subscription must not
-  // outlive |*this|.
-  virtual std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) = 0;
+  // Adds/removes an observer.
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
 
   // Attempts to fetch decryption keys, required by sync to resume.
   // Implementations are expected to NOT prompt the user for actions. |cb| is
@@ -74,14 +84,6 @@ class TrustedVaultClient {
       const CoreAccountInfo& account_info,
       base::OnceCallback<void(bool)> cb) = 0;
 
-  // Registers an observer-like callback that will be invoked when the
-  // recoverability of the keys has changed. The subscription must not outlive
-  // |*this|.
-  // TODO(crbug.com/1081649): Unify with AddKeysChangedObserver() by introducing
-  // an observer interface instead.
-  virtual std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& cb) = 0;
-
   // Registers a new trusted recovery method that can be used to retrieve keys,
   // usually for the purpose of resolving a recoverability-degraded case
   // surfaced by GetIsRecoverabilityDegraded().

components/sync/trusted_vault/standalone_trusted_vault_client.cc

@@ -163,11 +163,14 @@ StandaloneTrustedVaultClient::StandaloneTrustedVaultClient(
 
 StandaloneTrustedVaultClient::~StandaloneTrustedVaultClient() = default;
 
-std::unique_ptr<StandaloneTrustedVaultClient::Subscription>
-StandaloneTrustedVaultClient::AddKeysChangedObserver(
-    const base::RepeatingClosure& cb) {
+void StandaloneTrustedVaultClient::AddObserver(Observer* observer) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return keys_observer_list_.Add(cb);
+  observer_list_.AddObserver(observer);
+}
+
+void StandaloneTrustedVaultClient::RemoveObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observer_list_.RemoveObserver(observer);
 }
 
 void StandaloneTrustedVaultClient::FetchKeys(
@@ -190,7 +193,9 @@ void StandaloneTrustedVaultClient::StoreKeys(
   backend_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&StandaloneTrustedVaultBackend::StoreKeys,
                                 backend_, gaia_id, keys, last_key_version));
-  keys_observer_list_.Notify();
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultKeysChanged();
+  }
 }
 
 void StandaloneTrustedVaultClient::RemoveAllStoredKeys() {
@@ -200,7 +205,9 @@ void StandaloneTrustedVaultClient::RemoveAllStoredKeys() {
       FROM_HERE,
       base::BindOnce(&StandaloneTrustedVaultBackend::RemoveAllStoredKeys,
                      backend_));
-  keys_observer_list_.Notify();
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultKeysChanged();
+  }
 }
 
 void StandaloneTrustedVaultClient::MarkKeysAsStale(
@@ -227,13 +234,6 @@ void StandaloneTrustedVaultClient::GetIsRecoverabilityDegraded(
           account_info, BindToCurrentSequence(std::move(cb))));
 }
 
-std::unique_ptr<StandaloneTrustedVaultClient::Subscription>
-StandaloneTrustedVaultClient::AddRecoverabilityObserver(
-    const base::RepeatingClosure& cb) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return recoverability_observer_list_.Add(cb);
-}
-
 void StandaloneTrustedVaultClient::AddTrustedRecoveryMethod(
     const std::string& gaia_id,
     const std::vector<uint8_t>& public_key,
@@ -278,7 +278,9 @@ void StandaloneTrustedVaultClient::SetRecoverabilityDegradedForTesting() {
 
 void StandaloneTrustedVaultClient::NotifyRecoverabilityDegradedChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  recoverability_observer_list_.Notify();
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultRecoverabilityChanged();
+  }
 }
 
 }  // namespace syncer

components/sync/trusted_vault/standalone_trusted_vault_client.h

@@ -13,6 +13,7 @@
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
@@ -50,8 +51,8 @@ class StandaloneTrustedVaultClient : public TrustedVaultClient {
   ~StandaloneTrustedVaultClient() override;
 
   // TrustedVaultClient implementation.
-  std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) override;
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
   void FetchKeys(
       const CoreAccountInfo& account_info,
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb)
@@ -64,8 +65,6 @@ class StandaloneTrustedVaultClient : public TrustedVaultClient {
                        base::OnceCallback<void(bool)> cb) override;
   void GetIsRecoverabilityDegraded(const CoreAccountInfo& account_info,
                                    base::OnceCallback<void(bool)> cb) override;
-  std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& cb) override;
   void AddTrustedRecoveryMethod(const std::string& gaia_id,
                                 const std::vector<uint8_t>& public_key,
                                 base::OnceClosure cb) override;
@@ -84,8 +83,7 @@ class StandaloneTrustedVaultClient : public TrustedVaultClient {
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  CallbackList keys_observer_list_;
-  CallbackList recoverability_observer_list_;
+  base::ObserverList<Observer> observer_list_;
 
   // Allows access token fetching for primary account on the ui thread. Passed
   // as WeakPtr to TrustedVaultAccessTokenFetcherImpl.

ios/chrome/browser/sync/ios_trusted_vault_client.h

@@ -15,8 +15,8 @@ class IOSTrustedVaultClient : public syncer::TrustedVaultClient {
   ~IOSTrustedVaultClient() override;
 
   // TrustedVaultClient implementation.
-  std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& closure) override;
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
   void FetchKeys(
       const CoreAccountInfo& account_info,
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)>
@@ -30,8 +30,6 @@ class IOSTrustedVaultClient : public syncer::TrustedVaultClient {
   void GetIsRecoverabilityDegraded(
       const CoreAccountInfo& account_info,
       base::OnceCallback<void(bool)> callback) override;
-  std::unique_ptr<Subscription> AddRecoverabilityObserver(
-      const base::RepeatingClosure& callback) override;
   void AddTrustedRecoveryMethod(const std::string& gaia_id,
                                 const std::vector<uint8_t>& public_key,
                                 base::OnceClosure callback) override;

ios/chrome/browser/sync/ios_trusted_vault_client.mm

@@ -17,17 +17,24 @@ IOSTrustedVaultClient::IOSTrustedVaultClient() {}
 
 IOSTrustedVaultClient::~IOSTrustedVaultClient() = default;
 
-std::unique_ptr<IOSTrustedVaultClient::Subscription>
-IOSTrustedVaultClient::AddKeysChangedObserver(
-    const base::RepeatingClosure& closure) {
+void IOSTrustedVaultClient::AddObserver(Observer* observer) {
   ios::ChromeBrowserProvider* browser_provider =
       ios::GetChromeBrowserProvider();
   ios::ChromeTrustedVaultService* trusted_vault_service =
       browser_provider->GetChromeTrustedVaultService();
-  if (!trusted_vault_service) {
-    return nullptr;
+  if (trusted_vault_service) {
+    trusted_vault_service->AddObserver(observer);
+  }
+}
+
+void IOSTrustedVaultClient::RemoveObserver(Observer* observer) {
+  ios::ChromeBrowserProvider* browser_provider =
+      ios::GetChromeBrowserProvider();
+  ios::ChromeTrustedVaultService* trusted_vault_service =
+      browser_provider->GetChromeTrustedVaultService();
+  if (trusted_vault_service) {
+    trusted_vault_service->RemoveObserver(observer);
   }
-  return trusted_vault_service->AddKeysChangedObserver(closure);
 }
 
 void IOSTrustedVaultClient::FetchKeys(
@@ -73,13 +80,6 @@ void IOSTrustedVaultClient::GetIsRecoverabilityDegraded(
   std::move(callback).Run(false);
 }
 
-std::unique_ptr<IOSTrustedVaultClient::Subscription>
-IOSTrustedVaultClient::AddRecoverabilityObserver(
-    const base::RepeatingClosure& callback) {
-  // TODO(crbug.com/1100278): Needs implementation.
-  return nullptr;
-}
-
 void IOSTrustedVaultClient::AddTrustedRecoveryMethod(
     const std::string& gaia_id,
     const std::vector<uint8_t>& public_key,

ios/chrome/test/BUILD.gn

@@ -338,6 +338,7 @@ test("ios_chrome_unittests") {
     "//ios/chrome/content_widget_extension:unit_tests",
     "//ios/chrome/credential_provider_extension:unit_tests",
     "//ios/chrome/search_widget_extension:unit_tests",
+    "//ios/public/provider/chrome/browser/signin:unit_tests",
     "//ios/testing:http_server_bundle_data",
   ]
 

ios/public/provider/chrome/browser/signin/BUILD.gn

@@ -22,6 +22,7 @@ source_set("signin") {
   ]
   deps = [
     "//base",
+    "//components/sync/driver",
     "//google_apis",
   ]
 }
@@ -74,3 +75,15 @@ source_set("constants") {
   ]
   deps = [ "//base" ]
 }
+
+source_set("unit_tests") {
+  configs += [ "//build/config/compiler:enable_arc" ]
+  testonly = true
+  sources = [ "chrome_trusted_vault_service_unittest.mm" ]
+  deps = [
+    ":signin",
+    "//base",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+}

ios/public/provider/chrome/browser/signin/DEPS

+include_rules = [
+  "+components/sync/driver/trusted_vault_client.h",
+]
+
 specific_include_rules = {
   "test_signin_resources_provider\.mm": [
     "+ui/base/test/ios/ui_image_test_utils.h",

ios/public/provider/chrome/browser/signin/chrome_trusted_vault_service.h

@@ -5,10 +5,13 @@
 #ifndef IOS_PUBLIC_PROVIDER_CHROME_BROWSER_SIGNIN_CHROME_TRUSTED_VAULT_SERVICE_H_
 #define IOS_PUBLIC_PROVIDER_CHROME_BROWSER_SIGNIN_CHROME_TRUSTED_VAULT_SERVICE_H_
 
+#include <memory>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/callback_list.h"
+#include "base/observer_list.h"
+#include "components/sync/driver/trusted_vault_client.h"
 
 @class ChromeIdentity;
 @class UIViewController;
@@ -27,9 +30,14 @@ class ChromeTrustedVaultService {
   ChromeTrustedVaultService& operator=(const ChromeTrustedVaultService&) =
       delete;
 
+  using Observer = syncer::TrustedVaultClient::Observer;
   using CallbackList = base::CallbackList<void()>;
   using Subscription = CallbackList::Subscription;
 
+  // Adds/removes observers.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
   // Asynchronously fetch the shared keys for |identity|
   // and returns them by calling |callback|.
   virtual void FetchKeys(
@@ -48,8 +56,20 @@ class ChromeTrustedVaultService {
   // synchronously.
   virtual void CancelReauthentication(BOOL animated,
                                       void (^callback)(void)) = 0;
+  // TODO(crbug.com/1100278): Delete this deprecated function.
   virtual std::unique_ptr<Subscription> AddKeysChangedObserver(
-      const base::RepeatingClosure& cb) = 0;
+      const base::RepeatingClosure& cb);
+
+ protected:
+  // Functions to notify observers.
+  void NotifyKeysChanged();
+  void NotifyRecoverabilityChanged();
+
+ private:
+  base::ObserverList<Observer> observer_list_;
+  // TODO(crbug.com/1100278): Delete this field onceAddKeysChangedObserver() is
+  // cleaned up.
+  std::unique_ptr<Subscription> deprecated_keys_changed_subscription_;
 };
 
 }  // namespace ios

ios/public/provider/chrome/browser/signin/chrome_trusted_vault_service.mm

@@ -14,4 +14,38 @@ ChromeTrustedVaultService::ChromeTrustedVaultService() {}
 
 ChromeTrustedVaultService::~ChromeTrustedVaultService() {}
 
+void ChromeTrustedVaultService::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+
+  if (!deprecated_keys_changed_subscription_) {
+    // base::Unretained() is safe because the subscription is stored in a member
+    // field and hence cannot outlive |this|.
+    deprecated_keys_changed_subscription_ = AddKeysChangedObserver(
+        base::BindRepeating(&ChromeTrustedVaultService::NotifyKeysChanged,
+                            base::Unretained(this)));
+  }
+}
+
+void ChromeTrustedVaultService::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+std::unique_ptr<ChromeTrustedVaultService::Subscription>
+ChromeTrustedVaultService::AddKeysChangedObserver(
+    const base::RepeatingClosure& cb) {
+  return nullptr;
+}
+
+void ChromeTrustedVaultService::NotifyKeysChanged() {
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultKeysChanged();
+  }
+}
+
+void ChromeTrustedVaultService::NotifyRecoverabilityChanged() {
+  for (Observer& observer : observer_list_) {
+    observer.OnTrustedVaultRecoverabilityChanged();
+  }
+}
+
 }  // namespace ios

ios/public/provider/chrome/browser/signin/chrome_trusted_vault_service_unittest.mm

+// Copyright 2020 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.
+
+#import "ios/public/provider/chrome/browser/signin/chrome_trusted_vault_service.h"
+
+#import "testing/gmock/include/gmock/gmock.h"
+#import "testing/gtest/include/gtest/gtest.h"
+#import "testing/platform_test.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace ios {
+
+namespace {
+
+class TestChromeTrustedVaultService : public ChromeTrustedVaultService {
+ public:
+  TestChromeTrustedVaultService() = default;
+  ~TestChromeTrustedVaultService() override = default;
+
+  // Expose publicly for testing.
+  using ChromeTrustedVaultService::NotifyKeysChanged;
+  using ChromeTrustedVaultService::NotifyRecoverabilityChanged;
+
+  // ChromeTrustedVaultService overrides.
+  void FetchKeys(ChromeIdentity* chrome_identity,
+                 base::OnceCallback<void(const TrustedVaultSharedKeyList&)>
+                     callback) override {}
+  void Reauthentication(ChromeIdentity* chrome_identity,
+                        UIViewController* presentingViewController,
+                        void (^callback)(BOOL success,
+                                         NSError* error)) override {}
+  void CancelReauthentication(BOOL animated, void (^callback)(void)) override {}
+};
+
+class MockObserver : public syncer::TrustedVaultClient::Observer {
+ public:
+  MockObserver() = default;
+  ~MockObserver() override = default;
+
+  MOCK_METHOD0(OnTrustedVaultKeysChanged, void());
+  MOCK_METHOD0(OnTrustedVaultRecoverabilityChanged, void());
+};
+
+using ChromeTrustedVaultServiceTest = PlatformTest;
+
+TEST_F(ChromeTrustedVaultServiceTest, ShouldNotifyKeysChanged) {
+  testing::NiceMock<MockObserver> observer;
+  TestChromeTrustedVaultService service;
+  service.AddObserver(&observer);
+
+  EXPECT_CALL(observer, OnTrustedVaultKeysChanged());
+  service.NotifyKeysChanged();
+}
+
+TEST_F(ChromeTrustedVaultServiceTest, ShouldNotifyRecoverabilityChanged) {
+  testing::NiceMock<MockObserver> observer;
+  TestChromeTrustedVaultService service;
+  service.AddObserver(&observer);
+
+  EXPECT_CALL(observer, OnTrustedVaultRecoverabilityChanged());
+  service.NotifyRecoverabilityChanged();
+}
+
+TEST_F(ChromeTrustedVaultServiceTest, ShouldRemoveObserver) {
+  testing::NiceMock<MockObserver> observer;
+  TestChromeTrustedVaultService service;
+  service.AddObserver(&observer);
+  service.RemoveObserver(&observer);
+
+  EXPECT_CALL(observer, OnTrustedVaultKeysChanged()).Times(0);
+  service.NotifyKeysChanged();
+}
+
+}  // namespace
+
+}  // namespace ios