[Doodle] Automatically expire Doodles after their TTL

components/doodle/doodle_service.cc

Index: components/doodle/doodle_service.cc
diff --git a/components/doodle/doodle_service.cc b/components/doodle/doodle_service.cc
index c5cb9137b253169c162b6b26898d5249f11512f5..1933b34e0e8cd45c0e7be9b2e2a3dc2d7a99d40b 100644
--- a/components/doodle/doodle_service.cc
+++ b/components/doodle/doodle_service.cc
@@ -11,8 +11,9 @@
 
 namespace doodle {
 
-DoodleService::DoodleService(std::unique_ptr<DoodleFetcher> fetcher)
-    : fetcher_(std::move(fetcher)) {
+DoodleService::DoodleService(std::unique_ptr<DoodleFetcher> fetcher,
+                             std::unique_ptr<base::OneShotTimer> expiry_timer)
+    : fetcher_(std::move(fetcher)), expiry_timer_(std::move(expiry_timer)) {
   DCHECK(fetcher_);
 }
 
@@ -35,15 +36,42 @@ void DoodleService::DoodleFetched(
     DoodleState state,
     base::TimeDelta time_to_live,
     const base::Optional<DoodleConfig>& doodle_config) {
+  // Handle the case where the new config is already expired.
+  bool expired = time_to_live <= base::TimeDelta();
+  const base::Optional<DoodleConfig>& new_config =
+      expired ? base::nullopt : doodle_config;

[fhorschig, 2017/03/02 20:12:59]

nit: A reference isn't expensive, so why no go for a readable if?

const base::Optional<DoodleConfig>& new_config = base::nullopt;
if (time_to_live <= base::TimeDelta()) {  // New config is already expired.
  new_config = doodle_config;
}

[Marc Treib, 2017/03/03 10:06:57]

This won't work; you can't assign to a reference. They must be initialized and
then can't be changed.

[fhorschig, 2017/03/03 10:45:07]

Oh, sorry.

+
   // If nothing changed, then there's nothing to do. Note that this checks both
   // for existence changes as well as changes of the configs themselves.
-  if (cached_config_ == doodle_config) {
-    // TODO(treib): Once we support expiry, update the time to live here.
+  if (cached_config_ == new_config) {
+    // Even if the configs are identical, the time-to-live might have changed.
+    UpdateTimeToLive(time_to_live);

[fhorschig, 2017/03/02 20:12:59]

The reason you do it twice instead of earlier is because the timer 
could expire before the observers are notified? 
(I would be fine with that)

[Marc Treib, 2017/03/03 10:06:57]

No, the reason was so that I could early-out if the config hasn't changed
(without notifying).
I've restructured this method so that this call happens in only one place.

     return;
   }
 
   // Update the cache and notify observers.
-  cached_config_ = doodle_config;
+  cached_config_ = new_config;
+  for (auto& observer : observers_) {
+    observer.OnDoodleConfigUpdated(cached_config_);
+  }
+
+  UpdateTimeToLive(time_to_live);
+}
+
+void DoodleService::UpdateTimeToLive(base::TimeDelta time_to_live) {
+  // (Re-)schedule the cache expiry.
+  if (cached_config_.has_value()) {
+    expiry_timer_->Start(
+        FROM_HERE, time_to_live,
+        base::Bind(&DoodleService::DoodleExpired, base::Unretained(this)));
+  } else {
+    expiry_timer_->Stop();
+  }
+}
+
+void DoodleService::DoodleExpired() {
+  DCHECK(cached_config_.has_value());
+  cached_config_.reset();
   for (auto& observer : observers_) {
     observer.OnDoodleConfigUpdated(cached_config_);
   }