PrimaryAccountAccessTokenFetcher: Simplify observing of signin classes

PrimaryAccountAccessTokenFetcher currently has a very targeted
observance of SigninManager and ProfileOAuth2TokenService:
- It observes SigninManager during the duration of time from which it
  determines it needs to see a signin to when that signin occurs.
- It observes PO2TS during the duration of time from which it
  determines that the user is signed in without a refresh token to when
  the refresh token becomes available.

This targeted observance allows for having nice DCHECKs around exactly
what state of waiting the token fetcher is in when given observer
callbacks occur. However, we will shortly be porting this class to
talk to IdentityManager. After the porting, it will no longer be
possible to maintain this targeted observance, as by design
IdentityManager::Observer folds the functionality of
SigninManagerBase::Observer and OAuth2TokenService::Observer together.
Thus, for example, if the token fetcher is obstensibly waiting for a
refresh token to become available, it might in fact see the user sign
in again (if they managed to sign out and sign back in in the interim,
for example).

This CL changes PrimaryAccountAccessTokenFetcher in anticipation of
that porting:
- It starts observing SigninManager and PO2TS immediately when
  launching in WaitUntilAvailable mode via ScopedObserver instances if
  credentials are not immediately available.
- When it starts an access token request, it removes these observers
  if they are present (in addition to not being present if launched
  in RequestImmediately mode, access token requests get retried once by
  the fetcher in WaitUntilAvailable mode; thus, it's not guaranteed that
  these observers will be present at the time of starting an access
  token request).
- Otherwise, the observers are removed implicitly on destruction.

A side effect of these changes is that the nice DCHECKs mentioned
above must necessarily be removed. In fact, there could be behavioral
changes; e.g., PrimaryAccountAccessTokenFetcher could observe a signin
event where it hadn't before in the case mentioned above. However,
these behavioral changes are harmless, as
PrimaryAccountAccessTokenFetcher's observer callback implementations
are essentially stateless: they simply wait for the moment when both
the primary account info and the refresh token for that account are
available, and then synchronously fire off an access token request and
stop listening for further signin events. Observing more signin-related
events will simply result in more checks for whether that moment has
arrived.

Bug: 796544
Change-Id: I58738bb697e38008012f7b8bc759ed8c8382c516
Reviewed-on: https://chromium-review.googlesource.com/1092575
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566023}

services/identity/public/cpp/primary_account_access_token_fetcher.cc

