Revert "Delete GaiaCookieManagerServiceObserver in favor of callbacks."

This reverts commit 58fe1e69.

Reason for revert: This looks like it breaks an ios internal trybot https://ci.chromium.org/p/chrome/builders/luci.chrome.ci/ios-internal-chromium-tot

Original change's description:
> Delete GaiaCookieManagerServiceObserver in favor of callbacks.
> 
> This CL replaces GaiaCookieManagerServiceObserver with vanilla
> callbacks. Now that GCMS is owned by IdentityManager, and cannot have
> multiple observers, a callback interface seems more natural than an
> Observer class.
> 
> This also has the nice side effect of removing transitive includes of
> gaia_cookie_manager_service.h via identity_manager.h.
> 
> Change-Id: I9cdac56c8237c9a09eb2c2ce87f41f48872a9fb6
> Bug: 939372
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1630162
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Reviewed-by: Colin Blundell <blundell@chromium.org>
> Commit-Queue: Lowell Manners <lowell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#665616}

TBR=blundell@chromium.org,msarda@chromium.org,lowell@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 939372
Change-Id: I777df13eb9bc5eddeea46640b973c287d6622061
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1642232Reviewed-by: Lowell Manners <lowell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666007}

chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc

@@ -7,7 +7,6 @@
 #include <vector>
 
 #include "base/logging.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "services/identity/public/cpp/accounts_cookie_mutator.h"
 

components/signin/core/browser/gaia_cookie_manager_service.cc

@@ -654,6 +654,14 @@ void GaiaCookieManagerService::LogOutAllAccounts(gaia::GaiaSource source) {
   }
 }
 
