Allow fetching personalized snippets from ChromeReader.

components/ntp_snippets/ntp_snippets_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/ntp_snippets_fetcher.h"
 
+#include <stdlib.h>
+
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "components/ntp_snippets/ntp_snippets_constants.h"
+#include "components/signin/core/browser/profile_oauth2_token_service.h"
+#include "components/signin/core/browser/signin_manager.h"
+#include "components/signin/core/browser/signin_tracker.h"
+#include "components/variations/variations_associated_data.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/load_flags.h"
-#include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
+#include "third_party/icu/source/common/unicode/uloc.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
 
 using net::URLFetcher;
 using net::URLRequestContextGetter;
 using net::HttpRequestHeaders;
 using net::URLRequestStatus;
 
 namespace ntp_snippets {
 
 namespace {
 
+const char kApiScope[] = "https://www.googleapis.com/auth/webhistory";
+
 const char kStatusMessageURLRequestErrorFormat[] = "URLRequestStatus error %d";
 const char kStatusMessageHTTPErrorFormat[] = "HTTP error %d";
 
-const char kContentSnippetsServerFormat[] =
-    "https://chromereader-pa.googleapis.com/v1/fetch?key=%s";
+const char kContentSnippetsServer[] =
+    "https://chromereader-pa.googleapis.com/v1/fetch";
+const char kContentSnippetsServerNonAuthorizedParamFormat[] = "?key=%s";

[Marc Treib, 2016/05/04 11:50:09]

Put in the server URL also via StringPrintf, instead of string concatenation?

[jkrcal, 2016/05/04 14:04:08]

Done.

+const char kAuthorizationRequestHeaderFormat[] = "Bearer %s";
 
-const char kRequestParameterFormat[] =
+// Variation parameter for the variant of fetching to use.
+const char kVariantName[] = "variant";

[Marc Treib, 2016/05/04 11:50:10]

IMO this is overly generic. At least make it "fetching_variant"?

[jkrcal, 2016/05/04 14:04:08]

Done.

+
+// Constants listing possible values of the "variant" parameter.
+const char kVariantRestricted[] = "restricted";
+const char kVariantPersonalized[] = "personalized";
+const char kVariantRestrictedPersonalized[] = "restricted_personalized";
+
+// At the moment METADATA=ALL equals to requiring TITLE, SNIPPET, THUMBNAIL
+// which is exactly what we require.

[Marc Treib, 2016/05/04 11:50:09]

We listed title, snippet, thumbnail separately on purpose, so things don't break
if more metadata fields are added in the future.

[jkrcal, 2016/05/04 14:04:08]

Done.

+const char kRequestParameterFormatNonAuth[] =
     "{"
     "  \"response_detail_level\": \"STANDARD\","
     "  \"advanced_options\": {"
     "    \"local_scoring_params\": {"
     "      \"content_params\": {"
     "        \"only_return_personalized_results\": false"
     "      },"
     "      \"content_restricts\": {"
     "        \"type\": \"METADATA\","
-    "        \"value\": \"TITLE\""
-    "      },"
-    "      \"content_restricts\": {"
-    "        \"type\": \"METADATA\","
-    "        \"value\": \"SNIPPET\""
-    "      },"
-    "      \"content_restricts\": {"
-    "        \"type\": \"METADATA\","
-    "        \"value\": \"THUMBNAIL\""
+    "        \"value\": \"ALL\""
     "      }"
     "%s"
     "    },"
     "    \"global_scoring_params\": {"
-    "      \"num_to_return\": %i"
+    "      \"num_to_return\": %i,"
+    "      \"sort_type\": 1"
     "    }"
     "  }"
     "}";
+
+const char kRequestParameterFormatAuth[] =

[Marc Treib, 2016/05/04 11:50:09]

This is mostly the same as the NonAuth one. Can we merge them, and put in the
differences via StringPrintf etc?
Or maybe the time has come to build this thing as a json dict (via base::Value)
instead of string manipulation...

[jkrcal, 2016/05/04 14:04:08]

Merged via printf.
I am not sure json dict is worth the effort, here.
 (less readable and less efficient code, imho)

+    "{"
+    "  \"response_detail_level\": \"STANDARD\","
+    "  \"obfuscated_gaia_id\": \"%s\","
+    "  \"advanced_options\": {"
+    "    \"local_scoring_params\": {"
+    "      \"content_params\": {"
+    "        \"only_return_personalized_results\": false,"
+    "        \"user_segment\": \"%s\""
+    "      },"
+    "      \"content_restricts\": {"
+    "        \"type\": \"METADATA\","
+    "        \"value\": \"ALL\""
+    "      }"
+    "%s"
+    "    },"
+    "    \"global_scoring_params\": {"
+    "      \"num_to_return\": %i,"
+    "      \"sort_type\": 1"
+    "    }"
+    "  }"
+    "}";
 
 const char kHostRestrictFormat[] =
     "      ,\"content_selectors\": {"
     "        \"type\": \"HOST_RESTRICT\","
     "        \"value\": \"%s\""
     "      }";
 
 }  // namespace
 
 NTPSnippetsFetcher::NTPSnippetsFetcher(
+    SigninManagerBase* signin_manager,
+    OAuth2TokenService* token_service,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
     bool is_stable_channel)
-    : url_request_context_getter_(url_request_context_getter),
-      is_stable_channel_(is_stable_channel) {}
+    : OAuth2TokenService::Consumer("NTP_snippets"),
+      url_request_context_getter_(url_request_context_getter),
+      signin_manager_(signin_manager),
+      token_service_(token_service),
+      waiting_for_refresh_token_(false),
+      is_stable_channel_(is_stable_channel){

[Bernhard Bauer, 2016/05/04 11:46:58]

Space before opening brace (also in other places in this file).

[jkrcal, 2016/05/04 14:04:08]

Done.

+  // Parse the variation parameters and set the defaults if missing.
+  variant_ = variations::GetVariationParamValue(ntp_snippets::kStudyName,
+                                                kVariantName);
+  if (variant_ == "")

[Bernhard Bauer, 2016/05/04 11:46:58]

.empty()

[Marc Treib, 2016/05/04 11:50:10]

variant_.empty()

Also, what if it's an unknown string?

[jkrcal, 2016/05/04 14:04:08]

Done.

[jkrcal, 2016/05/04 14:04:08]

Done.

+    variant_ = kVariantRestrictedPersonalized;
+}
 
-NTPSnippetsFetcher::~NTPSnippetsFetcher() {}
+NTPSnippetsFetcher::~NTPSnippetsFetcher() {
+  if (waiting_for_refresh_token_)
+    token_service_->RemoveObserver(this);
+}
 
 std::unique_ptr<NTPSnippetsFetcher::SnippetsAvailableCallbackList::Subscription>
 NTPSnippetsFetcher::AddCallback(const SnippetsAvailableCallback& callback) {
   return callback_list_.Add(callback);
 }
 
 void NTPSnippetsFetcher::FetchSnippets(const std::set<std::string>& hosts,
+                                       const std::string& language_code,
                                        int count) {
-  const std::string& key = is_stable_channel_
-                               ? google_apis::GetAPIKey()
-                               : google_apis::GetNonStableAPIKey();
-  std::string url =
-      base::StringPrintf(kContentSnippetsServerFormat, key.c_str());
+  // TODO(treib): What to do if there's already a pending request?

[Bernhard Bauer, 2016/05/04 11:46:58]

This does seem like a thing we should worry about. What is going to happen right
now?

[Marc Treib, 2016/05/04 11:50:09]

Bad merge: This line had been removed (there's now a comment in the header
explaining things).

[jkrcal, 2016/05/04 14:04:08]

Done.

[jkrcal, 2016/05/04 14:04:08]

Bad merge: This line had been removed (there's now a comment in the header
explaining things).

+  hosts_ = hosts;
+
+  // Translate the BCP 47 |language_code| into a posix locale string
+  char locale[20];

[Bernhard Bauer, 2016/05/04 11:46:58]

Why 20?

[Marc Treib, 2016/05/04 11:50:10]

Why 20?

[jkrcal, 2016/05/04 14:04:07]

Done.

[jkrcal, 2016/05/04 14:04:08]

Done.

+  UErrorCode error;
+  uloc_forLanguageTag(language_code.c_str(), locale, 20, NULL, &error);

[Bernhard Bauer, 2016/05/04 11:46:58]

nullptr

[Marc Treib, 2016/05/04 11:50:10]

s/NULL/nullptr/

[jkrcal, 2016/05/04 14:04:07]

Done.

[jkrcal, 2016/05/04 14:04:08]

Done.

+  DCHECK(error == U_ZERO_ERROR) <<

[Bernhard Bauer, 2016/05/04 11:46:58]

Use DCHECK_EQ (with the expected value first) for nicer error messages in case
of failure (for example, it would include the error value automatically).

[Marc Treib, 2016/05/04 11:50:09]

I don't think this warrants a DCHECK. (D)LOG(WARNING)?

[jkrcal, 2016/05/04 14:04:07]

Thanks for the hint. In the end, I used DLOG(WARNING) instead.

[jkrcal, 2016/05/04 14:04:07]

Done.

+      "Error in translating language code to a locale string: " << error;
+  locale_ = std::string(locale);

[Bernhard Bauer, 2016/05/04 11:46:58]

The std::string constructor is implicit (or you could just use assign()).

[jkrcal, 2016/05/04 14:04:07]

Done.

+
+  count_ = count;
+
+  if (UseAuthenticated()){
+    if (signin_manager_->IsAuthenticated()) {
+      // signed-in: get OAuth for the API --> fetch snippets

[Marc Treib, 2016/05/04 11:50:09]

nit: Start with a captial letter and end with a period.
Also, "get OAuth token" (or even "access token"), and I'd remove "for the API" -
the token isn't really tied to the API.

[jkrcal, 2016/05/04 14:04:08]

Done.

+      StartTokenRequest();
+

[Marc Treib, 2016/05/04 11:50:09]

no empty line

[jkrcal, 2016/05/04 14:04:07]

Done.

+    } else if (signin_manager_->AuthInProgress()){
+      // currently signing in: wait for auth to finish (the refresh token) -->
+      //     get OAuth for the API --> fetch snippets
+      if (!waiting_for_refresh_token_) {
+        // Wait until we get a refresh token.
+        waiting_for_refresh_token_ = true;
+        token_service_->AddObserver(this);
+      }
+    }

[Marc Treib, 2016/05/04 11:50:10]

Hm, so the "else" here (not authenticated or auth in progress) can never happen,
because UseAuthenticated checks for that. I find that confusing, it's not clear
from reading this method. Can we just pull the signing_manager checks out of
UseAuthenticated?

[jkrcal, 2016/05/04 14:04:07]

This is the else of the "UseAuthenticated()"-if and it definitely can happen. Do
you agree?

[Marc Treib, 2016/05/04 15:24:24]

Sorry, I wasn't clear. I meant the missing "else" for the
authenticated/auth-in-progress if.

[jkrcal, 2016/05/09 11:58:05]

Done.

+  } else {
+    // not signed in: fetch snippets (without authentication)
+    FetchSnippetsNonAuthenticated();
+  }
+}
+
+void NTPSnippetsFetcher::FetchSnippetsImpl(const std::string& url,

[Bernhard Bauer, 2016/05/04 11:46:58]

I would represent URLs as GURL as much as possible.

[jkrcal, 2016/05/04 14:04:08]

Done.

+                                           HttpRequestHeaders& headers,
+                                           const std::string& request) {
   url_fetcher_ = URLFetcher::Create(GURL(url), URLFetcher::POST, this);
+
   url_fetcher_->SetRequestContext(url_request_context_getter_.get());
   url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                              net::LOAD_DO_NOT_SAVE_COOKIES);
-  HttpRequestHeaders headers;
+
   headers.SetHeader("Content-Type", "application/json; charset=UTF-8");
   url_fetcher_->SetExtraRequestHeaders(headers.ToString());
-  std::string host_restricts;
-  for (const std::string& host : hosts)
-    host_restricts += base::StringPrintf(kHostRestrictFormat, host.c_str());
-  url_fetcher_->SetUploadData("application/json",
-                              base::StringPrintf(kRequestParameterFormat,
-                                                 host_restricts.c_str(),
-                                                 count));
-
+  url_fetcher_->SetUploadData("application/json", request);
   // Fetchers are sometimes cancelled because a network change was detected.
   url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3);
   // Try to make fetching the files bit more robust even with poor connection.
   url_fetcher_->SetMaxRetriesOn5xx(3);
   url_fetcher_->Start();
 }
 
