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

components/ntp_snippets/ntp_snippets_service.h

Index: components/ntp_snippets/ntp_snippets_service.h
diff --git a/components/ntp_snippets/ntp_snippets_service.h b/components/ntp_snippets/ntp_snippets_service.h
index a5bac53c11f3fe8e0bd2034c6e06dcb037ef7eed..c407163fdd8887696a46c2492f06dff02cbb4264 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -16,7 +16,6 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/scoped_observer.h"
-#include "base/sequenced_task_runner.h"
 #include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/ntp_snippet.h"
@@ -50,6 +49,7 @@ class SyncService;
 
 namespace ntp_snippets {
 
+class NTPSnippetsDatabase;
 class NTPSnippetsServiceObserver;
 
 // Stores and vends fresh content data for the NTP.
@@ -63,15 +63,14 @@ class NTPSnippetsService : public KeyedService,
   // 'en' or 'en-US'. Note that this code should only specify the language, not
   // the locale, so 'en_US' (English language with US locale) and 'en-GB_US'
   // (British English person in the US) are not language codes.
-  NTPSnippetsService(
-      PrefService* pref_service,
-      sync_driver::SyncService* sync_service,
-      suggestions::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<image_fetcher::ImageFetcher> image_fetcher);
+  NTPSnippetsService(PrefService* pref_service,
+                     sync_driver::SyncService* sync_service,
+                     suggestions::SuggestionsService* suggestions_service,
+                     const std::string& application_language_code,
+                     NTPSnippetsScheduler* scheduler,
+                     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
+                     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
+                     std::unique_ptr<NTPSnippetsDatabase> database);
   ~NTPSnippetsService() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
@@ -81,6 +80,11 @@ class NTPSnippetsService : public KeyedService,
   // Inherited from KeyedService.
   void Shutdown() override;
 
+  // Returns whether the initial set of snippets has been loaded from the
+  // database. While this is false, the list of snippets will be empty, and all
+  // modifications to it (fetch, discard, etc) will be ignored.
+  bool loaded() const { return state_ == State::LOADED; }
+
   // Fetches snippets from the server and adds them to the current ones.
   void FetchSnippets();
   // Fetches snippets from the server for specified hosts (overriding
@@ -139,17 +143,17 @@ class NTPSnippetsService : public KeyedService,
   // sync_driver::SyncServiceObserver implementation.
   void OnStateChanged() override;
 
+  // Callback for the NTPSnippetsDatabase.
+  void OnDatabaseLoaded(NTPSnippet::PtrVector snippets);
+
+  // Callback for the SuggestionsService.
   void OnSuggestionsChanged(const suggestions::SuggestionsProfile& suggestions);
+
+  // Callback for the NTPSnippetsFetcher.
   void OnFetchFinished(NTPSnippetsFetcher::OptionalSnippets snippets);
 
   // Merges newly available snippets with the previously available list.
   void MergeSnippets(NTPSnippet::PtrVector new_snippets);
-  // TODO(treib): Investigate a better storage, maybe LevelDB or SQLite?
-  void LoadSnippetsFromPrefs();
-  void StoreSnippetsToPrefs();
-
-  void LoadDiscardedSnippetsFromPrefs();
-  void StoreDiscardedSnippetsToPrefs();
 
   std::set<std::string> GetSnippetHostsFromPrefs() const;
   void StoreSnippetHostsToPrefs(const std::set<std::string>& hosts);
@@ -165,6 +169,7 @@ class NTPSnippetsService : public KeyedService,
   enum class State {
     NOT_INITED,
     INITED,

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

We should add some comments now for what these states mean.

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

Sure, done. dgn's https://codereview.chromium.org/2000233002/ will soon overhaul
this anyway though.

+    LOADED,
     SHUT_DOWN
   } state_;
 
@@ -182,9 +187,6 @@ class NTPSnippetsService : public KeyedService,
 
   suggestions::SuggestionsService* suggestions_service_;
 
-  // The SequencedTaskRunner on which file system operations will be run.
-  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
-
   // All current suggestions (i.e. not discarded ones).
   NTPSnippet::PtrVector snippets_;

[Marc Treib, 2016/05/23 14:00:58]

To be discussed: Should we remove this (and discarded_snippets_ below), and
instead just fetch from the LevelDB on demand? AFAIK the LevelDB does in-memory
caching anyway.
It doesn't matter so much right now, but it will once we cache thumbnails -
wouldn't want to cache everything twice.

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

Yeah, that probably makes sense -- and it will allow us to remove a bunch of
code, so we could do that right now.

Can we make sure that the DB caches in memory though?

(Also, bonus points if we could drop the cache under memory pressure...)

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

It'll change the interface though (getting snippets will be async), so I'd like
to defer that to another CL.
I *think* LevelDB should do all of that already.

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

Hm... y'know, I'm thinking it might be worth frontloading this work now rather
than doing it later.
OK... It'd be good to verify that though.

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

It will require re-thinking the NTPSnippetsServiceObserver interface quite a
bit, and I still think that warrants another CL. ;P
But anyway, I'll start working on that now, and I can eventually merge it into
this CL if you insist ;)
According to the Leveldb docs
(https://rawgit.com/google/leveldb/master/doc/index.html, search for "Cache"),
it *can* explicitly cache in memory. Even though the docs don't say, it seems
that it uses an 8MB read cache and a 4MB write buffer by default
(https://github.com/google/leveldb/blob/master/include/leveldb/options.h). That
should be plenty for our case; we might even look into reducing these sizes.
Auto-eviction doesn't seem to be built in.

Of course, there's also the OS's disk cache, which should handle all this
reasonably well including auto-eviction, at least on desktop. I have no idea if
that even exists on Android though.

 
@@ -216,6 +218,9 @@ class NTPSnippetsService : public KeyedService,
 
   std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
 
+  // The database for persisting snippets.
+  std::unique_ptr<NTPSnippetsDatabase> database_;
+
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsService);
 };