[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..4142d45ac89df357a45931ece86c1da4f2a0c209 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_);

[mastiz, 2017/03/03 11:13:15]

According to my earlier comment, you might want a DCHECK for expiry_timer_;

[Marc Treib, 2017/03/03 13:01:48]

Done.

 }
 
@@ -35,15 +36,39 @@ void DoodleService::DoodleFetched(
     DoodleState state,
     base::TimeDelta time_to_live,
     const base::Optional<DoodleConfig>& doodle_config) {
-  // 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.
-    return;
+  // 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;

[mastiz, 2017/03/03 11:13:15]

This seems legit but I'm always scared when references and ternary operators are
combined, since it's easy to screw up if either of the two expressions results
in a non-reference type. Please double check that tests cover both scenarios.

[Marc Treib, 2017/03/03 13:01:48]

Yup, I have an explicit test for the already-expired case.

+
+  // If the config changed, update our cache and notify observers.
+  // Note that this checks both for existence changes as well as changes of the
+  // configs themselves.
+  if (cached_config_ != new_config) {
+    cached_config_ = new_config;
+    for (auto& observer : observers_) {
+      observer.OnDoodleConfigUpdated(cached_config_);
+    }
+  }
+
+  // Even if the configs are identical, the time-to-live might have changed.
+  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();
   }
+}
 
-  // Update the cache and notify observers.
-  cached_config_ = doodle_config;
+void DoodleService::DoodleExpired() {
+  DCHECK(cached_config_.has_value());
+  cached_config_.reset();
   for (auto& observer : observers_) {
     observer.OnDoodleConfigUpdated(cached_config_);
   }