Reland "Delete GaiaCookieManagerServiceObserver in favor of callbacks."

This reverts commit 8b51a44e.

Reason for revert: the original CL broke downstream, but the fix landed before this revert landed. Thus the revert is *causing* the build failure. Reverting it.

Original change's description:
> 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/+/1642232
> Reviewed-by: Lowell Manners <lowell@chromium.org>
> Commit-Queue: Lowell Manners <lowell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#666007}

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

Change-Id: I02604da8703d681033ea7a0d53252eb8b264065d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 939372
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1645357Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666207}

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

@@ -7,6 +7,7 @@
 #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,14 +654,6 @@ 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();
@@ -695,8 +687,8 @@ void GaiaCookieManagerService::OnCookieChange(
 
   if (cause == network::mojom::CookieChangeCause::EXPLICIT) {
     DCHECK(net::CookieChangeCauseIsDeletion(net::CookieChangeCause::EXPLICIT));
-    for (auto& observer : observer_list_) {
-      observer.OnGaiaCookieDeletedByUserAction();
+    if (gaia_cookie_deleted_by_user_action_callback_) {
+      gaia_cookie_deleted_by_user_action_callback_.Run();
     }
   }
 
@@ -744,6 +736,18 @@ 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) {
@@ -852,8 +856,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.
-  for (auto& observer : observer_list_) {
-    observer.OnGaiaAccountsInCookieUpdated(
+  if (gaia_accounts_updated_in_cookie_callback_) {
+    gaia_accounts_updated_in_cookie_callback_.Run(
         listed_accounts_, signed_out_accounts_,
         GoogleServiceAuthError(GoogleServiceAuthError::NONE));
   }
@@ -881,10 +885,12 @@ void GaiaCookieManagerService::OnListAccountsFailure(
   }
 
   RecordListAccountsFailure(error.state());
-  for (auto& observer : observer_list_) {
-    observer.OnGaiaAccountsInCookieUpdated(listed_accounts_,
-                                           signed_out_accounts_, error);
+
+  if (gaia_accounts_updated_in_cookie_callback_) {
+    gaia_accounts_updated_in_cookie_callback_.Run(listed_accounts_,
+                                                  signed_out_accounts_, error);
   }
+
   HandleNextRequest();
 }
 

components/signin/core/browser/gaia_cookie_manager_service.h

@@ -88,6 +88,12 @@ 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:
@@ -148,28 +154,6 @@ 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 {
@@ -274,10 +258,6 @@ 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();
 
@@ -299,6 +279,25 @@ 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_; }
 
@@ -365,6 +364,11 @@ 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_;
@@ -390,10 +394,6 @@ 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,21 +38,19 @@ namespace {
 using MockAddAccountToCookieCompletedCallback = base::MockCallback<
     GaiaCookieManagerService::AddAccountToCookieCompletedCallback>;
 
-class MockObserver : public GaiaCookieManagerService::Observer {
+class MockObserver {
  public:
-  explicit MockObserver(GaiaCookieManagerService* helper) : helper_(helper) {
-    helper_->AddObserver(this);
+  explicit MockObserver(GaiaCookieManagerService* helper) {
+    helper->SetGaiaAccountsInCookieUpdatedCallback(base::BindRepeating(
+        &MockObserver::OnGaiaAccountsInCookieUpdated, base::Unretained(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,6 +5,7 @@
 #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,8 +20,7 @@ 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 GaiaCookieManagerService::Observer,
-      public syncer::SyncServiceObserver,
+    : 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,6 +8,7 @@
 
 #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"
@@ -65,7 +66,15 @@ IdentityManager::IdentityManager(
   token_service_->AddDiagnosticsObserver(this);
   token_service_->AddObserver(this);
   account_tracker_service_->AddObserver(this);
-  gaia_cookie_manager_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)));
 
   // Seed the primary account with any state that |signin_manager_| loaded from
   // prefs.
@@ -86,7 +95,6 @@ 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,7 +13,6 @@
 #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"
@@ -36,6 +35,8 @@ class TestURLLoaderFactory;
 class PrefRegistrySimple;
 class SigninManagerAndroid;
 
+class GaiaCookieManagerService;
+
 namespace identity {
 
 class AccountsMutator;
@@ -52,7 +53,6 @@ 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::Observer:
+  // GaiaCookieManagerService callbacks:
   void OnGaiaAccountsInCookieUpdated(
       const std::vector<gaia::ListedAccount>& signed_in_accounts,
       const std::vector<gaia::ListedAccount>& signed_out_accounts,
-      const GoogleServiceAuthError& error) override;
-  void OnGaiaCookieDeletedByUserAction() override;
+      const GoogleServiceAuthError& error);
+  void OnGaiaCookieDeletedByUserAction();
 
   // OAuth2TokenService::DiagnosticsObserver:
   void OnAccessTokenRequested(

services/identity/public/cpp/identity_test_utils.cc

@@ -10,6 +10,7 @@
 #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"