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

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

BUG=605870
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4db94acd2c51b5c67cc00175b5a0dad4f865eb03
Cr-Commit-Position: refs/heads/master@{#397367}

[Marc Treib, 2016-05-23 14:00:57]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

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

PTAL! (not urgent; feel free to delay this until all the deadline-relevant stuff
is done!)

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database_unittest.cc:49: return snippet;
Arguably, this and the two operator==s above should go into a new
"ntp_snippet_test_util.h" or something like that - WDYT?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:191: NTPSnippet::PtrVector
snippets_;
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.

[Marc Treib, 2016-05-25 09:49:50]

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:222:
registry->RegisterListPref(prefs::kDiscardedSnippets);
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]

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets.gypi:52: 'includes': [ '../build/protoc.gypi' ],
This just goes into the target for the static library?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:28: : database_(new
ProtoDatabaseImpl<SnippetProto>(file_task_runner)),
std::move the |file_task_runner|?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:40: bool callback_was_null =
callback_.is_null();
Couldn't we DCHECK instead that we only do one callback?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:139:
database_->UpdateEntries(std::move(entries_to_save),
It sucks a bit that ProtoDatabase takes unique_ptr's everywhere (I assume it
predates std::move use in Chrome), but that doesn't necessarily mean we have to
do that as well. Could we take the KeyEntryVector directly and move into a
unique_ptr here?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.h:55:
leveldb_proto::ProtoDatabase<SnippetProto>::KeyEntryVector;
Out of curiosity: could we just use `using
leveldb_proto::ProtoDatabase<SnippetProto>::KeyEntryVector`?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.h:70: bool database_inited_;
Nit: Can we use "initialized"?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database_unittest.cc:69: if (db_)
This isn't necessary -- a .reset() on a null pointer is a no-op.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:170: void
EraseNullptrs(NTPSnippet::PtrVector* snippets) {
Nit: this is very often called "compact".

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:222:
registry->RegisterListPref(prefs::kDiscardedSnippets);
On 2016/05/25 09:49:49, Marc Treib wrote:
> 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?

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

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:301: if (!loaded())
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?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:171: INITED,
We should add some comments now for what these states mean.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:191: NTPSnippet::PtrVector
snippets_;
On 2016/05/23 14:00:58, Marc Treib wrote:
> 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.

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...)

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:247: run_loop_.Run();
If you already have an object for this, you could move the Run() to a separate
method, so you can create this first, then call Init() on the service, then wait
for it to finish. That way it will also work if the service finishes loading
synchronously.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:260: void
NTPSnippetsServiceShutdown() override {};
No semicolon

[Marc Treib, 2016-05-27 14:03:13]

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets.gypi:52: 'includes': [ '../build/protoc.gypi' ],
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> This just goes into the target for the static library?

Err.. what?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:28: : database_(new
ProtoDatabaseImpl<SnippetProto>(file_task_runner)),
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> std::move the |file_task_runner|?

Done.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:40: bool callback_was_null =
callback_.is_null();
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> Couldn't we DCHECK instead that we only do one callback?

Well, it changes the interface - the caller will have to make sure not to call
this again before a previous call finished. But since at least for now, it's
only ever called once, I guess that's fine, so done.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.cc:139:
database_->UpdateEntries(std::move(entries_to_save),
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> It sucks a bit that ProtoDatabase takes unique_ptr's everywhere (I assume it
> predates std::move use in Chrome), but that doesn't necessarily mean we have
to
> do that as well. Could we take the KeyEntryVector directly and move into a
> unique_ptr here?

Hm, I guess. But then it'd be easy to accidentally copy the vector. Also moving
into the unique_ptr is kinda ugly... I prefer it like this. So, do you insist?
;)

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.h:55:
leveldb_proto::ProtoDatabase<SnippetProto>::KeyEntryVector;
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> Out of curiosity: could we just use `using
> leveldb_proto::ProtoDatabase<SnippetProto>::KeyEntryVector`?

No - apparently that only works if leveldb_proto::ProtoDatabase<SnippetProto> is
a base class.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database.h:70: bool database_inited_;
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> Nit: Can we use "initialized"?

