[NTP Snippets] Persist the set of snippets in a pref

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 1b46bad976bd93300f8ddb6621e81300bd6acb82..201a0b01634fb18a50686af0ad678901e9768f16 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -20,6 +20,17 @@
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_scheduler.h"
 
+class PrefService;
+
+namespace base {
+class FilePath;
+class ListValue;
+}
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}
+
 namespace ntp_snippets {
 
 class NTPSnippetsServiceObserver;
@@ -35,12 +46,15 @@ class NTPSnippetsService : public KeyedService, NTPSnippetsFetcher::Observer {
   // '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 code.
-  NTPSnippetsService(scoped_refptr<base::SequencedTaskRunner> file_task_runner,
+  NTPSnippetsService(PrefService* pref_service,

[Bernhard Bauer, 2016/02/25 17:16:33]

Can you please add a TODO to move this to a more suited storage eventually?
Otherwise this seems like the kind of thing that would be in danger of staying
here for far too long...

[Marc Treib, 2016/02/26 17:06:12]

What would be a "more suited storage"? IMO prefs isn't actually a bad place for
this.

[Bernhard Bauer, 2016/02/26 17:30:48]

Prefs are loaded synchronously from disk at startup and kept in memory, so an
increase in size means longer startup and more memory usage (we do have crash
reports of Chrome running out of memory while serializing prefs). I think
something like LevelDB or SQLite might be better here in the long term.

(SQLite has the interesting additional property that Android has Java APIs for
accessing databases, so that might be helpful if we want to manipulate them
while the native browser process isn't initialized?)

I mean, I don't want to block rapid iteration on this, which is why I'd be happy
with a TODO for now, but I would like not to accumulate technical debt here for
too long.

[Marc Treib, 2016/02/29 10:04:42]

OTOH, storing this in prefs mean adding a few kB to an existing file read, which
will be much cheaper than even a single seek. Memory use is definitely a valid
concern though.
This is indeed interesting!
Done, added a TODO below.

+                     scoped_refptr<base::SequencedTaskRunner> file_task_runner,
                      const std::string& application_language_code,
                      NTPSnippetsScheduler* scheduler,
                      scoped_ptr<NTPSnippetsFetcher> snippets_fetcher);
   ~NTPSnippetsService() override;
 
+  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
   void Init(bool enabled);
 
   // Fetches snippets from the server. |overwrite| is true if existing snippets
@@ -80,14 +94,23 @@ class NTPSnippetsService : public KeyedService, NTPSnippetsFetcher::Observer {
   }
 
  private:
-  void OnFileReadDone(const std::string* json, bool success);
   void OnSnippetsDownloaded(const base::FilePath& download_path);
+  void OnFileReadDone(const std::string* json, bool success);
 
-  // Expects the JSON to be a list of dictionaries with keys matching the
-  // properties of a snippet (url, title, site_title, etc...). The url is the
-  // only mandatory value.
+  // Expects a top-level dictionary containing a "recos" list, which will be
+  // passed to |LoadFromJSONList|.
   bool LoadFromJSONString(const std::string& str);
 
+  // Expects a list of dictionaries each containing a "contentInfo" dictionary
+  // with keys matching the properties of a  snippet (url, title, site_title,
+  // etc...). The url is the only mandatory value.
+  bool LoadFromJSONList(const base::ListValue& list);
+
+  bool LoadFromPrefs();
+  void StoreToPrefs();
+
+  PrefService* pref_service_;
+
   // True if the suggestions are loaded.
   bool loaded_;