[NTP Snippets] Clean up NTPSnippetsFetcher

components/ntp_snippets/remote/ntp_snippets_fetcher.h

 // Copyright 2016 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_REMOTE_NTP_SNIPPETS_FETCHER_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_
 
 #include <memory>
 #include <set>
 #include <string>
@@ skipping 72 matching lines @@
     RESULT_MAX
   };
 
   // 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();
+
+    // 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,
       scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
       PrefService* pref_service,
       CategoryFactory* category_factory,
       translate::LanguageModel* language_model,
       const ParseJSONCallback& parse_json_callback,
       const std::string& api_key,
       const UserClassifier* user_classifier);
   ~NTPSnippetsFetcher() override;
 
   // Set a callback that is called when a new set of snippets are downloaded,
   // 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.
+  // Initiates a fetch from the server. When done (successfully or not), calls
+  // the subscriber of SetCallback().
   //
-  // |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()).
-  //
-  // 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_; }
 
   // Returns the last JSON fetched from the server.
   const std::string& last_json() const {
     return last_fetch_json_;
   }
 
   // Returns the personalization setting of the fetcher.
   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;
 
   // Overrides internal clock for testing purposes.
   void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {
     tick_clock_ = std::move(tick_clock);
   }
 
   void SetPersonalizationForTesting(Personalization personalization) {
@@ skipping 10 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithUILanguageOnly);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithOtherLanguageOnly);
 
   enum FetchAPI {
     CHROME_READER_API,
     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);
   void StartTokenRequest();
 
   // OAuth2TokenService::Consumer overrides:
   void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
                          const std::string& access_token,
                          const base::Time& expiration_time) override;
   void OnGetTokenFailure(const OAuth2TokenService::Request* request,
                          const GoogleServiceAuthError& error) override;
 
   // OAuth2TokenService::Observer overrides:
   void OnRefreshTokenAvailable(const std::string& account_id) override;
 
   // URLFetcherDelegate implementation.
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
   bool JsonToSnippets(const base::Value& parsed,
                       FetchedCategoriesVector* categories);
   void OnJsonParsed(std::unique_ptr<base::Value> parsed);
   void OnJsonError(const std::string& error);
   void FetchFinished(OptionalFetchedCategories fetched_categories,
                      FetchResult result,
                      const std::string& extra_message);
 
   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_;
 
   // Weak references, not owned.
   CategoryFactory* const category_factory_;
   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_;
 
   // Classifier that tells us how active the user is. Not owned.
   const UserClassifier* user_classifier_;
 
   // Request throttlers for limiting requests for different classes of users.
   RequestThrottler request_throttler_rare_ntp_user_;
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsFetcher);
 };
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_