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

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 <algorithm>
 #include <iterator>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #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"
 #include "components/prefs/pref_service.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "components/sync_driver/sync_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "ui/gfx/image/image.h"
 
 using image_fetcher::ImageFetcher;
@@ skipping 116 matching lines @@
   std::set<std::string> hosts;
   for (int i = 0; i < suggestions.suggestions_size(); ++i) {
     const ChromeSuggestion& suggestion = suggestions.suggestions(i);
     GURL url(suggestion.url());
     if (url.is_valid())
       hosts.insert(url.host());
   }
   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) {
     ids->insert(snippet->id());
     for (const SnippetSource& source : snippet->sources())
       ids->insert(source.url.spec());
   }
 }
 
+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,
     const gfx::Image& image) {
   callback.Run(snippet_id_url.spec(), image);
 }
 
 }  // namespace
 
 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)));
 
   // |sync_service_| can be null in tests or if sync is disabled.
   if (sync_service_)
     sync_service_observer_.Add(sync_service_);
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
   DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN);
 }
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippets);
   registry->RegisterListPref(prefs::kDiscardedSnippets);

[Marc Treib, 2016/05/25 09:49:49]

Ah, one thing I forgot to mention: These two prefs are now unused. Do you know
if it's okay to just remove them, or we need cleanup code to remove existing
values first?

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

I think given that this hasn't launched to production yet, it's fine to just
drop the prefs without cleaning them up.

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

But it might launch in this form, at least for some percentage of the Beta
population. So my question was more "will non-registered prefs get cleaned up
automagically", and I guess the answer to that is "no".
In that case, I think we should have cleanup code. WDYT?

   registry->RegisterListPref(prefs::kSnippetHosts);
 }
 
 void NTPSnippetsService::Init(bool enabled) {
   DCHECK(state_ == State::NOT_INITED);
   state_ = State::INITED;
 
   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;
 }
 
 void NTPSnippetsService::FetchSnippets() {
   FetchSnippetsFromHosts(GetSuggestionsHosts());
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
+  if (!loaded())
+    return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount);
 }
 
 void NTPSnippetsService::RescheduleFetching() {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
   if (enabled_) {
@@ skipping 23 matching lines @@
   const NTPSnippet& snippet = *it->get();
   // TODO(treib): Make ImageFetcher take a string instead of a GURL as an
   // identifier.
   image_fetcher_->StartOrQueueNetworkRequest(
       GURL(snippet.id()), snippet.salient_image_url(),
       base::Bind(WrapImageFetchedCallback, callback));
   // TODO(treib): Cache/persist the snippet image.
 }
 
 void NTPSnippetsService::ClearSnippets() {
+  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;
+
+  database_->Delete(snippets_);
   snippets_.clear();
 
-  StoreSnippetsToPrefs();
-
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 }
 
 std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
   // |suggestions_service_| can be null in tests.
   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) {
                      return snippet->id() == 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();
 }
 
 void NTPSnippetsService::AddObserver(NTPSnippetsServiceObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
@@ skipping 14 matching lines @@
                       NTPSnippetsServiceDisabled());
     return;
   }
 
   // TODO(dgn): When the data sources change, we may want to not fetch here,
   // as we will get notified of changes from the snippet sources as well, and
   // start multiple fetches.
   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;
 
   // 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());
+  // 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);
 
-  StoreSnippetsToPrefs();
   StoreSnippetHostsToPrefs(hosts);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
   FetchSnippetsFromHosts(hosts);
 }
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
+  if (!loaded())
+    return;
+
   if (snippets) {
     // Sparse histogram used because the number of snippets is small (bound by
     // kMaxSnippetCount).
     DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
   LoadingSnippetsFinished();
 }
 
 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);
   InsertAllIDs(snippets_, &old_snippet_ids);
   new_snippets.erase(
       std::remove_if(
           new_snippets.begin(), new_snippets.end(),
           [&old_snippet_ids](const std::unique_ptr<NTPSnippet>& snippet) {
             if (old_snippet_ids.count(snippet->id()))
               return true;
@@ skipping 30 matching lines @@
                        }),
         new_snippets.end());
     int num_snippets_discarded = num_new_snippets - new_snippets.size();
     UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
                           num_snippets_discarded > 0);
     if (num_snippets_discarded > 0) {
       UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
                                   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);
   for (const base::Value* value : *list) {
     std::string str;
     bool success = value->GetAsString(&str);
     DCHECK(success) << "Failed to parse snippet host from prefs";
     hosts.insert(std::move(str));
   }
   return hosts;
 }
 
 void NTPSnippetsService::StoreSnippetHostsToPrefs(
     const std::set<std::string>& hosts) {
   base::ListValue list;
   for (const std::string& host : hosts)
     list.AppendString(host);
   pref_service_->Set(prefs::kSnippetHosts, list);
 }
 
 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());
+  // 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 more snippets now than we want to show, drop the extra ones
-  // from the end of the list.
-  if (snippets_.size() > kMaxSnippetCount)
+  // 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());
   if (snippets_.empty() && !discarded_snippets_.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          discarded_snippets_.size());
   }
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
@@ skipping 20 matching lines @@
 bool NTPSnippetsService::IsSyncStateIncompatible() {
   if (!sync_service_ || !sync_service_->CanSyncStart())
     return true;
   if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone())
     return false;  // Sync service is not initialized, yet not disabled.
   return !sync_service_->GetActiveDataTypes().Has(
       syncer::HISTORY_DELETE_DIRECTIVES);
 }
 
 }  // namespace ntp_snippets