[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_);
 }
 
 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) {
+  // 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_);
+  }
 }
 
 }  // namespace doodle