@@ -22,61 +22,45 @@ PrimaryAccountAccessTokenFetcher::PrimaryAccountAccessTokenFetcher(
       token_service_(token_service),
       scopes_(scopes),
       callback_(std::move(callback)),
-      waiting_for_sign_in_(false),
-      waiting_for_refresh_token_(false),
+      signin_manager_observer_(this),
+      token_service_observer_(this),
       access_token_retried_(false),
       mode_(mode) {
   Start();
 }
 
 PrimaryAccountAccessTokenFetcher::~PrimaryAccountAccessTokenFetcher() {
-  if (waiting_for_sign_in_) {
-    signin_manager_->RemoveObserver(this);
-  }
-  if (waiting_for_refresh_token_) {
-    token_service_->RemoveObserver(this);
-  }
 }
 
 void PrimaryAccountAccessTokenFetcher::Start() {
-  if (mode_ == Mode::kImmediate) {
+  if (mode_ == Mode::kImmediate || AreCredentialsAvailable()) {
     StartAccessTokenRequest();
     return;
   }
 
-  if (signin_manager_->IsAuthenticated()) {
-    // Already signed in: Make sure we have a refresh token, then get the access
-    // token.
-    WaitForRefreshToken();
-    return;
-  }
-
-  // Not signed in: Wait for a sign-in to complete (to get the refresh token),
-  // then get the access token.
-  DCHECK(!waiting_for_sign_in_);
-  waiting_for_sign_in_ = true;
-  signin_manager_->AddObserver(this);
+  // Start observing the SigninManager and Token Service. These observers will
+  // be removed either when credentials are obtained and an access token request
+  // is started or when this object is destroyed.
+  signin_manager_observer_.Add(signin_manager_);
+  token_service_observer_.Add(token_service_);
 }
 
-void PrimaryAccountAccessTokenFetcher::WaitForRefreshToken() {
+bool PrimaryAccountAccessTokenFetcher::AreCredentialsAvailable() const {
   DCHECK_EQ(Mode::kWaitUntilAvailable, mode_);
-  DCHECK(signin_manager_->IsAuthenticated());
-  DCHECK(!waiting_for_refresh_token_);
-
-  if (token_service_->RefreshTokenIsAvailable(
-          signin_manager_->GetAuthenticatedAccountId())) {
-    // Already have refresh token: Get the access token directly.
-    StartAccessTokenRequest();
-    return;
-  }
 
-  // Signed in, but refresh token isn't there yet: Wait for the refresh
-  // token to be loaded, then get the access token.
-  waiting_for_refresh_token_ = true;
-  token_service_->AddObserver(this);
+  return (signin_manager_->IsAuthenticated() &&
+          token_service_->RefreshTokenIsAvailable(
+              signin_manager_->GetAuthenticatedAccountId()));
 }
 
 void PrimaryAccountAccessTokenFetcher::StartAccessTokenRequest() {
+  DCHECK(mode_ == Mode::kImmediate || AreCredentialsAvailable());
+
+  // By the time of starting an access token request, we should no longer be
+  // listening for signin-related events.
+  DCHECK(!signin_manager_observer_.IsObserving(signin_manager_));
+  DCHECK(!token_service_observer_.IsObserving(token_service_));
+
   // Note: We might get here even in cases where we know that there's no refresh
   // token. We're requesting an access token anyway, so that the token service
   // will generate an appropriate error code that we can return to the client.
@@ -92,29 +76,23 @@ void PrimaryAccountAccessTokenFetcher::StartAccessTokenRequest() {
 void PrimaryAccountAccessTokenFetcher::GoogleSigninSucceeded(
     const std::string& account_id,
     const std::string& username) {
-  DCHECK_EQ(Mode::kWaitUntilAvailable, mode_);
-  DCHECK(waiting_for_sign_in_);
-  DCHECK(!waiting_for_refresh_token_);
-  DCHECK(signin_manager_->IsAuthenticated());
-  waiting_for_sign_in_ = false;
-  signin_manager_->RemoveObserver(this);
-
-  WaitForRefreshToken();
+  ProcessSigninStateChange();
 }
 
 void PrimaryAccountAccessTokenFetcher::OnRefreshTokenAvailable(
     const std::string& account_id) {
+  ProcessSigninStateChange();
+}
+
+void PrimaryAccountAccessTokenFetcher::ProcessSigninStateChange() {
   DCHECK_EQ(Mode::kWaitUntilAvailable, mode_);
-  DCHECK(waiting_for_refresh_token_);
-  DCHECK(!waiting_for_sign_in_);
 
-  // Only react on tokens for the account the user has signed in with.
-  if (account_id != signin_manager_->GetAuthenticatedAccountId()) {
+  if (!AreCredentialsAvailable())
     return;
-  }
 
-  waiting_for_refresh_token_ = false;
-  token_service_->RemoveObserver(this);
+  signin_manager_observer_.Remove(signin_manager_);
+  token_service_observer_.Remove(token_service_);
+
   StartAccessTokenRequest();
 }
 
@@ -150,8 +128,7 @@ void PrimaryAccountAccessTokenFetcher::OnGetTokenFailure(
   // don't have to.
   if (mode_ == Mode::kWaitUntilAvailable && !access_token_retried_ &&
       error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED &&
-      token_service_->RefreshTokenIsAvailable(
-          signin_manager_->GetAuthenticatedAccountId())) {
+      AreCredentialsAvailable()) {
     access_token_retried_ = true;
     StartAccessTokenRequest();
     return;

services/identity/public/cpp/primary_account_access_token_fetcher.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "google_apis/gaia/oauth2_token_service.h"
@@ -58,7 +59,10 @@ class PrimaryAccountAccessTokenFetcher : public SigninManagerBase::Observer,
  private:
   void Start();
 
-  void WaitForRefreshToken();
+  // Returns true iff there is a primary account with a refresh token. Should
+  // only be called in mode |kWaitUntilAvailable|.
+  bool AreCredentialsAvailable() const;
+
   void StartAccessTokenRequest();
 
   // SigninManagerBase::Observer implementation.
@@ -68,6 +72,10 @@ class PrimaryAccountAccessTokenFetcher : public SigninManagerBase::Observer,
   // OAuth2TokenService::Observer implementation.
   void OnRefreshTokenAvailable(const std::string& account_id) override;
 
+  // Checks whether credentials are now available and starts an access token
+  // request if so. Should only be called in mode |kWaitUntilAvailable|.
+  void ProcessSigninStateChange();
+
   // OAuth2TokenService::Consumer implementation.
   void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
                          const std::string& access_token,
@@ -80,8 +88,10 @@ class PrimaryAccountAccessTokenFetcher : public SigninManagerBase::Observer,
   OAuth2TokenService::ScopeSet scopes_;
   TokenCallback callback_;
 
-  bool waiting_for_sign_in_;
-  bool waiting_for_refresh_token_;
+  ScopedObserver<SigninManagerBase, PrimaryAccountAccessTokenFetcher>
+      signin_manager_observer_;
+  ScopedObserver<OAuth2TokenService, PrimaryAccountAccessTokenFetcher>
+      token_service_observer_;
 
   std::unique_ptr<OAuth2TokenService::Request> access_token_request_;