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

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

BUG=587819
Committed: https://crrev.com/720f3a2148598295c59769195d29fa806e1ae492
Cr-Commit-Position: refs/heads/master@{#378234}

[Marc Treib, 2016-02-25 16:58:37]

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

[Marc Treib, 2016-02-25 16:58:38]

PTAL!

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

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:20: const char kSnippetsPref[] =
"ntp_snippets.snippets";
This should be defined in pref_names.{h,cc} files inside this component.

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:173: return false;
The return value seems unused.

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:182: update->Clear();
Instead of doing this, just build the new list and then set it with
PrefService::Set().

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:49:
NTPSnippetsService(PrefService* pref_service,
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 16:55:43]

Patchset #4 (id:60001) has been deleted

[Marc Treib, 2016-02-26 17:02:32]

Patchset #4 (id:80001) has been deleted

[Marc Treib, 2016-02-26 17:04:13]

Patchset #4 (id:100001) has been deleted

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

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:20: const char kSnippetsPref[] =
"ntp_snippets.snippets";
On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> This should be defined in pref_names.{h,cc} files inside this component.

Done.

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:173: return false;
On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> The return value seems unused.

I've removed the return value, and instead added a LOG for when LoadFromJSONList
fails.

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:182: update->Clear();
On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> Instead of doing this, just build the new list and then set it with
> PrefService::Set().

Done.

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:49:
NTPSnippetsService(PrefService* pref_service,
On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> 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...

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]

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:49:
NTPSnippetsService(PrefService* pref_service,
On 2016/02/26 17:06:12, Marc Treib wrote:
> On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> > 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...
> 
> What would be a "more suited storage"? IMO prefs isn't actually a bad place
for
> this.

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.

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

https://codereview.chromium.org/1737563003/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:172: LOG(ERROR) << "Failed to
parse " << prefs::kSnippets << " pref";
Nit: printing the value of the pref key is not going to be very useful -- just
"Failed to parse snippets from prefs"?

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

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

https://codereview.chromium.org/1737563003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:49:
NTPSnippetsService(PrefService* pref_service,
On 2016/02/26 17:30:48, Bernhard Bauer wrote:
> On 2016/02/26 17:06:12, Marc Treib wrote:
> > On 2016/02/25 17:16:33, Bernhard Bauer wrote:
> > > 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...
> > 
> > What would be a "more suited storage"? IMO prefs isn't actually a bad place
> for
> > this.
> 
> 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.

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.

> (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?)

This is indeed interesting!

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

Done, added a TODO below.

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

https://codereview.chromium.org/1737563003/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:172: LOG(ERROR) << "Failed to
parse " << prefs::kSnippets << " pref";
On 2016/02/26 17:30:48, Bernhard Bauer wrote:
> Nit: printing the value of the pref key is not going to be very useful -- just
> "Failed to parse snippets from prefs"?

Done.

[Bernhard Bauer, 2016-02-29 10:43:46]

LGTM!

[Marc Treib, 2016-02-29 10:45:25]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-02-29 10:45:29]

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

[commit-bot: I haz the power, 2016-02-29 10:47:42]

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

[commit-bot: I haz the power, 2016-02-29 10:47:43]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Marc Treib, 2016-02-29 11:14:16]

Patchset #6 (id:160001) has been deleted

[Marc Treib, 2016-02-29 11:16:25]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-29 11:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1737563003/#ps180001
(title: "rebase, add missing files")

[commit-bot: I haz the power, 2016-02-29 11:16:30]

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

[commit-bot: I haz the power, 2016-02-29 11:20:29]

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

[commit-bot: I haz the power, 2016-02-29 11:20:30]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Marc Treib, 2016-02-29 11:34:17]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-29 11:34:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1737563003/#ps200001
(title: "fix gn deps")

[commit-bot: I haz the power, 2016-02-29 11:34:48]

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

[commit-bot: I haz the power, 2016-02-29 11:44:54]

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

[commit-bot: I haz the power, 2016-02-29 11:44:56]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Marc Treib, 2016-02-29 12:33:57]

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

[Marc Treib, 2016-02-29 12:33:58]

+noyau for ios/. PTAL!

[noyau (Ping after 24h), 2016-02-29 15:57:14]

https://codereview.chromium.org/1737563003/diff/220001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1737563003/diff/220001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:428:
ntp_snippets::NTPSnippetsService::RegisterProfilePrefs(registry);
This needs to be done in ios/chrome/browser/prefs/browser_prefs.mm as well.

https://codereview.chromium.org/1737563003/diff/220001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1737563003/diff/220001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:183: dict->Set("contentInfo",
snippet->ToDictionary());
"contentInfo should probably be a constant.

[Marc Treib, 2016-02-29 17:07:38]

https://codereview.chromium.org/1737563003/diff/220001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1737563003/diff/220001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:428:
ntp_snippets::NTPSnippetsService::RegisterProfilePrefs(registry);
On 2016/02/29 15:57:14, noyau wrote:
> This needs to be done in ios/chrome/browser/prefs/browser_prefs.mm as well.

Done.

https://codereview.chromium.org/1737563003/diff/220001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1737563003/diff/220001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:183: dict->Set("contentInfo",
snippet->ToDictionary());
On 2016/02/29 15:57:14, noyau wrote:
> "contentInfo should probably be a constant.

Done.

[noyau (Ping after 24h), 2016-02-29 17:18:19]

lgtm

[Marc Treib, 2016-02-29 17:19:05]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-29 17:19:06]

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

[commit-bot: I haz the power, 2016-02-29 17:19:20]

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

[commit-bot: I haz the power, 2016-02-29 18:24:38]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-02-29 18:27:38]

Description was changed from

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

BUG=587819
==========

to

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

BUG=587819

Committed: https://crrev.com/720f3a2148598295c59769195d29fa806e1ae492
Cr-Commit-Position: refs/heads/master@{#378234}
==========

[commit-bot: I haz the power, 2016-02-29 18:27:40]

Patchset 9 (id:??) landed as
https://crrev.com/720f3a2148598295c59769195d29fa806e1ae492
Cr-Commit-Position: refs/heads/master@{#378234}