[Doodle] Replace the expiry_date in DoodleConfig by time_to_live

components/doodle/doodle_fetcher_impl.cc

 // 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.
 
 #include "components/doodle/doodle_fetcher_impl.h"
 
 #include <utility>
 
 #include "base/strings/string_number_conversions.h"
-#include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/google/core/browser/google_url_tracker.h"
 #include "components/google/core/browser/google_util.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
 
 using net::URLFetcher;
@@ skipping 41 matching lines @@
   }
   return DoodleType::UNKNOWN;
 }
 
 }  // namespace
 
 DoodleImage::DoodleImage()
     : height(0), width(0), is_animated_gif(false), is_cta(false) {}
 DoodleImage::~DoodleImage() = default;
 
 DoodleConfig::DoodleConfig() : doodle_type(DoodleType::UNKNOWN) {}

[fhorschig, 2017/03/01 15:40:52]

Do we want the default TTL to be 0?
Alternatives would be +/- TimeDelta::Max();

-TimeDelta::Max() is something I would like
because you could see that the config isn't
valid.

[Marc Treib, 2017/03/02 09:28:02]

Hm. Max() I don't like, because then we'd have a config that never expires.
-Max() is more reasonable, but it also seems a bit magic, in the sense that it's
not clear why this exact value would correspond to "invalid". It doesn't seem to
be a common convention.

TBH, "expires right now" seems like a reasonable default for the uninitialized
case to me. Kinda similar to how we set the image sizes to 0 rather than
INT_MIN. So unless you insist, I'd keep it as is.

 DoodleConfig::DoodleConfig(const DoodleConfig& config) = default;
 DoodleConfig::~DoodleConfig() = default;
 
 DoodleFetcherImpl::DoodleFetcherImpl(
     scoped_refptr<net::URLRequestContextGetter> download_context,
     GoogleURLTracker* google_url_tracker,
     const ParseJSONCallback& json_parsing_callback)
     : download_context_(download_context),
       json_parsing_callback_(json_parsing_callback),
       google_url_tracker_(google_url_tracker),
-      clock_(new base::DefaultClock()),
       weak_ptr_factory_(this) {
   DCHECK(google_url_tracker_);
 }
 
 DoodleFetcherImpl::~DoodleFetcherImpl() = default;
 
 void DoodleFetcherImpl::FetchDoodle(FinishedCallback callback) {
   if (IsFetchInProgress()) {
     callbacks_.push_back(std::move(callback));
     return;  // The callback will be called for the existing request's results.
@@ skipping 104 matching lines @@
   config->fullpage_interactive_url =
       ParseRelativeUrl(ddljson, "fullpage_interactive_url");
 
   config->doodle_type = ParseDoodleType(ddljson);
   ddljson.GetString("alt_text", &config->alt_text);
   ddljson.GetString("interactive_html", &config->interactive_html);
 
   // The JSON doesn't guarantee the number to fit into an int.
   double ttl = 0;  // Expires immediately if the parameter is missing.
   if (!ddljson.GetDouble("time_to_live_ms", &ttl) || ttl < 0) {
-    DLOG(WARNING) << "No valid Doodle image TTL present in ddljson!";
+    DLOG(WARNING) << "No valid Doodle TTL present in ddljson!";
     ttl = 0;
   }
+  // TODO(treib,fhorschig): Should this logic live in the service instead?

[fhorschig, 2017/03/01 15:40:52]

In my opinion absolutely. The clamping was mainly a requirement
for having an expiry time as we didn't want to pass the raw TTL.

If there is no opposing view, you can drop the question mark or
implement it in the service. 
(TODO would be fine with me so we can split the CLs.)

[Marc Treib, 2017/03/02 09:28:02]

Reformulated without a question :)

   if (ttl > kMaxTimeToLiveMS) {
     ttl = kMaxTimeToLiveMS;
-    DLOG(WARNING) << "Clamping Doodle image TTL to 30 days!";
+    DLOG(WARNING) << "Clamping Doodle TTL to 30 days!";
   }
-  config->expiry_date = clock_->Now() + base::TimeDelta::FromMillisecondsD(ttl);
+  config->time_to_live = base::TimeDelta::FromMillisecondsD(ttl);
 }
 
 GURL DoodleFetcherImpl::ParseRelativeUrl(
     const base::DictionaryValue& dict_value,
     const std::string& key) const {
   std::string str_url;
   dict_value.GetString(key, &str_url);
   if (str_url.empty()) {
     return GURL();
   }
@@ skipping 11 matching lines @@
 
 GURL DoodleFetcherImpl::GetGoogleBaseUrl() const {
   GURL cmd_line_url = google_util::CommandLineGoogleBaseURL();
   if (cmd_line_url.is_valid()) {
     return cmd_line_url;
   }
   return google_url_tracker_->google_url();
 }
 
 }  // namespace doodle