RemoteSuggestionsFetcher: Use common AccessTokenFetcher helper class

components/ntp_snippets/remote/remote_suggestions_fetcher.cc

 // Copyright 2016 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.
 
 #include "components/ntp_snippets/remote/remote_suggestions_fetcher.h"
 
 #include <cstdlib>
 #include <utility>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/features.h"
 #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"
 #include "components/variations/variations_associated_data.h"
 #include "net/url_request/url_fetcher.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using net::URLFetcher;
 using net::URLRequestContextGetter;
 using net::HttpRequestHeaders;
@@ skipping 199 matching lines @@
 RemoteSuggestionsFetcher::RemoteSuggestionsFetcher(
     SigninManagerBase* signin_manager,
     OAuth2TokenService* token_service,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
     PrefService* pref_service,
     LanguageModel* language_model,
     const ParseJSONCallback& parse_json_callback,
     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),
       parse_json_callback_(parse_json_callback),
       fetch_url_(api_endpoint),
       api_key_(api_key),
       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,
     SnippetsAvailableCallback callback) {
   if (!params.interactive_request) {
     UMA_HISTOGRAM_SPARSE_SLOWLY(
         "NewTabPage.Snippets.FetchTimeLocal",
         GetMinuteOfTheDay(/*local_time=*/true,
                           /*reduced_resolution=*/true, clock_.get()));
     UMA_HISTOGRAM_SPARSE_SLOWLY(
         "NewTabPage.Snippets.FetchTimeUTC",
         GetMinuteOfTheDay(/*local_time=*/false,
                           /*reduced_resolution=*/true, clock_.get()));
   }
 
   JsonRequest::Builder builder;
   builder.SetLanguageModel(language_model_)
       .SetParams(params)
       .SetParseJsonCallback(parse_json_callback_)
       .SetClock(clock_.get())
       .SetUrlRequestContextGetter(url_request_context_getter_)
       .SetUserClassifier(*user_classifier_);
 
   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));
   }
 }
 
 void RemoteSuggestionsFetcher::FetchSnippetsNonAuthenticated(
     JsonRequest::Builder builder,
     SnippetsAvailableCallback callback) {
   if (api_key_.empty()) {
     // If we don't have an API key, don't even try.
     FetchFinished(OptionalFetchedCategories(), std::move(callback),
                   FetchResult::MISSING_API_KEY, std::string());
     return;
   }
   // When not providing OAuth token, we need to pass the Google API key.
   builder.SetUrl(
       GURL(base::StringPrintf(kSnippetsServerNonAuthorizedFormat,
                               fetch_url_.spec().c_str(), api_key_.c_str())));
   StartRequest(std::move(builder), std::move(callback));
 }
 
 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));
 }
 
 void RemoteSuggestionsFetcher::StartRequest(
     JsonRequest::Builder builder,
     SnippetsAvailableCallback callback) {
   std::unique_ptr<JsonRequest> request = builder.Build();
   JsonRequest* raw_request = request.get();
   raw_request->Start(base::BindOnce(&RemoteSuggestionsFetcher::JsonRequestDone,
                                     base::Unretained(this), std::move(request),
                                     std::move(callback)));
 }
 
 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>
         builder_and_callback = std::move(pending_requests_.front());
     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());
 
     FetchFinished(OptionalFetchedCategories(),
                   std::move(builder_and_callback.second),
                   FetchResult::OAUTH_TOKEN_ERROR,
                   /*error_details=*/base::StringPrintf(
                       " (%s)", error.ToString().c_str()));
     pending_requests_.pop();
   }
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// 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,
     std::unique_ptr<base::Value> result,
     FetchResult status_code,
     const std::string& error_details) {
   DCHECK(request);
   // Record the time when request for fetching remote content snippets finished.
   const base::Time fetch_time = clock_->Now();
 
@@ skipping 93 matching lines @@
           category, BuildRemoteCategoryInfo(base::UTF8ToUTF16(utf8_title),
                                             allow_fetching_more_results)));
     }
     categories->back().suggestions = std::move(suggestions);
   }
 
   return true;
 }
 
 }  // namespace ntp_snippets