[NTP::Push] Add the classes for sending a breaking news subscription request

components/ntp_snippets/breaking_news/subscription_json_request.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/ntp_snippets/breaking_news/subscription_json_request.h"
+
+#include <algorithm>
+#include <utility>
+#include <vector>

[fhorschig, 2017/06/01 09:23:56]

unused import

[mamir, 2017/06/01 14:52:31]

Done.

+
+#include "base/command_line.h"

[fhorschig, 2017/06/01 09:23:55]

unused import

[mamir, 2017/06/01 14:52:31]

Done.

+#include "base/json/json_writer.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/sparse_histogram.h"

[fhorschig, 2017/06/01 09:23:56]

Metrics includes unused

[mamir, 2017/06/01 14:52:31]

Done.

+#include "base/strings/stringprintf.h"
+#include "base/time/clock.h"

[fhorschig, 2017/06/01 09:23:55]

unused

[mamir, 2017/06/01 14:52:31]

Acknowledged.

+#include "base/values.h"
+#include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/ntp_snippets/category_info.h"
+#include "components/ntp_snippets/features.h"
+#include "components/ntp_snippets/user_classifier.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_manager_base.h"

[fhorschig, 2017/06/01 09:23:56]

unused:
components/ntp_snippets/*
components/signin/*

[mamir, 2017/06/01 14:52:31]

Acknowledged.

+#include "components/strings/grit/components_strings.h"
+#include "components/variations/net/variations_http_headers.h"
+#include "components/variations/variations_associated_data.h"

[fhorschig, 2017/06/01 09:23:55]

unused?

[mamir, 2017/06/01 14:52:31]

Acknowledged.

+#include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"

[fhorschig, 2017/06/01 09:23:56]

unused?

[mamir, 2017/06/01 14:52:31]

Acknowledged.

+#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 "third_party/icu/source/common/unicode/uloc.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
+#include "ui/base/l10n/l10n_util.h"

[fhorschig, 2017/06/01 09:23:55]

The last three imports seem unused as well ... please check all imports whether
they are truly needed.

[mamir, 2017/06/01 14:52:31]

Acknowledged.

+
+using net::URLFetcher;

[fhorschig, 2017/06/01 09:23:56]

either remove this or remove the net:: before every following URLFetcher.

[mamir, 2017/06/01 14:52:30]

Done.

+using net::URLRequestContextGetter;

[fhorschig, 2017/06/01 09:23:55]

either remove this or remove the net:: before every following
URLRequestContextGetter.

[mamir, 2017/06/01 14:52:31]

Done.

+using net::HttpRequestHeaders;

[fhorschig, 2017/06/01 09:23:55]

either remove this or remove the net:: before every following
HttpRequestHeaders.

[mamir, 2017/06/01 14:52:31]

Done.

+using net::URLRequestStatus;
+
+namespace ntp_snippets {
+
+namespace internal {
+
+SubscriptionJsonRequest::SubscriptionJsonRequest() : weak_ptr_factory_(this) {}
+
+SubscriptionJsonRequest::~SubscriptionJsonRequest() {
+  if (!request_completed_callback_.is_null()) {
+    std::move(request_completed_callback_)
+        .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR,
+                                  "cancelled"));
+  }
+}
+
+void SubscriptionJsonRequest::Start(CompletedCallback callback) {

[fhorschig, 2017/06/01 09:23:55]

What happens if you call this method twice?
If I didn't miss anything:
- existing (not called) callbacks are dropped and overridden by the new one.
- a second request is fired (which shouldn't make any sense)

You can should check if a request is still running (e.g. if the callback wasn't
called?)
There are several things you could do here:
a) queue the new callback and call it when the running request is done
b) throw a temporary error ("request ongoing")
c) something else I didn't think of (e.g. create a new request)

I think a) would be the cleanest solution.

[mamir, 2017/06/01 14:52:31]

Implemented option 2 after our offline discussion

+  request_completed_callback_ = std::move(callback);
+  url_fetcher_->Start();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// URLFetcherDelegate overrides
+void SubscriptionJsonRequest::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK_EQ(url_fetcher_.get(), source);
+  const URLRequestStatus& status = url_fetcher_->GetStatus();
+  int response = url_fetcher_->GetResponseCode();
+
+  if (!status.is_success()) {
+    std::move(request_completed_callback_)
+        .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR,
+                                  base::StringPrintf(" %d", status.error())));

[fhorschig, 2017/06/01 09:23:56]

This produces really hard to debug Status states.
by just printing " %d" in both failure cases,
the user always gets a TEMPORARY_ERROR and a number 
that may or may not be HTTP error code.


(why the space in " %d" btw?)

[mamir, 2017/06/01 14:52:31]

Done.

+  } else if (response != net::HTTP_OK) {
+    std::move(request_completed_callback_)
+        .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR,

[fhorschig, 2017/06/01 09:23:56]

nit: Is TEMPORARY_ERROR what we want to return?
I could imagine this being wrong for 4XX.

[mamir, 2017/06/01 14:52:31]

Done.

+                                  base::StringPrintf(" %d", response)));
+  } else {
+    // request succeeded.

[fhorschig, 2017/06/01 09:23:56]

nit: This obvious comment might be obvious.

[mamir, 2017/06/01 14:52:31]

Done.

+    std::move(request_completed_callback_)
+        .Run(ntp_snippets::Status(ntp_snippets::StatusCode::SUCCESS, ""));

[Bernhard Bauer, 2017/06/01 09:31:23]

Nit: Use an empty std::string() constructor instead of a literal to save some
instructions.

[mamir, 2017/06/01 14:52:31]

Done.

+  }
+}
+
+SubscriptionJsonRequest::Builder::Builder() {}
+SubscriptionJsonRequest::Builder::Builder(SubscriptionJsonRequest::Builder&&) =
+    default;
+SubscriptionJsonRequest::Builder::~Builder() = default;
+
+std::unique_ptr<SubscriptionJsonRequest>
+SubscriptionJsonRequest::Builder::Build() const {
+  DCHECK(!url_.is_empty());
+  DCHECK(url_request_context_getter_);
+  auto request = base::WrapUnique(new SubscriptionJsonRequest());
+
+  std::string body = BuildBody();
+  std::string headers = BuildHeaders();
+  request->url_fetcher_ = BuildURLFetcher(request.get(), headers, body);
+
+  // Log the request for debugging network issues.
+  VLOG(1) << "Sending a subscription request to " << url_ << ":\n"
+          << headers << "\n"
+          << body;
+
+  return request;
+}
+
+SubscriptionJsonRequest::Builder& SubscriptionJsonRequest::Builder::SetToken(
+    const std::string& token) {
+  token_ = token;
+  return *this;
+}
+
+SubscriptionJsonRequest::Builder& SubscriptionJsonRequest::Builder::SetUrl(
+    const GURL& url) {
+  url_ = url;
+  return *this;
+}
+
+SubscriptionJsonRequest::Builder&
+SubscriptionJsonRequest::Builder::SetUrlRequestContextGetter(
+    const scoped_refptr<net::URLRequestContextGetter>& context_getter) {
+  url_request_context_getter_ = context_getter;
+  return *this;
+}
+
+std::string SubscriptionJsonRequest::Builder::BuildHeaders() const {
+  net::HttpRequestHeaders headers;
+  headers.SetHeader("Content-Type", "application/json; charset=UTF-8");
+
+  // Add X-Client-Data header with experiment IDs from field trials.
+  // Note: It's OK to pass |is_signed_in| false if it's unknown, as it does
+  // not affect transmission of experiments coming from the variations server.
+  variations::AppendVariationHeaders(url_,
+                                     false,  // incognito
+                                     false,  // uma_enabled
+                                     false,  // is_signed_in
+                                     &headers);
+  return headers.ToString();
+}
+
+std::string SubscriptionJsonRequest::Builder::BuildBody() const {
+  base::DictionaryValue request;
+  request.SetString("token", token_);

[fhorschig, 2017/06/01 09:23:55]

You access a private member (namely url_fetcher_) above. Why use a setter here?

[mamir, 2017/06/01 14:52:31]

Ignore as per our offline discussion.

+
+  std::string request_json;
+  bool success = base::JSONWriter::WriteWithOptions(
+      request, base::JSONWriter::OPTIONS_PRETTY_PRINT, &request_json);

[fhorschig, 2017/06/01 09:23:55]

Can we drop the option?
(in tests, you compare by JSON anyway)

[mamir, 2017/06/01 14:52:31]

Done.

+  DCHECK(success);
+  return request_json;
+}
+
+std::unique_ptr<net::URLFetcher>
+SubscriptionJsonRequest::Builder::BuildURLFetcher(
+    net::URLFetcherDelegate* delegate,
+    const std::string& headers,
+    const std::string& body) const {
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation("gcm_subscription", R"(
+        semantics {
+          sender: "Subscribe for breaking news delivered via GCM push messages"
+          description:
+            "Chromium can receive breaking news via GCM push messages. "
+            "This request suscribes the client to receiving them."
+          trigger:
+            "Subscription takes place only once per profile lifetime. "
+          data:
+            "The subscription token that identifies this Chromium profile."
+          destination: GOOGLE_OWNED_SERVICE
+        }
+        policy {
+          cookies_allowed: false
+          setting:
+            "This feature cannot be disabled by settings now"
+          chrome_policy {
+            NTPContentSuggestionsEnabled {
+              policy_options {mode: MANDATORY}
+              NTPContentSuggestionsEnabled: false
+            }
+          }
+        })");
+  std::unique_ptr<net::URLFetcher> url_fetcher = net::URLFetcher::Create(
+      url_, net::URLFetcher::POST, delegate, traffic_annotation);
+  url_fetcher->SetRequestContext(url_request_context_getter_.get());
+  url_fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                            net::LOAD_DO_NOT_SAVE_COOKIES);
+  data_use_measurement::DataUseUserData::AttachToFetcher(
+      url_fetcher.get(),
+      data_use_measurement::DataUseUserData::NTP_SNIPPETS_SUGGESTIONS);
+
+  url_fetcher->SetExtraRequestHeaders(headers);
+  url_fetcher->SetUploadData("application/json", body);
+
+  // Fetchers are sometimes cancelled because a network change was detected.
+  url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3);
+  return url_fetcher;
+}
+
+}  // namespace internal
+
+}  // namespace ntp_snippets