Fetch snippets from ChromeReader and show them on the NTP

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 "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "base/task_runner_util.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 "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"
+
+using net::URLFetcher;
+using net::URLRequestContextGetter;
+using net::HttpRequestHeaders;
+using net::URLRequestStatus;
+
+namespace ntp_snippets {
+
+const char kSnippetSuggestionsFilename[] = "ntp_snippets.json";
+const char kApiScope[] = "https://www.googleapis.com/auth/webhistory";
+const char kContentSnippetsServer[] =
+    "https://chromereader-pa.googleapis.com/v1/fetch";
+const char kAuthorizationRequestHeaderFormat[] = "Bearer %s";
+
+const char kUnpersonalizedRequestParameters[] =
+    "{ \"response_detail_level\": \"FULL_DEBUG\", \"advanced_options\": { "
+    "\"local_scoring_params\": {\"content_params\" : { "
+    "\"only_return_personalized_results\": false } }, "
+    "\"global_scoring_params\": { \"num_to_return\": 10 } } }";
+
+base::FilePath GetSnippetsSuggestionsPath() {
+  base::FilePath dir;
+#if defined(OS_ANDROID)
+  CHECK(PathService::Get(base::DIR_ANDROID_APP_DATA, &dir));

[Bernhard Bauer, 2016/02/08 18:19:26]

DCHECK should be fine for this.

[May, 2016/02/09 17:38:53]

Done.

+#else
+  NOTIMPLEMENTED();

[Bernhard Bauer, 2016/02/08 18:19:26]

Maybe also return an empty FilePath in this case? Also, maybe make this a
NOTREACHED() as long as this is not supposed to be used anywhere but Android?

[May, 2016/02/09 17:38:53]

Done.

+#endif
+  return dir.AppendASCII(kSnippetSuggestionsFilename);
+}
+
+NTPSnippetsFetcher::NTPSnippetsFetcher(
+    scoped_refptr<base::SequencedTaskRunner> file_task_runner,
+    SigninManager* signin_manager,
+    OAuth2TokenService* token_service,
+    URLRequestContextGetter* url_request_context_getter)
+    : OAuth2TokenService::Consumer("NTP_snippets"),
+      file_task_runner_(file_task_runner),
+      url_request_context_getter_(url_request_context_getter),
+      signin_manager_(signin_manager),
+      token_service_(token_service),
+      weak_ptr_factory_(this) {}
+
+NTPSnippetsFetcher::~NTPSnippetsFetcher() {}
+
+void NTPSnippetsFetcher::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void NTPSnippetsFetcher::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void NTPSnippetsFetcher::FetchSnippets(bool overwrite) {
+  if (overwrite) {
+    StartFetch();
+  } else {
+    base::PostTaskAndReplyWithResult(
+        file_task_runner_.get(), FROM_HERE,
+        base::Bind(&base::PathExists, GetSnippetsSuggestionsPath()),
+        base::Bind(&NTPSnippetsFetcher::OnFileExistsCheckDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void NTPSnippetsFetcher::OnFileExistsCheckDone(bool exists) {
+  if (exists) {
+    NotifyObservers();
+  } else {
+    StartFetch();
+  }
+}
+
+void NTPSnippetsFetcher::StartFetch() {
+  if (signin_manager_->IsAuthenticated()) {
+    StartTokenRequest();
+  } else {
+    // Wait until we get a refresh token.
+    token_service_->AddObserver(this);
+  }
+}
+
+void NTPSnippetsFetcher::StartTokenRequest() {
+  OAuth2TokenService::ScopeSet scopes;
+  scopes.insert(kApiScope);
+  oauth_request_ = token_service_->StartRequest(
+      signin_manager_->GetAuthenticatedAccountId(), scopes, this);
+}
+
+void NTPSnippetsFetcher::NotifyObservers() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnNTPSnippetsDownloaded());
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Consumer overrides
+void NTPSnippetsFetcher::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  oauth_request_.reset();
+  url_fetcher_ =
+      URLFetcher::Create(GURL(kContentSnippetsServer), URLFetcher::POST, this);
+  url_fetcher_->SetRequestContext(url_request_context_getter_);
+  url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SAVE_COOKIES);
+  HttpRequestHeaders headers;
+  headers.SetHeader("Authorization",
+                    base::StringPrintf(kAuthorizationRequestHeaderFormat,
+                                       access_token.c_str()));
+  headers.SetHeader("Content-Type", "application/json; charset=UTF-8");
+  headers.SetHeader("X-GFE-SSL", "yes");

[Bernhard Bauer, 2016/02/08 18:19:26]

o_O Why do we need this?

[May, 2016/02/09 17:38:53]

I'm not sure why we need it. I'd copied the request headers that apido sends
(command line the tool that OnePlatform uses for testing deployed services).

BUT BUT... I removed this just to see what would happen and it doesn't seem to
make a difference

+  url_fetcher_->SetExtraRequestHeaders(headers.ToString());
+  url_fetcher_->SetUploadData("application/json",
+                              kUnpersonalizedRequestParameters);
+  url_fetcher_->SaveResponseToTemporaryFile(file_task_runner_.get());
+  url_fetcher_->Start();
+}
+
+void NTPSnippetsFetcher::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  oauth_request_.reset();
+  DLOG(ERROR) << "Unable to get token: " << error.ToString();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Observer overrides
+void NTPSnippetsFetcher::OnRefreshTokenAvailable(
+    const std::string& account_id) {
+  token_service_->RemoveObserver(this);

[Marc Treib, 2016/02/09 09:17:54]

We should probably check if account_id matches the one we expect?

[May, 2016/02/09 17:38:53]

If we're going to keep using signed in accounts, yeah. But I'm going to change
this whole class to not require sign in given that ChromeReader now supports it.

[Marc Treib, 2016/02/10 10:44:43]

Acknowledged.

+  StartTokenRequest();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// URLFetcherDelegate overrides
+void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
+  DCHECK_EQ(url_fetcher_.get(), source);
+
+  const URLRequestStatus& status = source->GetStatus();
+  if (!status.is_success()) {
+    DLOG(WARNING) << "URLRequestStatus error " << status.error()
+                  << " while trying to download " << source->GetURL().spec();
+    return;
+  }
+
+  int response_code = source->GetResponseCode();
+  if (response_code != net::HTTP_OK) {

[Bernhard Bauer, 2016/02/08 18:19:26]

Technically, you should also handle the case where there is a race condition and
the access token just expires between the time you get it back from the cache
and the time it arrives at the server, in which case you have to retry the token
request to get a new one (but only once, after that an error should abort).

Really, we should extract all of this dance into a shared class. Maybe not for
this CL, but *cough* technical debt *cough*... Marc, WDYT?

Actually, why do we even use authentication here?

[Marc Treib, 2016/02/09 09:17:54]

Yup, I was thinking the same thing while going through this code.
Because (at least until recently) ChromeReader required authentication.

[May, 2016/02/09 17:38:53]

Yes we should. :) However, since we're moving to a model where we won't even use
the token to fetch the data (because we will instead use the user's history to
filter the snippets to get relevant results) so that issue will not come up.

And what Marc said -- we needed authentication to access the API previously. In
theory that's not the case anymore but I haven't had a chance to test it yet. I
have a bug to switch over to not requiring sign in to access ChromeReader
(crbug/584346).

[Marc Treib, 2016/02/10 10:44:43]

So for the record, I'm fine with landing as-is for now, so that we have
something that works :)

[May, 2016/02/10 18:16:46]

Acknowledged.

+    DLOG(WARNING) << "HTTP error " << response_code
+                  << " while trying to download " << source->GetURL().spec();
+    return;
+  }
+
+  base::FilePath response_path;
+  source->GetResponseAsFilePath(false, &response_path);
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&base::Move, response_path, GetSnippetsSuggestionsPath()),
+      base::Bind(&NTPSnippetsFetcher::OnFileMoveDone,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NTPSnippetsFetcher::OnFileMoveDone(bool success) {
+  if (!success) {
+    DLOG(WARNING) << "Could not move file to "
+                  << GetSnippetsSuggestionsPath().LossyDisplayName();
+    return;
+  }
+
+  NotifyObservers();
+}
+
+}  // namespace ntp_snippets