[Doodle] Pull time_to_live out of DoodleConfig

components/doodle/doodle_fetcher_impl.h

Index: components/doodle/doodle_fetcher_impl.h
diff --git a/components/doodle/doodle_fetcher_impl.h b/components/doodle/doodle_fetcher_impl.h
index 3f3eb19aeaa8042f8ce42b174c448f22f96fff2a..24c32a6ab5b44f727581c43cb5484eaf2f1e3589 100644
--- a/components/doodle/doodle_fetcher_impl.h
+++ b/components/doodle/doodle_fetcher_impl.h
@@ -25,6 +25,7 @@ class GoogleURLTracker;
 
 namespace base {
 class DictionaryValue;
+class TimeDelta;
 class Value;
 }
 
@@ -62,17 +63,19 @@ class DoodleFetcherImpl : public DoodleFetcher, public net::URLFetcherDelegate {
   // ParseJSONCallback Failure callback
   void OnJsonParseFailed(const std::string& error_message);
 
-  base::Optional<DoodleConfig> ParseDoodle(
-      const base::DictionaryValue& ddljson) const;
+  base::Optional<DoodleConfig> ParseDoodle(const base::DictionaryValue& ddljson,
+                                           base::TimeDelta* time_to_live) const;

[mastiz, 2017/03/02 19:55:42]

Optional: wouldn't it be cleaner to return a Optional<pair<DoodleConfig,
TimeDelta>> and possibly rename to ParseDoodleConfigAndTimeToLive? 

[Marc Treib, 2017/03/03 09:35:26]

Renamed.

I don't like std::pair much - IMO it often makes things hard to read. A struct
would be an option, but TBH I don't think it's worth the hassle of
packing/unpacking and std::move'ing the DoodleConfig itself...

   bool ParseImage(const base::DictionaryValue& image_dict,
                   const std::string& image_name,
                   DoodleImage* image) const;
   void ParseBaseInformation(const base::DictionaryValue& ddljson,
-                            DoodleConfig* config) const;
+                            DoodleConfig* config,
+                            base::TimeDelta* time_to_live) const;
   GURL ParseRelativeUrl(const base::DictionaryValue& dict_value,
                         const std::string& key) const;
 
   void RespondToAllCallbacks(DoodleState state,
+                             base::TimeDelta time_to_live,
                              const base::Optional<DoodleConfig>& config);

[mastiz, 2017/03/02 19:55:42]

Ditto: perhaps a typedef or struct could also help.

[Marc Treib, 2017/03/03 09:35:26]

Same: Unless you feel strongly, I don't think it's worth it.

[mastiz, 2017/03/03 11:17:44]

I was certainly not feeling strongly, so it's fine. What I dislike about the
current signature is that it's not clear that the function returns both or
neither.

   GURL GetGoogleBaseUrl() const;