[NTP Snippets] Persist snippets in a LevelDB instead of prefs

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 4da4e50a3ddeb8950cafb65e3d8ba89a28e7d58b..617d06ae6aed64a7b3318c3ad3204b084fe3a68f 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -21,6 +21,7 @@
 #include "base/values.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
+#include "components/ntp_snippets/ntp_snippets_database.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -157,16 +158,6 @@ std::set<std::string> GetSuggestionsHostsImpl(
   return hosts;
 }
 
-std::unique_ptr<base::ListValue> SnippetsToListValue(
-    const NTPSnippet::PtrVector& snippets) {
-  std::unique_ptr<base::ListValue> list(new base::ListValue);
-  for (const auto& snippet : snippets) {
-    std::unique_ptr<base::DictionaryValue> dict = snippet->ToDictionary();
-    list->Append(std::move(dict));
-  }
-  return list;
-}
-
 void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
                   std::set<std::string>* ids) {
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
@@ -176,6 +167,14 @@ void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
   }
 }
 
+void EraseNullptrs(NTPSnippet::PtrVector* snippets) {

[Bernhard Bauer, 2016/05/25 15:01:15]

Nit: this is very often called "compact".

[Marc Treib, 2016/05/27 14:03:12]

Hm, not sure if I've heard that before. But okay, done.

+  snippets->erase(
+      std::remove_if(
+          snippets->begin(), snippets->end(),
+          [](const std::unique_ptr<NTPSnippet>& snippet) { return !snippet; }),
+      snippets->end());
+}
+
 void WrapImageFetchedCallback(
     const NTPSnippetsService::ImageFetchedCallback& callback,
     const GURL& snippet_id_url,
@@ -189,22 +188,22 @@ NTPSnippetsService::NTPSnippetsService(
     PrefService* pref_service,
     sync_driver::SyncService* sync_service,
     SuggestionsService* suggestions_service,
-    scoped_refptr<base::SequencedTaskRunner> file_task_runner,
     const std::string& application_language_code,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
-    std::unique_ptr<ImageFetcher> image_fetcher)
+    std::unique_ptr<ImageFetcher> image_fetcher,
+    std::unique_ptr<NTPSnippetsDatabase> database)
     : state_(State::NOT_INITED),
       enabled_(false),
       pref_service_(pref_service),
       sync_service_(sync_service),
       sync_service_observer_(this),
       suggestions_service_(suggestions_service),
-      file_task_runner_(file_task_runner),
       application_language_code_(application_language_code),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
