Allow fetching personalized snippets from ChromeReader.

components/ntp_snippets/ntp_snippets_fetcher.cc

Index: components/ntp_snippets/ntp_snippets_fetcher.cc
diff --git a/components/ntp_snippets/ntp_snippets_fetcher.cc b/components/ntp_snippets/ntp_snippets_fetcher.cc
index acad45cf88cc3417be13c39811ca99de7c98a917..9c3ca223ebe4e9ae7b079354558473ef9fe3200b 100644
--- a/components/ntp_snippets/ntp_snippets_fetcher.cc
+++ b/components/ntp_snippets/ntp_snippets_fetcher.cc
@@ -4,6 +4,8 @@
 
 #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"
@@ -11,12 +13,18 @@
 #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;
@@ -27,13 +35,27 @@ 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";
+
+// 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.

 
-const char kRequestParameterFormat[] =
+// 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\": {"
@@ -43,20 +65,36 @@ const char kRequestParameterFormat[] =
     "      },"
     "      \"content_restricts\": {"
     "        \"type\": \"METADATA\","
-    "        \"value\": \"TITLE\""
-    "      },"
-    "      \"content_restricts\": {"
-    "        \"type\": \"METADATA\","
-    "        \"value\": \"SNIPPET\""
+    "        \"value\": \"ALL\""
+    "      }"
+    "%s"
+    "    },"
+    "    \"global_scoring_params\": {"
+    "      \"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\": \"THUMBNAIL\""
+    "        \"value\": \"ALL\""
     "      }"
     "%s"
     "    },"
     "    \"global_scoring_params\": {"
-    "      \"num_to_return\": %i"
+    "      \"num_to_return\": %i,"
+    "      \"sort_type\": 1"
     "    }"
     "  }"
     "}";
@@ -70,12 +108,27 @@ const char kHostRestrictFormat[] =
 }  // 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) {
@@ -83,27 +136,53 @@ NTPSnippetsFetcher::AddCallback(const SnippetsAvailableCallback& 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.
@@ -111,6 +190,89 @@ void NTPSnippetsFetcher::FetchSnippets(const std::set<std::string>& hosts,
   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) {
@@ -133,8 +295,11 @@ void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
 
   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);