NTP: Extract JSON requests from Fetcher.

components/ntp_snippets/remote/ntp_snippets_fetcher.h

Index: components/ntp_snippets/remote/ntp_snippets_fetcher.h
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.h b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
index 40da2f820e093dabb4156d6c34d4a35a6711049c..8201b1a3cf0acdec3521c79f8837d0fa16de7622 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.h
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
@@ -7,24 +7,20 @@
 
 #include <memory>
 #include <queue>
-#include <set>
 #include <string>
 #include <utility>
-#include <vector>
 
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/time/tick_clock.h"
-#include "base/time/time.h"
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/category_info.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
+#include "components/ntp_snippets/remote/ntp_snippets_json_request.h"
 #include "components/ntp_snippets/remote/request_throttler.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"
 #include "net/url_request/url_request_context_getter.h"
 
 class PrefService;
@@ -36,6 +32,10 @@ class Value;
 
 namespace ntp_snippets {
 
+namespace internal {

[tschumann, 2016/12/16 16:36:20]

it's not in internal anymore ;-)

[fhorschig, 2016/12/20 10:07:38]

Done.

+class NTPSnippetsRequestParams;
+}
+
 class UserClassifier;
 
 // TODO(tschumann): BuildArticleCategoryInfo() and BuildRemoteCategoryInfo()
@@ -58,15 +58,8 @@ CategoryInfo BuildRemoteCategoryInfo(const base::string16& title,
 class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
                            public OAuth2TokenService::Observer {
  public:
-  // Callbacks for JSON parsing, needed because the parsing is platform-
-  // dependent.
-  using SuccessCallback =
-      base::Callback<void(std::unique_ptr<base::Value> result)>;
-  using ErrorCallback = base::Callback<void(const std::string& error)>;
-  using ParseJSONCallback =
-      base::Callback<void(const std::string& raw_json_string,
-                          const SuccessCallback& success_callback,
-                          const ErrorCallback& error_callback)>;
+  // Exposing JSON parsing callback that needs to be injected on creation.
+  using ParseJSONCallback = internal::NTPSnippetsJsonRequest::ParseJSONCallback;

[tschumann, 2016/12/16 16:36:20]

for types which are exposed publicly, I'd try to avoid alias names and also
avoid exposing internal:: names.
What about moving NTPSnippetsRequestParams, ParseJSONCallback, and
Personalization enum into the ntp_snippets_request_params.h?

side note: untangling the cyclic dependencies between the ntsnippetsfetcher and
the json-request is cumbersome. It's actually a smell and could be resolved by
extracting a SnippetsFetcher interface. There we could define all shared types
as well. The SnippetsFetcherImpl can then depend on both, the interface and the
json-request builder.
Such an interface has another benefit that we can define clean mocks for the
NTPSnippetsFetcher (if we want to use it in tests). Might be worth a TODO(). But
we should probably only address that once we have the need to use a proper Mock
:-)

[fhorschig, 2016/12/20 10:07:38]

Added the TODO. The idea is great, but refactoring in this large CL is not.

 
   struct FetchedCategory {
     Category category;
@@ -81,22 +74,6 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   using FetchedCategoriesVector = std::vector<FetchedCategory>;
   using OptionalFetchedCategories = base::Optional<FetchedCategoriesVector>;
 
-  // Enumeration listing all possible outcomes for fetch attempts. Used for UMA
-  // histograms, so do not change existing values. Insert new values at the end,
-  // and update the histogram definition.
-  enum class FetchResult {
-    SUCCESS,
-    DEPRECATED_EMPTY_HOSTS,
-    URL_REQUEST_STATUS_ERROR,
-    HTTP_ERROR,
-    JSON_PARSE_ERROR,
-    INVALID_SNIPPET_CONTENT_ERROR,
-    OAUTH_TOKEN_ERROR,
-    INTERACTIVE_QUOTA_ERROR,
-    NON_INTERACTIVE_QUOTA_ERROR,
-    RESULT_MAX
-  };
-
   // |snippets| contains parsed snippets if a fetch succeeded. If problems
   // occur, |snippets| contains no value (no actual vector in base::Optional).
   // Error details can be retrieved using last_status().
@@ -104,33 +81,6 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
       base::OnceCallback<void(Status status,
                               OptionalFetchedCategories fetched_categories)>;
 
-  // Enumeration listing all possible variants of dealing with personalization.
-  enum class Personalization { kPersonal, kNonPersonal, kBoth };
-
-  // Contains all the parameters for one fetch.
-  struct Params {
-    Params();
-    Params(const Params&);
-    ~Params();
-
-    // BCP 47 language code specifying the user's UI language.
-    std::string language_code;
-
-    // A set of suggestion IDs that should not be returned again.
-    std::set<std::string> excluded_ids;
-
-    // Maximum number of snippets to fetch.
-    int count_to_fetch = 0;
-
-    // Whether this is an interactive request, i.e. triggered by an explicit
-    // user action. Typically, non-interactive requests are subject to a daily
-    // quota.
-    bool interactive_request = false;
-
-    // If set, only return results for this category.
-    base::Optional<Category> exclusive_category;
-  };
-
   NTPSnippetsFetcher(
       SigninManagerBase* signin_manager,
       OAuth2TokenService* token_service,
@@ -144,12 +94,12 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   ~NTPSnippetsFetcher() override;
 
   // Initiates a fetch from the server. When done (successfully or not), calls
-  // the subscriber of SetCallback().
+  // the callback.
   //
-  // If an ongoing fetch exists, it will be silently abandoned and a new one
-  // started, without triggering an additional callback (i.e. the callback will
-  // only be called once).
-  void FetchSnippets(const Params& params, SnippetsAvailableCallback callback);
+  // If an ongoing fetch exists, both fetches won't influence each other (i.e.
+  // every callback will be called exactly once).
+  void FetchSnippets(const NTPSnippetsRequestParams& params,
+                     SnippetsAvailableCallback callback);
 
   // Debug string representing the status/result of the last fetch attempt.
   const std::string& last_status() const { return last_status_; }
@@ -160,7 +110,7 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   }
 
   // Returns the personalization setting of the fetcher.
-  Personalization personalization() const { return personalization_; }
+  internal::Personalization personalization() const { return personalization_; }
 
   // Returns the URL endpoint used by the fetcher.
   const GURL& fetch_url() const { return fetch_url_; }
@@ -187,98 +137,16 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
                            BuildRequestWithOtherLanguageOnly);
   friend class ChromeReaderSnippetsFetcherTest;
 
-  enum FetchAPI {
-    CHROME_READER_API,
-    CHROME_CONTENT_SUGGESTIONS_API,
-  };
-
-  class JsonRequest;
-
-  // A class that builds authenticated and non-authenticated JsonRequests.
-  // This class is only in the header for testing.
-  // TODO(fhorschig): Move into separate file with snippets::internal namespace.
-  class RequestBuilder {
-   public:
-    RequestBuilder();
-    RequestBuilder(RequestBuilder&&);
-    ~RequestBuilder();
-
-    // Builds a Request object that contains all data to fetch new snippets.
-    std::unique_ptr<JsonRequest> Build() const;
-
-    RequestBuilder& SetAuthentication(const std::string& account_id,
-                                      const std::string& auth_header);
-    RequestBuilder& SetCreationTime(base::TimeTicks creation_time);
-    RequestBuilder& SetFetchAPI(FetchAPI fetch_api);
-    // The language_model borrowed from the fetcher needs to stay alive until
-    // the request body is built.
-    RequestBuilder& SetLanguageModel(
-        const translate::LanguageModel* language_model);
-    RequestBuilder& SetParams(const Params& params);
-    RequestBuilder& SetParseJsonCallback(ParseJSONCallback callback);
-    RequestBuilder& SetPersonalization(Personalization personalization);
-    // The tick_clock borrowed from the fetcher will be injected into the
-    // request. It will be used at build time and after the fetch returned.
-    // It has to be alive until the request is destroyed.
-    RequestBuilder& SetTickClock(base::TickClock* tick_clock);
-    RequestBuilder& SetUrl(const GURL& url);
-    RequestBuilder& SetUrlRequestContextGetter(
-        const scoped_refptr<net::URLRequestContextGetter>& context_getter);
-    RequestBuilder& SetUserClassifier(const UserClassifier& user_classifier);
-
-    // These preview methods allow to inspect the Request without exposing it
-    // publicly.
-    // TODO(fhorschig): Remove these when moving the RequestBuilder to
-    // snippets::internal and trigger the request to intercept the request.
-    std::string PreviewRequestBodyForTesting() { return BuildBody(); }
-    std::string PreviewRequestHeadersForTesting() { return BuildHeaders(); }
-    RequestBuilder& SetUserClassForTesting(const std::string& user_class) {
-      user_class_ = user_class;
-      return *this;
-    }
-
-   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;
-
-    bool ReturnOnlyPersonalizedResults() const {
-      return !obfuscated_gaia_id_.empty() &&
-             personalization_ == NTPSnippetsFetcher::Personalization::kPersonal;
-    }
-
-    void PrepareLanguages(
-        translate::LanguageModel::LanguageInfo* ui_language,
-        translate::LanguageModel::LanguageInfo* other_top_language) const;
-
-    // Only required, if the request needs to be sent.
-    std::string auth_header_;
-    base::TickClock* tick_clock_;
-    FetchAPI fetch_api_;
-    Params params_;
-    ParseJSONCallback parse_json_callback_;
-    Personalization personalization_;
-    GURL url_;
-    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
-
-    // Optional properties.
-    std::string obfuscated_gaia_id_;
-    std::string user_class_;
-    const translate::LanguageModel* language_model_;
-
-    DISALLOW_COPY_AND_ASSIGN(RequestBuilder);
-  };
-
-  void FetchSnippetsNonAuthenticated(RequestBuilder builder,
-                                     SnippetsAvailableCallback callback);
-  void FetchSnippetsAuthenticated(RequestBuilder builder,
-                                  SnippetsAvailableCallback callback,
-                                  const std::string& account_id,
-                                  const std::string& oauth_access_token);
-  void StartRequest(RequestBuilder builder, SnippetsAvailableCallback callback);
+  void FetchSnippetsNonAuthenticated(
+      internal::NTPSnippetsJsonRequest::Builder builder,
+      SnippetsAvailableCallback callback);
+  void FetchSnippetsAuthenticated(
+      internal::NTPSnippetsJsonRequest::Builder builder,
+      SnippetsAvailableCallback callback,
+      const std::string& account_id,
+      const std::string& oauth_access_token);
+  void StartRequest(internal::NTPSnippetsJsonRequest::Builder builder,
+                    SnippetsAvailableCallback callback);
 
   void StartTokenRequest();
 
@@ -292,14 +160,15 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   // OAuth2TokenService::Observer overrides:
   void OnRefreshTokenAvailable(const std::string& account_id) override;
 
-  void JsonRequestDone(std::unique_ptr<JsonRequest> request,
-                       SnippetsAvailableCallback callback,
-                       std::unique_ptr<base::Value> result,
-                       FetchResult status_code,
-                       const std::string& error_details);
+  void JsonRequestDone(
+      std::unique_ptr<internal::NTPSnippetsJsonRequest> request,
+      SnippetsAvailableCallback callback,
+      std::unique_ptr<base::Value> result,
+      internal::FetchResult status_code,
+      const std::string& error_details);
   void FetchFinished(OptionalFetchedCategories categories,
                      SnippetsAvailableCallback callback,
-                     FetchResult status_code,
+                     internal::FetchResult status_code,
                      const std::string& error_details);
 
   bool JsonToSnippets(const base::Value& parsed,
@@ -323,7 +192,8 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
 
   // Stores requests that wait for an access token.
-  std::queue<std::pair<RequestBuilder, SnippetsAvailableCallback>>
+  std::queue<std::pair<internal::NTPSnippetsJsonRequest::Builder,
+                       SnippetsAvailableCallback>>
       pending_requests_;
 
   // Weak references, not owned.
@@ -335,13 +205,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   // API endpoint for fetching snippets.
   const GURL fetch_url_;
   // Which API to use
-  const FetchAPI fetch_api_;
+  const internal::FetchAPI fetch_api_;
 
   // API key to use for non-authenticated requests.
   const std::string api_key_;
 
   // The variant of the fetching to use, loaded from variation parameters.
-  Personalization personalization_;
+  internal::Personalization personalization_;
 
   // Allow for an injectable tick clock for testing.
   std::unique_ptr<base::TickClock> tick_clock_;