[NTP Snippets] Persist non-article remote suggestions in the DB

[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
Committed: https://crrev.com/2477339aa480d0c36117c81919cef761b3242775
Cr-Commit-Position: refs/heads/master@{#424385}

[Marc Treib, 2016-10-10 13:20:07]

Description was changed from

==========
[NTP Snippets] Per non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
==========

to

==========
[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
==========

[Marc Treib, 2016-10-10 13:20:39]

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

[Marc Treib, 2016-10-10 13:20:39]

Here's the second part - PTAL!

[Bernhard Bauer, 2016-10-10 13:54:55]

Looks good; mostly nits:

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:18: const int kArticlesRemoteId =
1;
I'm starting to get the feeling that just using a pair<bool, int> might have
been easier :)

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:19:
static_assert(int(KnownCategories::ARTICLES) -
Should this be a static_cast<int>()?

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:191: remote_category_id =
proto.remote_category_id();
Nit: Maybe use a ternary operator so you only assign the variable once?

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.h (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.h:42: explicit NTPSnippet(const
std::string& id, int remote_category_id);
Nit: no explicit anymore

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet_unittest.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet_unittest.cc:22: return
NTPSnippet::CreateFromContentSuggestionsDictionary(*json_dict, 1);
You could expose the remote category in the header instead of hard-coding it?

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:639:
/*remote_category_id=*/-1,  // unused
Nit: Maybe put the value into a variable kUnusedRemoteCategoryId and use that?
Self-documenting code FTW, etc. :)

[Marc Treib, 2016-10-10 15:54:48]

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:18: const int kArticlesRemoteId =
1;
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> I'm starting to get the feeling that just using a pair<bool, int> might have
> been easier :)

...I don't quite follow? You mean essentially store an "Optional<int>
remote_id_" in NTPSnippet?

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:19:
static_assert(int(KnownCategories::ARTICLES) -
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> Should this be a static_cast<int>()?

Sure, done.

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:191: remote_category_id =
proto.remote_category_id();
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> Nit: Maybe use a ternary operator so you only assign the variable once?

Done.

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.h (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.h:42: explicit NTPSnippet(const
std::string& id, int remote_category_id);
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> Nit: no explicit anymore

Done.

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet_unittest.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet_unittest.cc:22: return
NTPSnippet::CreateFromContentSuggestionsDictionary(*json_dict, 1);
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> You could expose the remote category in the header instead of hard-coding it?

Done.

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:639:
/*remote_category_id=*/-1,  // unused
On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> Nit: Maybe put the value into a variable kUnusedRemoteCategoryId and use that?
> Self-documenting code FTW, etc. :)

Done.

[Bernhard Bauer, 2016-10-10 16:03:12]

LGTM!

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:18: const int kArticlesRemoteId =
1;
On 2016/10/10 15:54:47, Marc Treib wrote:
> On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> > I'm starting to get the feeling that just using a pair<bool, int> might have
> > been easier :)
> 
> ...I don't quite follow? You mean essentially store an "Optional<int>
> remote_id_" in NTPSnippet?

No, I meant that maybe we should have stored category IDs as pairs of
(local/remote x ID) instead of encoding both in a single int value.

[Marc Treib, 2016-10-10 16:04:15]

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2402323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippet.cc:18: const int kArticlesRemoteId =
1;
On 2016/10/10 16:03:12, Bernhard Bauer wrote:
> On 2016/10/10 15:54:47, Marc Treib wrote:
> > On 2016/10/10 13:54:54, Bernhard Bauer wrote:
> > > I'm starting to get the feeling that just using a pair<bool, int> might
have
> > > been easier :)
> > 
> > ...I don't quite follow? You mean essentially store an "Optional<int>
> > remote_id_" in NTPSnippet?
> 
> No, I meant that maybe we should have stored category IDs as pairs of
> (local/remote x ID) instead of encoding both in a single int value.

Aah - yes, that might have made sense...

[Marc Treib, 2016-10-10 16:04:19]

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

[commit-bot: I haz the power, 2016-10-10 16:04:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 16:59:31]

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

[commit-bot: I haz the power, 2016-10-10 16:59:32]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-11 07:25:10]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-10-11 07:25:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 07:30:25]

Description was changed from

==========
[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
==========

to

==========
[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
==========

[commit-bot: I haz the power, 2016-10-11 07:30:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-11 07:32:25]

Description was changed from

==========
[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476
==========

to

==========
[NTP Snippets] Persist non-article remote suggestions in the DB

Follow-up to https://codereview.chromium.org/2395273003/

BUG=653476

Committed: https://crrev.com/2477339aa480d0c36117c81919cef761b3242775
Cr-Commit-Position: refs/heads/master@{#424385}
==========

[commit-bot: I haz the power, 2016-10-11 07:32:26]

Patchset 3 (id:??) landed as
https://crrev.com/2477339aa480d0c36117c81919cef761b3242775
Cr-Commit-Position: refs/heads/master@{#424385}