+std::string NTPSnippetsFetcher::GetHostsRestrict() const {

[Marc Treib, 2016/05/04 11:50:09]

GetHostRestricts

[jkrcal, 2016/05/04 14:04:08]

Done.

[Marc Treib, 2016/05/04 15:24:24]

GetHostRestricts - host is singular, restricts is plural :)

[jkrcal, 2016/05/09 11:58:05]

Done. (sorry for the oversight of mine!)

+  std::string host_restricts;
+  if (variant_ == kVariantRestricted ||
+      variant_ == kVariantRestrictedPersonalized) {
+    for (const std::string& host : hosts_)
+      host_restricts += base::StringPrintf(kHostRestrictFormat, host.c_str());
+  }
+  return host_restricts;
+}
+
+bool NTPSnippetsFetcher::UseAuthenticated() {
+  return (variant_ == kVariantPersonalized ||
+      (variant_ == kVariantRestrictedPersonalized && !hosts_.empty())) &&
+      (signin_manager_->IsAuthenticated() || signin_manager_->AuthInProgress());
+}
+
+void NTPSnippetsFetcher::FetchSnippetsNonAuthenticated() {
+  // When not providing OAuth token, we need to pass the Google API key
+  const std::string& key = is_stable_channel_
+                               ? google_apis::GetAPIKey()
+                               : google_apis::GetNonStableAPIKey();
+  std::string url = std::string(kContentSnippetsServer) +
+      base::StringPrintf(kContentSnippetsServerNonAuthorizedParamFormat,

[Bernhard Bauer, 2016/05/04 11:46:58]

You could extend the format to include a parameter for the URL.

[jkrcal, 2016/05/04 14:04:07]

Done.

+                         key.c_str());
+  HttpRequestHeaders headers;

[Bernhard Bauer, 2016/05/04 11:46:58]

You could just inline this.

[jkrcal, 2016/05/04 14:04:08]

Done.

+
+  FetchSnippetsImpl(url, headers,
+                    base::StringPrintf(kRequestParameterFormatNonAuth,
+                                       GetHostsRestrict().c_str(),
+                                       count_));
+}
+
+void NTPSnippetsFetcher::FetchSnippetsAuthenticated(
+    const std::string& account_id,
+    const std::string& oauth_access_token) {
+

[Marc Treib, 2016/05/04 11:50:10]

no empty line

[jkrcal, 2016/05/04 14:04:08]

Done.

+  HttpRequestHeaders headers;
+  headers.SetHeader("Authorization",
+                    base::StringPrintf(kAuthorizationRequestHeaderFormat,
+                                       oauth_access_token.c_str()));
+
+  FetchSnippetsImpl(std::string(kContentSnippetsServer), headers,

[Marc Treib, 2016/05/04 11:50:09]

I don't think you need the explicit conversion to std::string

[jkrcal, 2016/05/04 14:04:08]

Done.

+                    base::StringPrintf(kRequestParameterFormatAuth,
+                                       account_id.c_str(),
+                                       locale_.c_str(),
+                                       GetHostsRestrict().c_str(),
+                                       count_));
+}
+
+void NTPSnippetsFetcher::StartTokenRequest() {
+  OAuth2TokenService::ScopeSet scopes;
+  scopes.insert(kApiScope);
+  oauth_request_ = token_service_->StartRequest(
+      signin_manager_->GetAuthenticatedAccountId(), scopes, this);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Consumer overrides
+void NTPSnippetsFetcher::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  oauth_request_.reset();
+
+  FetchSnippetsAuthenticated(request->GetAccountId(), access_token);
+}
+
+void NTPSnippetsFetcher::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  oauth_request_.reset();
+  DLOG(ERROR) << "Unable to get token: " << error.ToString();

