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

components/ntp_snippets/breaking_news/subscription_json_request.h

+// 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.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_BREAKING_NEWS_SUBSCRIPTION_JSON_REQUEST_H_
+#define COMPONENTS_NTP_SNIPPETS_BREAKING_NEWS_SUBSCRIPTION_JSON_REQUEST_H_
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/breaking_news/subscription_request_params.h"
+#include "components/ntp_snippets/status.h"
+#include "components/translate/core/browser/language_model.h"
+#include "google_apis/gaia/oauth2_token_service.h"
+#include "net/http/http_request_headers.h"
+
+namespace base {
+class Value;
+}  // namespace base
+
+namespace ntp_snippets {
+
+namespace internal {
+
+// A single request to subscribe for breaking news via GCM,

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

s/,/.

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

Done.

+class SubscriptionJsonRequest : public net::URLFetcherDelegate {
+ public:
+  // A client can expect error_details only, if there was any error during the
+  // subscription or parsing. In successful cases, it will be an empty string.
+  using CompletedCallback =
+      base::OnceCallback<void(std::unique_ptr<base::Value> result,
+                              const std::string& error_details)>;

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

How are error details used?
Could we handle errors with the existing ntp_snippets::StatusCode?
(see components/ntp_snippets/status.h)

It seems the result is never used (or present in the response).
Could you then simplify the callback to
base::OnceCallback<void(const ntp_snippets::Status& status)>?

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

Done.

+
+  // Builds authenticated and non-authenticated SubscriptionJsonRequests.
+  class Builder {
+   public:
+    Builder();
+    Builder(Builder&&);

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

Please check if the declaration of this move constructor is necessary.

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

Needed in the test IIUC.

+    ~Builder();
+
+    // Builds a Request object that contains all data to fetch new snippets.
+    std::unique_ptr<SubscriptionJsonRequest> Build() const;
+
+    Builder& SetParams(const SubscriptionRequestParams& params);
+    Builder& SetUrl(const GURL& url);
+    Builder& SetUrlRequestContextGetter(
+        const scoped_refptr<net::URLRequestContextGetter>& context_getter);
+    Builder& SetParseJsonCallback(ParseJSONCallback callback);

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

Drop if you don't need to parse JSON.

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

Done.

+
+    // These preview methods allow to inspect the Request without exposing it
+    // publicly.
+    std::string PreviewRequestBodyForTesting() { return BuildBody(); }
+    std::string PreviewRequestHeadersForTesting() { return BuildHeaders(); }
+
+   private:
+    std::string BuildHeaders() const;
+    std::string BuildBody() const;
+    std::unique_ptr<net::URLFetcher> BuildURLFetcher(
+        net::URLFetcherDelegate* request,
+        const std::string& headers,
+        const std::string& body) const;
+
+    // Only required, if the request needs to be sent.
+    std::string auth_header_;
+    SubscriptionRequestParams params_;
+    ParseJSONCallback parse_json_callback_;
+    GURL url_;
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+
+    // Optional properties.
+    std::string obfuscated_gaia_id_;
+
+    DISALLOW_COPY_AND_ASSIGN(Builder);
+  };
+
+  SubscriptionJsonRequest(const ParseJSONCallback& callback);

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

As discussed, I guess this constructor should be private.
Drop the callback if you don't need to parse JSON.

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

Done.

+  SubscriptionJsonRequest(SubscriptionJsonRequest&&);

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

Do you need this? (it's =default in cpp ... please let me know)

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

Removed!

+  ~SubscriptionJsonRequest() override;
+
+  void Start(CompletedCallback callback);
+
+  std::string GetResponseString() const;

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

Will you need that?
Usually all the status you need come with the callback.

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

Removed!

+
+ private:
+  // URLFetcherDelegate implementation.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  // The fetcher for subscrbing. Only non-null if a subscription process is
+  // currently ongoing.
+  std::unique_ptr<net::URLFetcher> url_fetcher_;
+
+  // This callback is called to parse a json string. It contains callbacks for
+  // error and success cases.
+  ParseJSONCallback parse_json_callback_;

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

Drop if you don't need to parse JSON.

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

Done.

+
+  // The callback to notify when URLFetcher finished and results are available.
+  CompletedCallback request_completed_callback_;
+
+  base::WeakPtrFactory<SubscriptionJsonRequest> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SubscriptionJsonRequest);
+};
+
+}  // namespace internal
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_BREAKING_NEWS_SUBSCRIPTION_JSON_REQUEST_H_