[Doodle] Automatically expire Doodles after their TTL

components/doodle/doodle_service.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_service.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/time/time.h"
 
 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.

 }
 
 DoodleService::~DoodleService() = default;
 
 void DoodleService::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void DoodleService::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 void DoodleService::Refresh() {
   fetcher_->FetchDoodle(
       base::BindOnce(&DoodleService::DoodleFetched, base::Unretained(this)));
 }
 
 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_);
+    }
   }
 
-  // Update the cache and notify observers.
-  cached_config_ = doodle_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();
+  }
+}
+
+void DoodleService::DoodleExpired() {
+  DCHECK(cached_config_.has_value());
+  cached_config_.reset();
   for (auto& observer : observers_) {
     observer.OnDoodleConfigUpdated(cached_config_);
   }
 }
 
 }  // namespace doodle