[NTP Snippets] Implement snippets expiry

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 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/ntp_snippets/ntp_snippets_service.h"
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
@@ skipping 130 matching lines @@
     if (!value->GetAsDictionary(&dict))
       return false;
 
     const base::DictionaryValue* content = nullptr;
     if (!dict->GetDictionary(kContentInfo, &content))
       return false;
     scoped_ptr<NTPSnippet> snippet = NTPSnippet::CreateFromDictionary(*content);
     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;
 }
 
 void NTPSnippetsService::LoadFromPrefs() {
   // |pref_service_| can be null in tests.
   if (!pref_service_)
     return;
   if (!LoadFromJSONList(*pref_service_->GetList(prefs::kSnippets)))
     LOG(ERROR) << "Failed to parse snippets from prefs";
 }
 
 void NTPSnippetsService::StoreToPrefs() {
   // |pref_service_| can be null in tests.
   if (!pref_service_)
     return;
   base::ListValue list;
   for (const auto& snippet : snippets_) {
     scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
     dict->Set(kContentInfo, snippet->ToDictionary());
     list.Append(std::move(dict));
   }
   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