[NTP Snippets] Clean up NTPSnippetsFetcher

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 f5bbf27412cc229053c8a01e15988a324e07afc0..270c1bc9d69391099faf08683b64b4946adf7de1 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.h
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
@@ -90,6 +90,30 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
     kBoth
   };
 
+  // Contains all the parameters for one fetch.
+  struct Params {
+    Params();
+    Params(const Params&);
+    ~Params();
+
+    // If non-empty, restricts the result to the given set of hosts.
+    std::set<std::string> hosts;
+
+    // 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;
+  };
+
   NTPSnippetsFetcher(
       SigninManagerBase* signin_manager,
       OAuth2TokenService* token_service,
@@ -106,24 +130,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   // overriding any previously set callback.
   void SetCallback(const SnippetsAvailableCallback& callback);
 
-  // Fetches snippets from the server. |hosts| restricts the results to a set of
-  // hosts, e.g. "www.google.com". If |hosts| is empty, no host restrictions are
-  // applied.
-  //
-  // |excluded_ids| will be reported to the server; the server should not return
-  // suggestions with those IDs.
-  //
-  // If an ongoing fetch exists, it will be cancelled and a new one started,
-  // without triggering an additional callback (i.e. not noticeable by
-  // subscriber of SetCallback()).
+  // Initiates a fetch from the server. When done (successfully or not), calls
+  // the subscriber of SetCallback().
   //
-  // Fetches snippets only if the daily quota not exceeded, unless
-  // |interactive_request| is set to true (use only for user-initiated fetches).
-  void FetchSnippetsFromHosts(const std::set<std::string>& hosts,
-                              const std::string& language_code,
-                              const std::set<std::string>& excluded_ids,
-                              int count,
-                              bool interactive_request);
+  // 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);
 
   // Debug string representing the status/result of the last fetch attempt.
   const std::string& last_status() const { return last_status_; }
@@ -137,7 +150,7 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   Personalization personalization() const { return personalization_; }
 
   // Returns the URL endpoint used by the fetcher.
-  GURL fetch_url() const { return fetch_url_; }
+  const GURL& fetch_url() const { return fetch_url_; }
 
   // Does the fetcher use authentication to get personalized results?
   bool UsesAuthentication() const;
@@ -168,29 +181,27 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
     CHROME_CONTENT_SUGGESTIONS_API,
   };
 
-  struct RequestParams {
-    FetchAPI fetch_api;
+  struct RequestBuilder {
+    Params params;
+    FetchAPI fetch_api = CHROME_READER_API;
     std::string obfuscated_gaia_id;
-    bool only_return_personalized_results;
-    std::string user_locale;
-    std::set<std::string> host_restricts;
-    std::set<std::string> excluded_ids;
-    int count_to_fetch;
-    bool interactive_request;
+    bool only_return_personalized_results = false;
     std::string user_class;
-    translate::LanguageModel::LanguageInfo ui_language;
-    translate::LanguageModel::LanguageInfo other_top_language;
+    translate::LanguageModel::LanguageInfo ui_language{"", 0.0f};

[Bernhard Bauer, 2016/10/27 12:13:18]

I thought we fixed that?

[Marc Treib, 2016/10/27 13:14:58]

Almost - https://codereview.chromium.org/2449873003/ is still waiting for an
approval. I'll rebase it on top of this one after this one lands.

+    translate::LanguageModel::LanguageInfo other_top_language{"", 0.0f};
 
-    RequestParams();
-    ~RequestParams();
+    RequestBuilder();
+    RequestBuilder(RequestBuilder&&);
+    ~RequestBuilder();
 
     std::string BuildRequest();
   };
 
+  RequestBuilder MakeRequestBuilder() const;
+
   void FetchSnippetsImpl(const GURL& url,
                          const std::string& auth_header,
                          const std::string& request);
-  void SetUpCommonFetchingParameters(RequestParams* params) const;
   void FetchSnippetsNonAuthenticated();
   void FetchSnippetsAuthenticated(const std::string& account_id,
                                   const std::string& oauth_access_token);
@@ -219,11 +230,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
 
   bool DemandQuotaForRequest(bool interactive_request);
 
-  // Authorization for signed-in users.
+  // Authentication for signed-in users.
   SigninManagerBase* signin_manager_;
   OAuth2TokenService* token_service_;
   std::unique_ptr<OAuth2TokenService::Request> oauth_request_;
   bool waiting_for_refresh_token_;
+  // When a token request gets canceled, we want to retry once.

[Bernhard Bauer, 2016/10/27 12:13:18]

Nit: empty line before comments please :)