+void GaiaCookieManagerService::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void GaiaCookieManagerService::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
 void GaiaCookieManagerService::CancelAll() {
   VLOG(1) << "GaiaCookieManagerService::CancelAll";
   gaia_auth_fetcher_.reset();
@@ -687,8 +695,8 @@ void GaiaCookieManagerService::OnCookieChange(
 
   if (cause == network::mojom::CookieChangeCause::EXPLICIT) {
     DCHECK(net::CookieChangeCauseIsDeletion(net::CookieChangeCause::EXPLICIT));
-    if (gaia_cookie_deleted_by_user_action_callback_) {
-      gaia_cookie_deleted_by_user_action_callback_.Run();
+    for (auto& observer : observer_list_) {
+      observer.OnGaiaCookieDeletedByUserAction();
     }
   }
 
@@ -736,18 +744,6 @@ void GaiaCookieManagerService::SignalSetAccountsComplete(
   requests_.front().RunSetAccountsInCookieCompletedCallback(result);
 }
 
-void GaiaCookieManagerService::SetGaiaAccountsInCookieUpdatedCallback(
-    GaiaAccountsInCookieUpdatedCallback callback) {
-  DCHECK(!gaia_accounts_updated_in_cookie_callback_);
-  gaia_accounts_updated_in_cookie_callback_ = std::move(callback);
-}
-
-void GaiaCookieManagerService::SetGaiaCookieDeletedByUserActionCallback(
-    GaiaCookieDeletedByUserActionCallback callback) {
-  DCHECK(!gaia_cookie_deleted_by_user_action_callback_);
-  gaia_cookie_deleted_by_user_action_callback_ = std::move(callback);
-}
-
 void GaiaCookieManagerService::OnUbertokenFetchComplete(
     GoogleServiceAuthError error,
     const std::string& uber_token) {
@@ -856,8 +852,8 @@ void GaiaCookieManagerService::OnListAccountsSuccess(const std::string& data) {
   // services, in response to OnGaiaAccountsInCookieUpdated, may try in return
   // to call ListAccounts, which would immediately return false if the
   // ListAccounts request is still sitting in queue.
-  if (gaia_accounts_updated_in_cookie_callback_) {
-    gaia_accounts_updated_in_cookie_callback_.Run(
+  for (auto& observer : observer_list_) {
+    observer.OnGaiaAccountsInCookieUpdated(
         listed_accounts_, signed_out_accounts_,
         GoogleServiceAuthError(GoogleServiceAuthError::NONE));
   }
@@ -885,12 +881,10 @@ void GaiaCookieManagerService::OnListAccountsFailure(
   }
 
   RecordListAccountsFailure(error.state());
-
-  if (gaia_accounts_updated_in_cookie_callback_) {
-    gaia_accounts_updated_in_cookie_callback_.Run(listed_accounts_,
-                                                  signed_out_accounts_, error);
+  for (auto& observer : observer_list_) {
+    observer.OnGaiaAccountsInCookieUpdated(listed_accounts_,
+                                           signed_out_accounts_, error);
   }
-
   HandleNextRequest();
 }
 

components/signin/core/browser/gaia_cookie_manager_service.h

@@ -88,12 +88,6 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
                                   const GoogleServiceAuthError&)>
       AddAccountToCookieCompletedCallback;
 
-  typedef base::RepeatingCallback<void(const std::vector<gaia::ListedAccount>&,
-                                       const std::vector<gaia::ListedAccount>&,
-                                       const GoogleServiceAuthError&)>
-      GaiaAccountsInCookieUpdatedCallback;
-  typedef base::RepeatingCallback<void()> GaiaCookieDeletedByUserActionCallback;
-
   // Contains the information and parameters for any request.
   class GaiaCookieRequest {
    public:
@@ -154,6 +148,28 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
     DISALLOW_COPY_AND_ASSIGN(GaiaCookieRequest);
   };
 
+  class Observer {
+   public:
+    // Called whenever the GaiaCookieManagerService's list of GAIA accounts is
+    // updated. The GCMS monitors the APISID cookie and triggers a /ListAccounts
+    // call on change. The GCMS will also call ListAccounts upon the first call
+    // to ListAccounts(). The GCMS will delay calling ListAccounts if other
+    // requests are in queue that would modify the APISID cookie.
+    // If the ListAccounts call fails and the GCMS cannot recover, the reason
+    // is passed in |error|.
+    virtual void OnGaiaAccountsInCookieUpdated(
+        const std::vector<gaia::ListedAccount>& accounts,
+        const std::vector<gaia::ListedAccount>& signed_out_accounts,
+        const GoogleServiceAuthError& error) {}
+
+    // Called when the Gaia cookie has been deleted explicitly by a user action,
+    // e.g. from the settings or by an extension.
+    virtual void OnGaiaCookieDeletedByUserAction() {}
+
+   protected:
+    virtual ~Observer() {}
+  };
+
   // Class to retrieve the external connection check results from gaia.
   // Declared publicly for unit tests.
   class ExternalCcResultFetcher : public GaiaAuthConsumer {
@@ -258,6 +274,10 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
   // service. Virtual for testing.
   virtual void ForceOnCookieChangeProcessing();
 
+  // Add or remove observers of this helper.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
   // Cancel all login requests.
   void CancelAll();
 
@@ -279,25 +299,6 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
     list_accounts_stale_ = stale;
   }
 
-  // If set, this callback will be invoked whenever the
-  // GaiaCookieManagerService's list of GAIA accounts is updated. The GCMS
-  // monitors the APISID cookie and triggers a /ListAccounts call on change.
-  // The GCMS will also call ListAccounts upon the first call to
-  // ListAccounts(). The GCMS will delay calling ListAccounts if other
-  // requests are in queue that would modify the APISID cookie.
-  // If the ListAccounts call fails and the GCMS cannot recover, the reason
-  // is passed in |error|.
-  // This method can only be called once.
-  void SetGaiaAccountsInCookieUpdatedCallback(
-      GaiaAccountsInCookieUpdatedCallback callback);
-
-  // If set, this callback will be invoked whenever the Gaia cookie has
-  // been deleted explicitly by a user action, e.g. from the settings or by an
-  // extension.
-  // This method can only be called once.
-  void SetGaiaCookieDeletedByUserActionCallback(
-      GaiaCookieDeletedByUserActionCallback callback);
-
   // Returns a non-NULL pointer to its instance of net::BackoffEntry
   const net::BackoffEntry* GetBackoffEntry() { return &fetcher_backoff_; }
 
@@ -364,11 +365,6 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
   OAuth2TokenService* token_service_;
   SigninClient* signin_client_;
 
-  GaiaAccountsInCookieUpdatedCallback gaia_accounts_updated_in_cookie_callback_;
-  GaiaCookieDeletedByUserActionCallback
-      gaia_cookie_deleted_by_user_action_callback_;
-  base::RepeatingCallback<scoped_refptr<network::SharedURLLoaderFactory>()>
-      shared_url_loader_factory_getter_;
   std::unique_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
   std::unique_ptr<signin::UbertokenFetcherImpl> uber_token_fetcher_;
   ExternalCcResultFetcher external_cc_result_fetcher_;
@@ -394,6 +390,10 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
   // executed at a time.
   base::circular_deque<GaiaCookieRequest> requests_;
 
+  // List of observers to notify when merge session completes.
+  // Makes sure list is empty on destruction.
+  base::ObserverList<Observer, true>::Unchecked observer_list_;
+
   // True once the ExternalCCResultFetcher has completed once.
   bool external_cc_result_fetched_;
 

components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

@@ -38,19 +38,21 @@ namespace {
 using MockAddAccountToCookieCompletedCallback = base::MockCallback<
     GaiaCookieManagerService::AddAccountToCookieCompletedCallback>;
 
-class MockObserver {
+class MockObserver : public GaiaCookieManagerService::Observer {
  public:
-  explicit MockObserver(GaiaCookieManagerService* helper) {
-    helper->SetGaiaAccountsInCookieUpdatedCallback(base::BindRepeating(
-        &MockObserver::OnGaiaAccountsInCookieUpdated, base::Unretained(this)));
+  explicit MockObserver(GaiaCookieManagerService* helper) : helper_(helper) {
+    helper_->AddObserver(this);
   }
 
+  ~MockObserver() override { helper_->RemoveObserver(this); }
+
   MOCK_METHOD3(OnGaiaAccountsInCookieUpdated,
                void(const std::vector<gaia::ListedAccount>&,
                     const std::vector<gaia::ListedAccount>&,
                     const GoogleServiceAuthError&));
 
  private:
+  GaiaCookieManagerService* helper_;
 
   DISALLOW_COPY_AND_ASSIGN(MockObserver);
 };

components/signin/core/browser/identity_manager_wrapper.cc

@@ -5,7 +5,6 @@
 #include "components/signin/core/browser/identity_manager_wrapper.h"
 
 #include "components/keyed_service/core/keyed_service.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "services/identity/public/cpp/accounts_cookie_mutator.h"
 #include "services/identity/public/cpp/accounts_mutator.h"
 #include "services/identity/public/cpp/diagnostics_provider.h"

components/sync/driver/sync_session_durations_metrics_recorder.h

@@ -20,7 +20,8 @@ namespace syncer {
 // Tracks the active browsing time that the user spends signed in and/or syncing
 // as fraction of their total browsing time.
 class SyncSessionDurationsMetricsRecorder
-    : public syncer::SyncServiceObserver,
+    : public GaiaCookieManagerService::Observer,
+      public syncer::SyncServiceObserver,
       public identity::IdentityManager::Observer {
  public:
   // Callers must ensure that the parameters outlive this object.

services/identity/public/cpp/identity_manager.cc

@@ -8,7 +8,6 @@
 
 #include "build/build_config.h"
 #include "components/signin/core/browser/account_fetcher_service.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "components/signin/core/browser/ubertoken_fetcher_impl.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "services/identity/public/cpp/accounts_cookie_mutator.h"
@@ -66,15 +65,7 @@ IdentityManager::IdentityManager(
   token_service_->AddDiagnosticsObserver(this);
   token_service_->AddObserver(this);
   account_tracker_service_->AddObserver(this);
-
-  // IdentityManager owns gaia_cookie_manager_service_ and will outlive it, so
-  // base::Unretained is safe.
-  gaia_cookie_manager_service_->SetGaiaAccountsInCookieUpdatedCallback(
-      base::BindRepeating(&IdentityManager::OnGaiaAccountsInCookieUpdated,
-                          base::Unretained(this)));
-  gaia_cookie_manager_service_->SetGaiaCookieDeletedByUserActionCallback(
-      base::BindRepeating(&IdentityManager::OnGaiaCookieDeletedByUserAction,
-                          base::Unretained(this)));
+  gaia_cookie_manager_service_->AddObserver(this);
 
   // Seed the primary account with any state that |signin_manager_| loaded from
   // prefs.
@@ -95,6 +86,7 @@ IdentityManager::~IdentityManager() {
   token_service_->RemoveObserver(this);
   token_service_->RemoveDiagnosticsObserver(this);
   account_tracker_service_->RemoveObserver(this);
+  gaia_cookie_manager_service_->RemoveObserver(this);
 }
 
 // TODO(862619) change return type to base::Optional<CoreAccountInfo>

services/identity/public/cpp/identity_manager.h

@@ -13,6 +13,7 @@
 #include "components/signin/core/browser/account_fetcher_service.h"
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/account_tracker_service.h"
+#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/signin/core/browser/signin_metrics.h"
@@ -35,8 +36,6 @@ class TestURLLoaderFactory;
 class PrefRegistrySimple;
 class SigninManagerAndroid;
 
-class GaiaCookieManagerService;
-
 namespace identity {
 
 class AccountsMutator;
@@ -53,6 +52,7 @@ struct CookieParams;
 class IdentityManager : public SigninManagerBase::Observer,
                         public OAuth2TokenService::DiagnosticsObserver,
                         public OAuth2TokenService::Observer,
+                        public GaiaCookieManagerService::Observer,
                         public AccountTrackerService::Observer {
  public:
   class Observer {
@@ -596,12 +596,12 @@ class IdentityManager : public SigninManagerBase::Observer,
   void OnAuthErrorChanged(const std::string& account_id,
                           const GoogleServiceAuthError& auth_error) override;
 
-  // GaiaCookieManagerService callbacks:
+  // GaiaCookieManagerService::Observer:
   void OnGaiaAccountsInCookieUpdated(
       const std::vector<gaia::ListedAccount>& signed_in_accounts,
       const std::vector<gaia::ListedAccount>& signed_out_accounts,
-      const GoogleServiceAuthError& error);
-  void OnGaiaCookieDeletedByUserAction();
+      const GoogleServiceAuthError& error) override;
+  void OnGaiaCookieDeletedByUserAction() override;
 
   // OAuth2TokenService::DiagnosticsObserver:
   void OnAccessTokenRequested(

services/identity/public/cpp/identity_test_utils.cc

@@ -10,7 +10,6 @@
 #include "base/run_loop.h"
 #include "base/strings/string_split.h"
 #include "components/signin/core/browser/account_tracker_service.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "components/signin/core/browser/list_accounts_test_utils.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "google_apis/gaia/gaia_auth_util.h"