[Marc Treib, 2016/05/04 11:50:10]

Should we fall back to the non-authenticated case here?

[jkrcal, 2016/05/04 14:04:08]

Done.

[Marc Treib, 2016/05/04 15:24:24]

FWIW, this was an actual question - I'm not sure what the right thing to do is
here. If we do the fallback, then we need to make sure to correctly record
metrics (click-through-rate for personalized vs non-personalized and such).

[jkrcal, 2016/05/09 11:58:05]

I am not sure either. I think fallback to non-auth is slightly better.
We do not log any auth/non-auth histograms at the moment. We need to be careful
about it later.

I added a comment to make it even clearer this is a fallback.

+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Observer overrides
+void NTPSnippetsFetcher::OnRefreshTokenAvailable(
+  const std::string& account_id) {
+  token_service_->RemoveObserver(this);
+  waiting_for_refresh_token_ = false;
+  StartTokenRequest();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // URLFetcherDelegate overrides
 void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
   DCHECK_EQ(url_fetcher_.get(), source);
 
   std::string message;
   const URLRequestStatus& status = source->GetStatus();
 
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       "NewTabPage.Snippets.FetchHttpResponseOrErrorCode",
       status.is_success() ? source->GetResponseCode() : status.error());
 
   if (!status.is_success()) {
     message = base::StringPrintf(kStatusMessageURLRequestErrorFormat,
                                  status.error());
   } else if (source->GetResponseCode() != net::HTTP_OK) {

[Bernhard Bauer, 2016/05/04 11:46:58]

Now that we use authentication, we need to deal with the edge case again where
the auth token expires just before we send the request (in which case we need to
fetch a new auth token). But really, we should extract that into a common class
instead of adding it to every. single. class that uses auth tokens. I filed
https://crbug.com/609084 for that.

[jkrcal, 2016/05/04 14:04:08]

I added a TODO to solve this later (in a common class).

     message = base::StringPrintf(kStatusMessageHTTPErrorFormat,
                                  source->GetResponseCode());
   }
 
   std::string response;
   if (!message.empty()) {
+    std::string error_response;
+    source->GetResponseAsString(&error_response);

[Marc Treib, 2016/05/04 11:50:09]

Now GetResponseAsString is called in both cases, so pull it out of the if?

[jkrcal, 2016/05/04 14:04:08]

In the if branch I want to use a different variable and thus keep response
empty. Does it make sense?

[Marc Treib, 2016/05/04 15:24:24]

Ah, I see. Okay, fair enough.

     DLOG(WARNING) << message << " while trying to download "
-                  << source->GetURL().spec();
+                  << source->GetURL().spec() << ":"

[Bernhard Bauer, 2016/05/04 11:46:58]

Super-nit: space after colon :)

[jkrcal, 2016/05/04 14:04:07]

Done.

+                  << error_response;
 
   } else {
     bool stores_result_to_string = source->GetResponseAsString(&response);
     DCHECK(stores_result_to_string);
   }
 
   callback_list_.Notify(response, message);
 }
 
 }  // namespace ntp_snippets