[Offline Prefetch] Prefetch request fetcher

components/offline_pages/core/prefetch/prefetch_request_fetcher.cc

+// Copyright 2017 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/offline_pages/core/prefetch/prefetch_request_fetcher.h"
+
+#include "base/logging.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_status.h"
+#include "url/gurl.h"
+
+namespace offline_prefetch {
+
+namespace {
+const char kRequestContentType[] = "application/x-protobuf";
+}  // namespace
+
+PrefetchRequestFetcher::PrefetchRequestFetcher(
+    const GURL& url,
+    const std::string& message,
+    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    const FinishedCallback& callback)
+    : request_context_getter_(request_context_getter), callback_(callback) {
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation("offline_prefetch", R"(
+        semantics {
+          sender: "Offline Prefetch"
+          description:
+            "Chromium interacts with Offline Page Service to prefetch "
+            "suggested website resources."
+          trigger:
+            "When there are suggested website resources to fetch."
+          data:
+            "URLs of the suggested website resources to fetch."
+          destination: GOOGLE_OWNED_SERVICE
+        }
+        policy {
+          cookies_allowed: false
+          setting:
+            "Users can enable or disable the offline prefetch by toggling"
+            "chrome://flags#offline-prefetch in Chromium on Android."
+          policy_exception_justification:
+            "Not implemented, considered not useful."
+        })");
+  url_fetcher_ = net::URLFetcher::Create(url, net::URLFetcher::POST, this,
+                                         traffic_annotation);
+  url_fetcher_->SetRequestContext(request_context_getter_.get());

[dewittj, 2017/05/02 21:16:35]

I think this section also needs:
url_fetcher_->SetAutomaticallyRetryOn5xx(false);
url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(0);
url_fetcher_->SetStopOnRedirect(true);

Even if they aren't needed, we should have tests for these cases.

[jianli, 2017/05/02 22:23:10]

Added 1st and 2nd. For SetStopOnRedirect, I am a bit hesitated since I think
redirect is a perfectly fine case for server to reroute/reconfig. Otherwise the
client will just break. Unless redirect will cause harm to the client or the
server has an explicit doc saying no redirect, we will probably support it.

It is hard to test disbaling these retries since TestURLFetcher does not support
this.

+  url_fetcher_->SetUploadData(kRequestContentType, message);
+  url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SAVE_COOKIES);
+  url_fetcher_->Start();
+}
+
+PrefetchRequestFetcher::~PrefetchRequestFetcher() {}
+
+void PrefetchRequestFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
+  std::string data;
+  Status status = ParseResponse(source, &data);
+
+  // TODO(jianli): Report UMA.

[dewittj, 2017/05/02 21:16:35]

nit: TODO(crbug.com/XXX)

[jianli, 2017/05/02 22:23:10]

I am going to add this probably in next patch after we hook up the fetcher code
with service and start to do real testing. Creating a new crbug seems not to be
necessary.

+
+  callback_.Run(status, data);
+}
+
+PrefetchRequestFetcher::Status PrefetchRequestFetcher::ParseResponse(
+    const net::URLFetcher* source,
+    std::string* data) {
+  if (!source->GetStatus().is_success()) {
+    DVLOG(1) << "Net error: " << source->GetStatus().ToNetError();
+    return Status::SHOULD_RETRY_WITHOUT_BACKOFF;

[dewittj, 2017/05/02 21:16:35]

ERR_BLOCKED_BY_ADMINISTRATOR -> SHOULD_SUSPEND

ERR_CONNECTION_RESET -> SHOULD_RETRY_WITH_BACKOFF
ERR_CONNECTION_CLOSED -> SHOULD_RETRY_WITH_BACKOFF
ERR_CONNECTION_REFUSED -> SHOULD_RETRY_WITH_BACKOFF
SSL Handshake Problems -> SHOULD_RETRY_WITH_BACKOFF

[jianli, 2017/05/02 22:23:10]

Added ERR_BLOCKED_BY_ADMINISTRATOR. All other net errors are simply treated as
SHOULD_RETRY_WITH_BACKOFF.

+  }
+
+  net::HttpStatusCode response_status =
+      static_cast<net::HttpStatusCode>(source->GetResponseCode());
+  if (response_status != net::HTTP_OK) {
+    DVLOG(1) << "HTTP status: " << response_status;

[dewittj, 2017/05/02 21:16:35]

http 400 is currently DISCARD per the design doc
go/network-errors-and-responses-when-prefetching-offline-pages - either need to
add that here or modify the design doc.

[jianli, 2017/05/02 22:23:10]

I also think we may not want to support DISCARD since it is not necessary and
make the whole design more complciated. Left a feedback on the doc.

+    if (response_status == net::HTTP_NOT_IMPLEMENTED)
+      return Status::SHOULD_SUSPEND;
+    return Status::SHOULD_RETRY_WITH_BACKOFF;
+  }
+
+  if (!source->GetResponseAsString(data) || data->empty()) {
+    DVLOG(1) << "Failed to get response or empty response";
+    return Status::SHOULD_RETRY_WITH_BACKOFF;
+  }
+
+  return Status::SUCCESS;
+}
+
+}  // offline_prefetch