[Marc Treib, 2016/10/27 13:14:58]

Done.

+  bool oauth_token_retried_;

[Bernhard Bauer, 2016/10/27 12:13:18]

Initialize to false?

[Marc Treib, 2016/10/27 13:14:58]

It is initialized to false, in the ctor's initializer list, like all the other
members here. I like to keep things consistent :)

That said, we could probably move most of the initialization directly to the
member declarations. Separate CL though.

[Bernhard Bauer, 2016/10/27 13:25:31]

Isn't that what you're doing in a bunch of other places in this CL as well?

[Marc Treib, 2016/10/27 13:31:31]

I've changed the params/builder structs, but not the main class itself - that's
kind of a separate thing to me (e.g. because we won't be able to move *all* the
initialization into the header). But if you feel strongly, I can move the things
that can be moved?

[Bernhard Bauer, 2016/10/27 13:34:58]

Personally, I think that sort of thing would fit in nicely with a cleanup CL
like this, but I don't really feel that strongly.

[Marc Treib, 2016/10/27 13:53:01]

Hm. TBH, I'm not a big fan of splitting initialization like this (half in the
header, half in the ctor initializer list), but maybe it's okay for these "state
bools" (which are ugly anyway...)
I thought the style guide had some guidance on this, but I can't find anything.
Oh well. Done, for these two bools.

 
   // Holds the URL request context.
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
@@ -233,42 +246,18 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   translate::LanguageModel* const language_model_;
 
   const ParseJSONCallback parse_json_callback_;
-  base::TimeTicks fetch_start_time_;
-  std::string last_status_;
-  std::string last_fetch_json_;
-
-  // Hosts to restrict the snippets to.
-  std::set<std::string> hosts_;
-
-  // Snippets to exclude from the results.
-  std::set<std::string> excluded_ids_;
-
-  // Count of snippets to fetch.
-  int count_to_fetch_;
-
-  // Language code to restrict to for personalized results.
-  std::string locale_;
 
   // API endpoint for fetching snippets.
   const GURL fetch_url_;
   // Which API to use
   const FetchAPI fetch_api_;
 
-  // The fetcher for downloading the snippets.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
-
-  // The callback to notify when new snippets get fetched.
-  SnippetsAvailableCallback snippets_available_callback_;
-
   // 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_;
 
-  // Is the request user initiated?
-  bool interactive_request_;
-
   // Allow for an injectable tick clock for testing.
   std::unique_ptr<base::TickClock> tick_clock_;
 
@@ -280,8 +269,22 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   RequestThrottler request_throttler_active_ntp_user_;
   RequestThrottler request_throttler_active_suggestions_consumer_;
 
-  // When a token request gets canceled, we want to retry once.
-  bool oauth_token_retried_;
+  // The callback to notify when new snippets get fetched.
+  SnippetsAvailableCallback snippets_available_callback_;
+
+  // The parameters for the current request.
+  Params params_;
+
+  // The fetcher for downloading the snippets. Only non-null if a fetch is
+  // currently ongoing.
+  std::unique_ptr<net::URLFetcher> url_fetcher_;
+
+  // When the current request was started, for logging purposes.
+  base::TimeTicks fetch_start_time_;
+
+  // Info on the last finished fetch.
+  std::string last_status_;
+  std::string last_fetch_json_;
 
   base::WeakPtrFactory<NTPSnippetsFetcher> weak_ptr_factory_;