Done.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_database_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_database_unittest.cc:69: if (db_)
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> This isn't necessary -- a .reset() on a null pointer is a no-op.

Done.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:170: void
EraseNullptrs(NTPSnippet::PtrVector* snippets) {
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> Nit: this is very often called "compact".

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

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:222:
registry->RegisterListPref(prefs::kDiscardedSnippets);
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> On 2016/05/25 09:49:49, Marc Treib wrote:
> > 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?
> 
> I think given that this hasn't launched to production yet, it's fine to just
> drop the prefs without cleaning them up.

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?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:301: if (!loaded())
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> 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?

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_"?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:171: INITED,
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> We should add some comments now for what these states mean.

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

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:191: NTPSnippet::PtrVector
snippets_;
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> On 2016/05/23 14:00:58, Marc Treib wrote:
> > 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.
> 
> Yeah, that probably makes sense -- and it will allow us to remove a bunch of
> code, so we could do that right now.

It'll change the interface though (getting snippets will be async), so I'd like
to defer that to another CL.

> Can we make sure that the DB caches in memory though?
> 
> (Also, bonus points if we could drop the cache under memory pressure...)

I *think* LevelDB should do all of that already.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:247: run_loop_.Run();
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> If you already have an object for this, you could move the Run() to a separate
> method, so you can create this first, then call Init() on the service, then
wait
> for it to finish. That way it will also work if the service finishes loading
> synchronously.

Init() doesn't exist anymore :)
I guess we could return early if the service is already loaded, but since that
can't happen right now, I'd prefer to keep this as-is.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:260: void
NTPSnippetsServiceShutdown() override {};
On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> No semicolon

Done.

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

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets.gypi:52: 'includes': [ '../build/protoc.gypi' ],
On 2016/05/27 14:03:12, Marc Treib wrote:
> On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > This just goes into the target for the static library?
> 
> Err.. what?

This is part of the declaration of the target "ntp_snippets", which is of type
static_library. I was just wondering whether this actually works, or whether it
requires a separate target.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:301: if (!loaded())
On 2016/05/27 14:03:12, Marc Treib wrote:
> On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > 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?
> 
> 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.

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.

> We don't cancel any pending fetch here, so it's possible anyway that new
> snippets appear soon after.

That might be a separate bug?

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

Yup, that seems reasonable, although see two paragraphs up: maybe we'll need to
queue a bunch of actions?

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:191: NTPSnippet::PtrVector
snippets_;
On 2016/05/27 14:03:12, Marc Treib wrote:
> On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > On 2016/05/23 14:00:58, Marc Treib wrote:
> > > 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.
> > 
> > Yeah, that probably makes sense -- and it will allow us to remove a bunch of
> > code, so we could do that right now.
> 
> It'll change the interface though (getting snippets will be async), so I'd
like
> to defer that to another CL.

Hm... y'know, I'm thinking it might be worth frontloading this work now rather
than doing it later.

> > Can we make sure that the DB caches in memory though?
> > 
> > (Also, bonus points if we could drop the cache under memory pressure...)
> 
> I *think* LevelDB should do all of that already.

OK... It'd be good to verify that though.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:247: run_loop_.Run();
On 2016/05/27 14:03:12, Marc Treib wrote:
> On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > If you already have an object for this, you could move the Run() to a
separate
> > method, so you can create this first, then call Init() on the service, then
> wait
> > for it to finish. That way it will also work if the service finishes loading
> > synchronously.
> 
> Init() doesn't exist anymore :)
> I guess we could return early if the service is already loaded, but since that
> can't happen right now, I'd prefer to keep this as-is.

It's a bit of a defense-in-depth thing: if you at some point in the future
should notify synchronously, you might start failing in this test. The test code
is a common pattern: First set up the observer, then do the action, then wait
for the observation.

https://codereview.chromium.org/1987333003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
EXPECT_TRUE(!service_->loaded());
Really, shouldn't this be EXPECT_FALSE? :-D

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

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets.gypi (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets.gypi:52: 'includes': [ '../build/protoc.gypi' ],
On 2016/05/27 16:06:42, Bernhard Bauer wrote:
> On 2016/05/27 14:03:12, Marc Treib wrote:
> > On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > > This just goes into the target for the static library?
> > 
> > Err.. what?
> 
> This is part of the declaration of the target "ntp_snippets", which is of type
> static_library. I was just wondering whether this actually works, or whether
it
> requires a separate target.

Ah :)
components/suggestions.gypi uses it in the same way, so I assume this works.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:301: if (!loaded())
On 2016/05/27 16:06:42, Bernhard Bauer wrote:
> On 2016/05/27 14:03:12, Marc Treib wrote:
> > On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.

> > We don't cancel any pending fetch here, so it's possible anyway that new
> > snippets appear soon after.
> 
> That might be a separate bug?

Possibly :) But as above, probably better handled by Nicolas' CL.
(That CL is one reason why I'm trying to land this quickly...)

> > Ignoring the "fetch" seems problematic though: We'll probably attempt
> background
> > fetches before the DB is loaded. WDYT about a "bool fetch_after_load_"?
> 
> Yup, that seems reasonable, although see two paragraphs up: maybe we'll need
to
> queue a bunch of actions?

fetch_after_load_ done.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:191: NTPSnippet::PtrVector
snippets_;
On 2016/05/27 16:06:43, Bernhard Bauer wrote:
> On 2016/05/27 14:03:12, Marc Treib wrote:
> > On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > > On 2016/05/23 14:00:58, Marc Treib wrote:
> > > > 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.
> > > 
> > > Yeah, that probably makes sense -- and it will allow us to remove a bunch
of
> > > code, so we could do that right now.
> > 
> > It'll change the interface though (getting snippets will be async), so I'd
> like
> > to defer that to another CL.
> 
> Hm... y'know, I'm thinking it might be worth frontloading this work now rather
> than doing it later.

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 ;)

