[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>
+
+#include "base/command_line.h"
+#include "base/json/json_writer.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/sparse_histogram.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/clock.h"
+#include "base/values.h"
+#include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/ntp_snippets/breaking_news/subscription_request_params.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"
+#include "components/strings/grit/components_strings.h"
+#include "components/variations/net/variations_http_headers.h"
+#include "components/variations/variations_associated_data.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_response_headers.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 "third_party/icu/source/common/unicode/uloc.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
+#include "ui/base/l10n/l10n_util.h"
+
+using net::URLFetcher;
+using net::URLRequestContextGetter;
+using net::HttpRequestHeaders;
+using net::URLRequestStatus;
+
+namespace ntp_snippets {
+
+namespace internal {
+
+SubscriptionJsonRequest::SubscriptionJsonRequest(
+    const ParseJSONCallback& callback)
+    : parse_json_callback_(callback), weak_ptr_factory_(this) {}
+
+SubscriptionJsonRequest::~SubscriptionJsonRequest() {
+  LOG_IF(DFATAL, !request_completed_callback_.is_null())

[fhorschig, 2017/05/31 14:51:12]

As bernhard said, this is weird (sorry).
Could you improve that by calling any pending callback with 
a temporary error?
(The urlfetcher is destroyed and the request cancelled here.)

[mamir, 2017/05/31 18:46:02]

Is it OK as of now?

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

Yes, sgtm.

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

Acknowledged.

+      << "The CompletionCallback was never called!";
+}
+
+void SubscriptionJsonRequest::Start(CompletedCallback callback) {
+  request_completed_callback_ = std::move(callback);
+  url_fetcher_->Start();
+}
+
+std::string SubscriptionJsonRequest::GetResponseString() const {
+  std::string response;
+  url_fetcher_->GetResponseAsString(&response);
+  return response;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// 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(/*result=*/nullptr,

[fhorschig, 2017/05/31 14:51:12]

As discussed, a status code is sufficient if result is always nullptr.

[mamir, 2017/05/31 18:46:02]

Done.

+             /*error_details=*/base::StringPrintf(" %d", status.error()));
+  } else if (response != net::HTTP_OK) {
+    std::move(request_completed_callback_)
+        .Run(/*result=*/nullptr,

[fhorschig, 2017/05/31 14:51:12]

same here. status should suffice.

[mamir, 2017/05/31 18:46:02]

Done.

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

[fhorschig, 2017/05/31 14:51:12]

here you could use a success status.

[mamir, 2017/05/31 18:46:02]

Done.

+  }
+}
+
+SubscriptionJsonRequest::Builder::Builder() {}
+SubscriptionJsonRequest::Builder::Builder(SubscriptionJsonRequest::Builder&&) =
+    default;

[fhorschig, 2017/05/31 14:51:12]

I really want to know whether we need that.

[mamir, 2017/05/31 18:46:01]

Required the test IIUC

+SubscriptionJsonRequest::Builder::~Builder() = default;
+
+std::unique_ptr<SubscriptionJsonRequest>
+SubscriptionJsonRequest::Builder::Build() const {
+  DCHECK(!url_.is_empty());
+  DCHECK(url_request_context_getter_);
+  auto request =
+      base::MakeUnique<SubscriptionJsonRequest>(parse_json_callback_);
+  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::SetParams(
+    const SubscriptionRequestParams& params) {
+  params_ = params;
+  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;
+}
+
+SubscriptionJsonRequest::Builder&
+SubscriptionJsonRequest::Builder::SetParseJsonCallback(
+    ParseJSONCallback callback) {
+  parse_json_callback_ = callback;
+  return *this;
+}
+
+std::string SubscriptionJsonRequest::Builder::BuildHeaders() const {
+  net::HttpRequestHeaders headers;
+  headers.SetHeader("Content-Type", "application/json; charset=UTF-8");
+  if (!auth_header_.empty()) {
+    headers.SetHeader("Authorization", auth_header_);

[fhorschig, 2017/05/31 14:51:13]

The auth_header_ is always empty for the forseeable future, right?
Would it make sense to introduce it later, when we need it?

[mamir, 2017/05/31 18:46:01]

Yes it makes sense.
Removed.

+  }
+  // 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.
+  bool is_signed_in = false;

[fhorschig, 2017/05/31 14:51:12]

why don't you inline that?

[mamir, 2017/05/31 18:46:01]

Done.

+  variations::AppendVariationHeaders(url_,
+                                     false,  // incognito
+                                     false,  // uma_enabled
+                                     is_signed_in, &headers);
+  return headers.ToString();
+}
+
+std::string SubscriptionJsonRequest::Builder::BuildBody() const {
+  auto request = base::MakeUnique<base::DictionaryValue>();

[fhorschig, 2017/05/31 14:51:12]

Can we use a base::DictionaryValue instead of a pointer here?

[mamir, 2017/05/31 18:46:02]

Done.

+  request->SetString("token", params_.token);
+
+  std::string request_json;
+  bool success = base::JSONWriter::WriteWithOptions(
+      *request, base::JSONWriter::OPTIONS_PRETTY_PRINT, &request_json);
+  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 received breaking news via GCM push messages. "
+            "This request suscribes the client to receiving them."
+          trigger:
+            "Suscrpion takes place only once per profile lifetime. "
+          data:
+            "The sunscrption 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);

[fhorschig, 2017/05/31 14:51:12]

Do we want to retry three times?

[mamir, 2017/05/31 18:46:02]

What do you think? :-)

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

I doubt it. Once should suffice.

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

Done.

+  return url_fetcher;
+}
+
+}  // namespace internal
+
+}  // namespace ntp_snippets