-      image_fetcher_(std::move(image_fetcher)) {
+      image_fetcher_(std::move(image_fetcher)),
+      database_(std::move(database)) {
   snippets_fetcher_->SetCallback(base::Bind(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 
@@ -230,31 +229,20 @@ void NTPSnippetsService::Init(bool enabled) {
 
   enabled_ = enabled;
   if (enabled_) {
-    // |suggestions_service_| can be null in tests.
-    if (snippets_fetcher_->UsesHostRestrictions() && suggestions_service_) {
-      suggestions_service_subscription_ = suggestions_service_->AddCallback(
-          base::Bind(&NTPSnippetsService::OnSuggestionsChanged,
-                     base::Unretained(this)));
-    }
-
-    // Get any existing snippets immediately from prefs.
-    LoadDiscardedSnippetsFromPrefs();
-    LoadSnippetsFromPrefs();
-
-    // If we don't have any snippets yet, start a fetch.
-    if (snippets_.empty())
-      FetchSnippets();
+    database_->Load(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
+                               base::Unretained(this)));
   }
 
   RescheduleFetching();
 }
 
 void NTPSnippetsService::Shutdown() {
-  DCHECK(state_ == State::INITED);
+  DCHECK(state_ == State::INITED || state_ == State::LOADED);
   state_ = State::SHUT_DOWN;
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceShutdown());
+  expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
   enabled_ = false;
 }
@@ -265,6 +253,8 @@ void NTPSnippetsService::FetchSnippets() {
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
+  if (!loaded())
+    return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount);
 }
@@ -308,9 +298,11 @@ void NTPSnippetsService::FetchSnippetImage(
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  snippets_.clear();
+  if (!loaded())

[Bernhard Bauer, 2016/05/25 15:01:15]

Do we really want to silently do nothing in this case? Would it be better to
expose this flag, and require that callers catch that case?

[Marc Treib, 2016/05/27 14:03:12]

That seems excessively cumbersome...
IMO ignoring the "clear" is fine: If we're not loaded, then there aren't any
snippets, so the clear is a no-op.
We don't cancel any pending fetch here, so it's possible anyway that new
snippets appear soon after.

Ignoring the "fetch" seems problematic though: We'll probably attempt background
fetches before the DB is loaded. WDYT about a "bool fetch_after_load_"?

[Bernhard Bauer, 2016/05/27 16:06:42]

Well, what about when the load comes back? If the idea is that we clear snippets
e.g. when signing out, we might keep the snippets if signout happens before they
have been loaded.
That might be a separate bug?
Yup, that seems reasonable, although see two paragraphs up: maybe we'll need to
queue a bunch of actions?

[Marc Treib, 2016/05/30 13:22:00]

Well, it depends on what exactly we want "clear" to mean. Since it's basically a
debug feature (it's not exposed anywhere except for snippets-internals), IMO
this behavior is fine.
For the signout case, clearing the snippets isn't enough anyway, even if we'd
wait until after the load, and that whole case will be completely revamped by
Nicolas' CL soon.
Possibly :) But as above, probably better handled by Nicolas' CL.
(That CL is one reason why I'm trying to land this quickly...)
fetch_after_load_ done.

+    return;
 
-  StoreSnippetsToPrefs();
+  database_->Delete(snippets_);
+  snippets_.clear();
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
@@ -321,12 +313,15 @@ std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
   if (!suggestions_service_)
     return std::set<std::string>();
 
-  // TODO(treib) this should just call GetSnippetHostsFromPrefs
+  // TODO(treib): This should just call GetSnippetHostsFromPrefs.
   return GetSuggestionsHostsImpl(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
+  if (!loaded())
+    return false;
+
   auto it =
       std::find_if(snippets_.begin(), snippets_.end(),
                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
@@ -334,18 +329,26 @@ bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
                    });
   if (it == snippets_.end())
     return false;
+
+  (*it)->set_discarded(true);
+
+  database_->Save(**it);
+
   discarded_snippets_.push_back(std::move(*it));
   snippets_.erase(it);
-  StoreDiscardedSnippetsToPrefs();
-  StoreSnippetsToPrefs();
+
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
   return true;
 }
 
 void NTPSnippetsService::ClearDiscardedSnippets() {
+  if (!loaded())
+    return;
+
+  database_->Delete(discarded_snippets_);
   discarded_snippets_.clear();
-  StoreDiscardedSnippetsToPrefs();
+
   FetchSnippets();
 }
 
@@ -380,8 +383,44 @@ void NTPSnippetsService::OnStateChanged() {
   FetchSnippets();
 }
 
+void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
+  DCHECK(state_ == State::INITED || state_ == State::SHUT_DOWN);
+  if (state_ == State::SHUT_DOWN)
+    return;
+  state_ = State::LOADED;
+
+  DCHECK(snippets_.empty());
+  DCHECK(discarded_snippets_.empty());
+  for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
+    if (snippet->is_discarded())
+      discarded_snippets_.emplace_back(std::move(snippet));
+    else
+      snippets_.emplace_back(std::move(snippet));
+  }
+  std::sort(snippets_.begin(), snippets_.end(),
+            [](const std::unique_ptr<NTPSnippet>& lhs,
+               const std::unique_ptr<NTPSnippet>& rhs) {
+              return lhs->score() > rhs->score();
+            });
+  LoadingSnippetsFinished();
+
+  // If host restrictions are enabled, register for host list updates.
+  // |suggestions_service_| can be null in tests.
+  if (snippets_fetcher_->UsesHostRestrictions() && suggestions_service_) {
+    suggestions_service_subscription_ =
+        suggestions_service_->AddCallback(base::Bind(
+            &NTPSnippetsService::OnSuggestionsChanged, base::Unretained(this)));
+  }
+
+  // If we don't have any snippets yet, start a fetch.
+  if (snippets_.empty())
+    FetchSnippets();
+}
+
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
+  DCHECK(loaded());
+
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
     return;
@@ -389,14 +428,16 @@ void NTPSnippetsService::OnSuggestionsChanged(
   // Remove existing snippets that aren't in the suggestions anymore.
   // TODO(treib,maybelle): If there is another source with an allowed host,
   // then we should fall back to that.
-  snippets_.erase(
-      std::remove_if(snippets_.begin(), snippets_.end(),
-                     [&hosts](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return !hosts.count(snippet->best_source().url.host());
-                     }),
-      snippets_.end());
-
-  StoreSnippetsToPrefs();
+  // First, move them over into |to_delete|.
+  NTPSnippet::PtrVector to_delete;
+  for (std::unique_ptr<NTPSnippet>& snippet : snippets_) {
+    if (!hosts.count(snippet->best_source().url.host()))
+      to_delete.emplace_back(std::move(snippet));
+  }
+  EraseNullptrs(&snippets_);
+  // Then delete the removed snippets from the database.
+  database_->Delete(to_delete);
+
   StoreSnippetHostsToPrefs(hosts);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
@@ -407,6 +448,9 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
+  if (!loaded())
+    return;
+
   if (snippets) {
     // Sparse histogram used because the number of snippets is small (bound by
     // kMaxSnippetCount).
@@ -419,6 +463,8 @@ void NTPSnippetsService::OnFetchFinished(
 }
 
 void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
+  DCHECK(loaded());
+
   // Remove new snippets that we already have, or that have been discarded.
   std::set<std::string> old_snippet_ids;
   InsertAllIDs(discarded_snippets_, &old_snippet_ids);
@@ -469,37 +515,16 @@ void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
                                   num_snippets_discarded);
     }
   }