> > > Can we make sure that the DB caches in memory though?
> > > 
> > > (Also, bonus points if we could drop the cache under memory pressure...)
> > 
> > I *think* LevelDB should do all of that already.
> 
> OK... It'd be good to verify that though.

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.

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:247: run_loop_.Run();
On 2016/05/27 16:06:43, Bernhard Bauer wrote:
> On 2016/05/27 14:03:12, Marc Treib wrote:
> > On 2016/05/25 15:01:15, Bernhard Bauer wrote:
> > > If you already have an object for this, you could move the Run() to a
> separate
> > > method, so you can create this first, then call Init() on the service,
then
> > wait
> > > for it to finish. That way it will also work if the service finishes
loading
> > > synchronously.
> > 
> > Init() doesn't exist anymore :)
> > I guess we could return early if the service is already loaded, but since
that
> > can't happen right now, I'd prefer to keep this as-is.
> 
> It's a bit of a defense-in-depth thing: if you at some point in the future
> should notify synchronously, you might start failing in this test. The test
code
> is a common pattern: First set up the observer, then do the action, then wait
> for the observation.

This pattern isn't possible anymore, since the NTPSnippetsService ctor
immediately starts the DB load.
I have changed the helper to also handle the case when the DB is already loaded,
so it'll continue to work if that ever becomes synchronous.

https://codereview.chromium.org/1987333003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/1987333003/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
EXPECT_TRUE(!service_->loaded());
On 2016/05/27 16:06:43, Bernhard Bauer wrote:
> Really, shouldn't this be EXPECT_FALSE? :-D

Right :D
This line doesn't exist anymore though; this class handles both cases now.

[Marc Treib, 2016-05-30 14:46:15]

Description was changed from

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

BUG=605870
==========

to

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

BUG=605870
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-05-30 14:53:26]

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (left):

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:134:
*pref_service->GetList(ntp_snippets::prefs::kSnippets), &json);
This is a bit unfortunate: We can't send the actual stored json anymore, because
it doesn't exist anymore. I tried sending stringified protos instead, but
MessageLite only supports binary serialization. The full-blown Message has
DebugString(), but it isn't used in Chromium.

[Bernhard Bauer, 2016-05-31 08:20:59]

