RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class

components/ntp_snippets/remote/remote_suggestions_fetcher.cc

Index: components/ntp_snippets/remote/remote_suggestions_fetcher.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_fetcher.cc b/components/ntp_snippets/remote/remote_suggestions_fetcher.cc
index c7218ee365faa36cf78c37613afe2a491d610b92..47baec556e42e5593ca03c88ffaf3743b0906d36 100644
--- a/components/ntp_snippets/remote/remote_suggestions_fetcher.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_fetcher.cc
@@ -24,6 +24,7 @@
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/remote/request_params.h"
 #include "components/ntp_snippets/user_classifier.h"
+#include "components/signin/core/browser/access_token_fetcher.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/strings/grit/components_strings.h"
@@ -243,8 +244,7 @@ RemoteSuggestionsFetcher::RemoteSuggestionsFetcher(
     const GURL& api_endpoint,
     const std::string& api_key,
     const UserClassifier* user_classifier)
-    : OAuth2TokenService::Consumer("ntp_snippets"),
-      signin_manager_(signin_manager),
+    : signin_manager_(signin_manager),
       token_service_(token_service),
       url_request_context_getter_(std::move(url_request_context_getter)),
       language_model_(language_model),
@@ -254,11 +254,7 @@ RemoteSuggestionsFetcher::RemoteSuggestionsFetcher(
       clock_(new base::DefaultClock()),
       user_classifier_(user_classifier) {}
 
-RemoteSuggestionsFetcher::~RemoteSuggestionsFetcher() {
-  if (waiting_for_refresh_token_) {
-    token_service_->RemoveObserver(this);
-  }
-}
+RemoteSuggestionsFetcher::~RemoteSuggestionsFetcher() = default;
 
 void RemoteSuggestionsFetcher::FetchSnippets(
     const RequestParams& params,
@@ -284,18 +280,8 @@ void RemoteSuggestionsFetcher::FetchSnippets(
 
   if (signin_manager_->IsAuthenticated()) {
     // Signed-in: get OAuth token --> fetch suggestions.
-    oauth_token_retried_ = false;
     pending_requests_.emplace(std::move(builder), std::move(callback));
     StartTokenRequest();
-  } else if (signin_manager_->AuthInProgress()) {

[Marc Treib, 2017/03/23 19:23:19]

Do you remember if we had actual proof that this case ever happened? Per the
discussion/investigation on https://codereview.chromium.org/2582573002, it
should never return true on Android (and even if it did, we should maybe treat
it as not-signed-in). Please double-check :)

If we do want to handle it, we could just put an "|| AuthInProgress" into the if
above; the AccessTokenFetcher would then wait for the sign-in to complete.

[jkrcal, 2017/03/24 09:09:26]

The question is: how does it behave on iOS? 
Unless we know it also does not happen on iOS, I'd rather keep add the "|| .."
clause to the previous if and keep the TODO in the unit-test :|

[Marc Treib, 2017/03/27 08:08:20]

Alright, makes sense, at least until we've investigated further. I've added the
AuthInProgress check again, and extended the test comment a bit.

-    // Currently signing in: wait for auth to finish (the refresh token) -->
-    //     get OAuth token --> fetch suggestions.
-    pending_requests_.emplace(std::move(builder), std::move(callback));
-    if (!waiting_for_refresh_token_) {
-      // Wait until we get a refresh token.
-      waiting_for_refresh_token_ = true;
-      token_service_->AddObserver(this);
-    }
   } else {
     // Not signed in: fetch suggestions (without authentication).
     FetchSnippetsNonAuthenticated(std::move(builder), std::move(callback));
@@ -321,11 +307,10 @@ void RemoteSuggestionsFetcher::FetchSnippetsNonAuthenticated(
 void RemoteSuggestionsFetcher::FetchSnippetsAuthenticated(
     JsonRequest::Builder builder,
     SnippetsAvailableCallback callback,
-    const std::string& account_id,
     const std::string& oauth_access_token) {
   // TODO(jkrcal, treib): Add unit-tests for authenticated fetches.
   builder.SetUrl(fetch_url_)
-      .SetAuthentication(account_id,
+      .SetAuthentication(signin_manager_->GetAuthenticatedAccountId(),
                          base::StringPrintf(kAuthorizationRequestHeaderFormat,
                                             oauth_access_token.c_str()));
   StartRequest(std::move(builder), std::move(callback));
@@ -342,23 +327,32 @@ void RemoteSuggestionsFetcher::StartRequest(
 }
 
 void RemoteSuggestionsFetcher::StartTokenRequest() {
-  OAuth2TokenService::ScopeSet scopes;
-  scopes.insert(kContentSuggestionsApiScope);
-  oauth_request_ = token_service_->StartRequest(
-      signin_manager_->GetAuthenticatedAccountId(), scopes, this);
+  // If there is already an ongoing token request, just wait for that.
+  if (token_fetcher_) {
+    return;
+  }
+
+  OAuth2TokenService::ScopeSet scopes{kContentSuggestionsApiScope};
+  token_fetcher_ = base::MakeUnique<AccessTokenFetcher>(
+      "ntp_snippets", signin_manager_, token_service_, scopes,
+      base::BindOnce(&RemoteSuggestionsFetcher::AccessTokenAvailable,
+                     base::Unretained(this)));
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// OAuth2TokenService::Consumer overrides
-void RemoteSuggestionsFetcher::OnGetTokenSuccess(
-    const OAuth2TokenService::Request* request,
-    const std::string& access_token,
-    const base::Time& expiration_time) {
-  // Delete the request after we leave this method.
-  std::unique_ptr<OAuth2TokenService::Request> oauth_request(
-      std::move(oauth_request_));
-  DCHECK_EQ(oauth_request.get(), request)
-      << "Got tokens from some previous request";
+void RemoteSuggestionsFetcher::AccessTokenAvailable(

[jkrcal, 2017/03/24 09:09:26]

The naming is misleading for me as this function also includes the case of an
error. Maybe AccessTokenFetchFinished?

[Marc Treib, 2017/03/27 08:08:20]

Done.

+    const GoogleServiceAuthError& error,
+    const std::string& access_token) {
+  // Delete the fetcher after we leave this method.

[jkrcal, 2017/03/24 09:09:26]

nit: I do not see the token_fetcher_ being used later in this method. Why not
token_fetcher_.reset(nullptr)? I do not think this would need the comment.

[Marc Treib, 2017/03/27 08:08:20]

This method is called from the fetcher, so deleting it earlier is dangerous.
E.g. our method parameters (passed by reference) might stop to exist.

[jkrcal, 2017/03/27 08:35:59]

Understood!

nit: Can you expand the comment a bit? e.g.
// Delete the fetcher only after we leave this method (which is called from the
fetcher itself).

[Marc Treib, 2017/03/27 08:37:59]

Sure, done!

+  DCHECK(token_fetcher_);
+  std::unique_ptr<AccessTokenFetcher> token_fetcher_deleter(
+      std::move(token_fetcher_));
+
+  if (error.state() != GoogleServiceAuthError::NONE) {
+    AccessTokenError(error);
+    return;
+  }
+
+  DCHECK(!access_token.empty());
 
   while (!pending_requests_.empty()) {
     std::pair<JsonRequest::Builder, SnippetsAvailableCallback>
@@ -366,25 +360,16 @@ void RemoteSuggestionsFetcher::OnGetTokenSuccess(
     pending_requests_.pop();
     FetchSnippetsAuthenticated(std::move(builder_and_callback.first),
                                std::move(builder_and_callback.second),
-                               oauth_request->GetAccountId(), access_token);
+                               access_token);
   }
 }
 
-void RemoteSuggestionsFetcher::OnGetTokenFailure(
-    const OAuth2TokenService::Request* request,
+void RemoteSuggestionsFetcher::AccessTokenError(
     const GoogleServiceAuthError& error) {
-  oauth_request_.reset();
-
-  if (!oauth_token_retried_ &&
-      error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED) {
-    // The request (especially on startup) can get reset by loading the refresh
-    // token - do it one more time.
-    oauth_token_retried_ = true;
-    StartTokenRequest();
-    return;
-  }
+  DCHECK_NE(error.state(), GoogleServiceAuthError::NONE);
 
   DLOG(ERROR) << "Unable to get token: " << error.ToString();
+
   while (!pending_requests_.empty()) {
     std::pair<JsonRequest::Builder, SnippetsAvailableCallback>
         builder_and_callback = std::move(pending_requests_.front());
@@ -398,21 +383,6 @@ void RemoteSuggestionsFetcher::OnGetTokenFailure(
   }
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// OAuth2TokenService::Observer overrides
-void RemoteSuggestionsFetcher::OnRefreshTokenAvailable(
-    const std::string& account_id) {
-  // Only react on tokens for the account the user has signed in with.
-  if (account_id != signin_manager_->GetAuthenticatedAccountId()) {
-    return;
-  }
-
-  token_service_->RemoveObserver(this);
-  waiting_for_refresh_token_ = false;
-  oauth_token_retried_ = false;
-  StartTokenRequest();
-}
-
 void RemoteSuggestionsFetcher::JsonRequestDone(
     std::unique_ptr<JsonRequest> request,
     SnippetsAvailableCallback callback,