+
+  // Save the new snippets to the DB.
+  database_->Save(new_snippets);
+
   // Insert the new snippets at the front.
   snippets_.insert(snippets_.begin(),
                    std::make_move_iterator(new_snippets.begin()),
                    std::make_move_iterator(new_snippets.end()));
 }
 
-void NTPSnippetsService::LoadSnippetsFromPrefs() {
-  NTPSnippet::PtrVector prefs_snippets;
-  bool success = NTPSnippet::AddFromListValue(
-      *pref_service_->GetList(prefs::kSnippets), &prefs_snippets);
-  DCHECK(success) << "Failed to parse snippets from prefs";
-  MergeSnippets(std::move(prefs_snippets));
-  LoadingSnippetsFinished();
-}
-
-void NTPSnippetsService::StoreSnippetsToPrefs() {
-  pref_service_->Set(prefs::kSnippets, *SnippetsToListValue(snippets_));
-}
-
-void NTPSnippetsService::LoadDiscardedSnippetsFromPrefs() {
-  discarded_snippets_.clear();
-  bool success = NTPSnippet::AddFromListValue(
-      *pref_service_->GetList(prefs::kDiscardedSnippets), &discarded_snippets_);
-  DCHECK(success) << "Failed to parse discarded snippets from prefs";
-}
-
-void NTPSnippetsService::StoreDiscardedSnippetsToPrefs() {
-  pref_service_->Set(prefs::kDiscardedSnippets,
-                     *SnippetsToListValue(discarded_snippets_));
-}
-
 std::set<std::string> NTPSnippetsService::GetSnippetHostsFromPrefs() const {
   std::set<std::string> hosts;
   const base::ListValue* list = pref_service_->GetList(prefs::kSnippetHosts);
@@ -521,30 +546,38 @@ void NTPSnippetsService::StoreSnippetHostsToPrefs(
 }
 
 void NTPSnippetsService::LoadingSnippetsFinished() {
+  DCHECK(loaded());
+
   // Remove expired snippets.
   base::Time expiry = base::Time::Now();
 
-  snippets_.erase(
-      std::remove_if(snippets_.begin(), snippets_.end(),
-                     [&expiry](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return snippet->expiry_date() <= expiry;
-                     }),
-      snippets_.end());
-
-  // If there are more snippets now than we want to show, drop the extra ones
-  // from the end of the list.
-  if (snippets_.size() > kMaxSnippetCount)
+  // Move expired snippets over into |to_delete|.
+  NTPSnippet::PtrVector to_delete;
+  for (std::unique_ptr<NTPSnippet>& snippet : snippets_) {
+    if (snippet->expiry_date() <= expiry)
+      to_delete.emplace_back(std::move(snippet));
+  }
+  EraseNullptrs(&snippets_);
+
+  // If there are still more snippets than we want to show, move the extra ones
+  // over into |to_delete| as well.
+  if (snippets_.size() > kMaxSnippetCount) {
+    to_delete.insert(
+        to_delete.end(),
+        std::make_move_iterator(snippets_.begin() + kMaxSnippetCount),
+        std::make_move_iterator(snippets_.end()));
     snippets_.resize(kMaxSnippetCount);
+  }
 
-  StoreSnippetsToPrefs();
+  // Move expired discarded snippets over into |to_delete| as well.
+  for (std::unique_ptr<NTPSnippet>& snippet : discarded_snippets_) {
+    if (snippet->expiry_date() <= expiry)
+      to_delete.emplace_back(std::move(snippet));
+  }
+  EraseNullptrs(&discarded_snippets_);
 
-  discarded_snippets_.erase(
-      std::remove_if(discarded_snippets_.begin(), discarded_snippets_.end(),
-                     [&expiry](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return snippet->expiry_date() <= expiry;
-                     }),
-      discarded_snippets_.end());
-  StoreDiscardedSnippetsToPrefs();
+  // Finally, actually delete the removed snippets from the DB.
+  database_->Delete(to_delete);
 
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               snippets_.size());