lgtm

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (left):

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:134:
*pref_service->GetList(ntp_snippets::prefs::kSnippets), &json);
On 2016/05/30 14:53:26, Marc Treib wrote:
> This is a bit unfortunate: We can't send the actual stored json anymore,
because
> it doesn't exist anymore. I tried sending stringified protos instead, but
> MessageLite only supports binary serialization. The full-blown Message has
> DebugString(), but it isn't used in Chromium.

Yeah, but I guess between dumping the last downloaded JSON and the
representation of stored parsed snippets, I hope we have everything necessary.

[Marc Treib, 2016-05-31 08:23:44]

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (left):

https://codereview.chromium.org/1987333003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:134:
*pref_service->GetList(ntp_snippets::prefs::kSnippets), &json);
On 2016/05/31 08:20:59, Bernhard Bauer wrote:
> On 2016/05/30 14:53:26, Marc Treib wrote:
> > This is a bit unfortunate: We can't send the actual stored json anymore,
> because
> > it doesn't exist anymore. I tried sending stringified protos instead, but
> > MessageLite only supports binary serialization. The full-blown Message has
> > DebugString(), but it isn't used in Chromium.
> 
> Yeah, but I guess between dumping the last downloaded JSON and the
> representation of stored parsed snippets, I hope we have everything necessary.

In principle, yeah. Right now we just dump the exact thing we send to the
internals page, which is *not* everything from the parsed snippet (though that
should probably be fixed).

[Marc Treib, 2016-05-31 08:24:42]

treib@chromium.org changed reviewers:
+ mathp@chromium.org, noyau@chromium.org

[Marc Treib, 2016-05-31 08:24:43]

+noyau for ios/
+mathp for new dependency on leveldb_proto

PTAL!

[Mathieu, 2016-05-31 14:14:40]

lgtm for dependency

[noyau (Ping after 24h), 2016-05-31 15:26:59]

https://codereview.chromium.org/1987333003/diff/160001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/1987333003/diff/160001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:104:
browser_state->GetStatePath().Append(FILE_PATH_LITERAL("NTPSnippets")));
Should the string be a constant shared by all platforms?

[Marc Treib, 2016-05-31 16:21:24]

https://codereview.chromium.org/1987333003/diff/160001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/1987333003/diff/160001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:104:
browser_state->GetStatePath().Append(FILE_PATH_LITERAL("NTPSnippets")));
On 2016/05/31 15:26:59, noyau wrote:
> Should the string be a constant shared by all platforms?

Sure, that makes sense. I moved the constant into ntp_snippets_constants.h.

[Marc Treib, 2016-05-31 16:21:33]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-31 16:22:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987333003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987333003/180001

[noyau (Ping after 24h), 2016-05-31 16:26:50]

lgtm

[commit-bot: I haz the power, 2016-05-31 17:13:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 17:13:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-06-01 12:49:21]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 12:49:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987333003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987333003/220001

[commit-bot: I haz the power, 2016-06-01 14:28:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 14:28:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-06-01 14:56:53]

Patchset #12 (id:220001) has been deleted

[Marc Treib, 2016-06-02 08:15:37]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 08:15:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987333003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987333003/240001

[commit-bot: I haz the power, 2016-06-02 10:23:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 10:23:09]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marc Treib, 2016-06-02 10:24:39]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-06-02 10:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
bauerb@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1987333003/#ps240001
(title: "fix test memleaks")

[commit-bot: I haz the power, 2016-06-02 10:25:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987333003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987333003/240001

[commit-bot: I haz the power, 2016-06-02 11:32:55]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-06-02 11:34:15]

Description was changed from

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

BUG=605870
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

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

BUG=605870
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4db94acd2c51b5c67cc00175b5a0dad4f865eb03
Cr-Commit-Position: refs/heads/master@{#397367}
==========

[commit-bot: I haz the power, 2016-06-02 11:34:17]

Patchset 12 (id:??) landed as
https://crrev.com/4db94acd2c51b5c67cc00175b5a0dad4f865eb03
Cr-Commit-Position: refs/heads/master@{#397367}