[NTP Snippets] Implement snippets expiry

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 50d8883bab478ee0acb7a34475f8ab1235707612..6208e9d0d941186a8300f9a1650eeb23f86cfc8c 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -148,22 +148,32 @@ bool NTPSnippetsService::LoadFromJSONList(const base::ListValue& list) {
     if (!snippet)
       return false;
 
-    // Check if we already have a snippet with the same URL. If so, replace it
-    // rather than adding a duplicate.
+    // Check if we already have a snippet with the same URL.
     const GURL& url = snippet->url();
     auto it = std::find_if(snippets_.begin(), snippets_.end(),
                            [&url](const scoped_ptr<NTPSnippet>& old_snippet) {
       return old_snippet->url() == url;
     });
-    if (it != snippets_.end())
-      *it = std::move(snippet);
-    else
+    if (it == snippets_.end()) {
+      // If the snippet has no publish/expiry dates, fill in defaults.
+      if (snippet->publish_date().is_null())
+        snippet->set_publish_date(base::Time::Now());
+      if (snippet->expiry_date().is_null()) {
+        snippet->set_expiry_date(
+            snippet->publish_date() +
+            base::TimeDelta::FromSeconds(2 * kDefaultFetchingIntervalSeconds));
+      }
+
       snippets_.push_back(std::move(snippet));
+    }
   }
   loaded_ = true;
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded(this));
+
+  ScheduleExpiryTimer();

[Bernhard Bauer, 2016/03/01 11:05:04]

Do we also want to expire old items, if this is called at startup?

[Marc Treib, 2016/03/01 11:11:39]

Haha, I just uploaded a new patch set that does that!

+
   return true;
 }
 
@@ -188,4 +198,34 @@ void NTPSnippetsService::StoreToPrefs() {
   pref_service_->Set(prefs::kSnippets, list);
 }
 
+void NTPSnippetsService::ScheduleExpiryTimer() {
+  if (snippets_.empty())
+    return;
+
+  base::Time next_expiry = base::Time::Max();
+  for (const auto& snippet : snippets_) {

[Bernhard Bauer, 2016/03/01 11:05:04]

You could go all-out here as well, and use std::min_element :)

[Marc Treib, 2016/03/01 11:22:45]

Hm, I could, but then I'd have to define a comparator lambda and touch iterators
(eewww). It's actually more code and IMO harder to read than the loop.

[Bernhard Bauer, 2016/03/01 11:54:27]

Well, you already use a predicate lambda below... hence "all-out" :)

[Marc Treib, 2016/03/01 13:16:49]

The code with min_element would look like this:

  auto min_it = std::min_element(
      snippets_.begin(), snippets_.end(),
      [](const scoped_ptr<NTPSnippet>& lhs, const scoped_ptr<NTPSnippet>& rhs) {
        return lhs->expiry_date() < rhs->expiry_date();
      });
  base::Time next_expiry = (*min_it)->expiry_date();

I prefer what's there now TBH, but if you insist...? :)

[Bernhard Bauer, 2016/03/01 15:13:18]

Eh, I don't think it's worse than what we currently do to expire the snippets
(calling a method with a lambda that returns an iterator, and then acting on
that iterator), but OTOH the code as currently written is pretty concise, and we
can't really do the expiration this way in an equally concises manner, so I
think this is fine.

+    if (snippet->expiry_date() < next_expiry)
+      next_expiry = snippet->expiry_date();
+  }
+  // Note: If |next_expiry| is in the past, this will run immediately.
+  expiry_timer_.Start(FROM_HERE, next_expiry - base::Time::Now(),
+                      base::Bind(&NTPSnippetsService::RemoveExpiredSnippets,
+                                 base::Unretained(this)));
+}
+
+void NTPSnippetsService::RemoveExpiredSnippets() {
+  base::Time expiry = base::Time::Now();
+  snippets_.erase(

[Bernhard Bauer, 2016/03/01 11:05:04]

This only erases a single element, but remove_if can potentially remove more
than one element, no?

(It would also be nice if this method could deal with no snippet being expired
yet, so we could call it in more places, e.g. at startup.)

[Marc Treib, 2016/03/01 11:11:39]

Nope, this erases all expired elements. std::remove_if is weird - it returns the
new end iterator. The erase() call around it then actually removes everything
between the new end and the old end.
Hmm, what do you mean? If nothing is expired, then nothing will be removed.

[Bernhard Bauer, 2016/03/01 11:54:27]

Oh, your indentation on line 223 is misleading -- it's the second argument to
the top-level erase() call, but it looks like an argument to remove_if(). That's
why I thought it's the single-argument form of erase().
NM, that was under the assumption that you'd remove a single element (see
above).

[Marc Treib, 2016/03/01 13:16:49]

Fixed the indentation.

+      std::remove_if(snippets_.begin(), snippets_.end(),
+                     [&expiry](const scoped_ptr<NTPSnippet>& snippet) {
+                       return snippet->expiry_date() <= expiry;
+                     }),
+                     snippets_.end());
+
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceLoaded(this));
+
+  ScheduleExpiryTimer();
+}
+
 }  // namespace ntp_snippets