[NTP Snippets] Persist remote categories in prefs

[NTP Snippets] Persist remote categories in prefs

This is the first part of persisting non-article remote suggestions.
The next part will be persisting the suggestions themselves.

BUG=653476
Committed: https://crrev.com/136a7131948b4adc886e8da7ddfac54c2ae8b1b8
Cr-Commit-Position: refs/heads/master@{#424122}

[Marc Treib, 2016-10-07 14:31:51]

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

[commit-bot: I haz the power, 2016-10-07 14:32:02]

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

[Marc Treib, 2016-10-07 14:32:13]

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

[Marc Treib, 2016-10-07 14:32:13]

PTAL!

[tschumann, 2016-10-07 14:45:54]

tschumann@chromium.org changed reviewers:
+ tschumann@chromium.org

[tschumann, 2016-10-07 14:45:54]

tschumann@chromium.org changed reviewers:
+ tschumann@chromium.org

[tschumann, 2016-10-07 14:45:55]

lgtm
with nits.
we should probably add a TODO() where we receive the response from the service
to update the category ordering if necessary. Otherwise, experiments with
varying section order won't work...

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:743:
GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
inline json_str?

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:751:
ASSERT_NE(observer().StatusForCategory(other_category()),
ideally, this method would be called unknown_category() or
remote_unknown_category().

[tschumann, 2016-10-07 14:45:55]

lgtm
with nits.
we should probably add a TODO() where we receive the response from the service
to update the category ordering if necessary. Otherwise, experiments with
varying section order won't work...

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:743:
GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
inline json_str?

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:751:
ASSERT_NE(observer().StatusForCategory(other_category()),
ideally, this method would be called unknown_category() or
remote_unknown_category().

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

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

[commit-bot: I haz the power, 2016-10-07 15:15:33]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-07 15:28:41]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2016-10-07 15:29:56]

TODO for reordering added.

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:743:
GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
On 2016/10/07 14:45:55, tschumann wrote:
> inline json_str?

Done.

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:751:
ASSERT_NE(observer().StatusForCategory(other_category()),
On 2016/10/07 14:45:55, tschumann wrote:
> ideally, this method would be called unknown_category() or
> remote_unknown_category().

Hm. There's corresponding MakeArticleID/MakeOtherID - IMO MakeUnknownID would be
a bit weird.
I tend to think "other" is unspecific enough. Unless you feel strongly, I'd
leave it as is.

[Marc Treib, 2016-10-07 15:30:23]

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

[commit-bot: I haz the power, 2016-10-07 15:30:34]

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-07 16:12:26]

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

[commit-bot: I haz the power, 2016-10-07 16:12:27]

Dry run: This issue passed the CQ dry run.

[tschumann, 2016-10-07 16:12:45]

lgtm

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:751:
ASSERT_NE(observer().StatusForCategory(other_category()),
On 2016/10/07 15:29:55, Marc Treib wrote:
> On 2016/10/07 14:45:55, tschumann wrote:
> > ideally, this method would be called unknown_category() or
> > remote_unknown_category().
> 
> Hm. There's corresponding MakeArticleID/MakeOtherID - IMO MakeUnknownID would
be
> a bit weird.
> I tend to think "other" is unspecific enough. Unless you feel strongly, I'd
> leave it as is.

The problem I'm seeing is that in this case, we don't want to be unspecific. If
it's a known ID then the test might just pass accidentally.
We could of course just create the category here our selves ;-)

[Bernhard Bauer, 2016-10-07 16:21:27]

lgtm

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

https://codereview.chromium.org/2395273003/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:48: // INSERT NEW REMOTE CATEGORIES HERE!
So it would be the server's responsibility not to reuse IDs for server-side
categories?

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

https://codereview.chromium.org/2395273003/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:1033: const char
kCategoryContentId[] = "id";
Hm... Shouldn't the anonymous namespace go to the top of the file?

[Marc Treib, 2016-10-10 08:33:43]

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2395273003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:751:
ASSERT_NE(observer().StatusForCategory(other_category()),
On 2016/10/07 16:12:45, tschumann wrote:
> On 2016/10/07 15:29:55, Marc Treib wrote:
> > On 2016/10/07 14:45:55, tschumann wrote:
> > > ideally, this method would be called unknown_category() or
> > > remote_unknown_category().
> > 
> > Hm. There's corresponding MakeArticleID/MakeOtherID - IMO MakeUnknownID
would
> be
> > a bit weird.
> > I tend to think "other" is unspecific enough. Unless you feel strongly, I'd
> > leave it as is.
> 
> The problem I'm seeing is that in this case, we don't want to be unspecific.
If
> it's a known ID then the test might just pass accidentally.
> We could of course just create the category here our selves ;-)

Alright, made an unknown_category.

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

https://codereview.chromium.org/2395273003/diff/40001/components/ntp_snippets...
components/ntp_snippets/category.h:48: // INSERT NEW REMOTE CATEGORIES HERE!
On 2016/10/07 16:21:27, Bernhard Bauer wrote:
> So it would be the server's responsibility not to reuse IDs for server-side
> categories?

Correct. But that's the plan anyway.

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

https://codereview.chromium.org/2395273003/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:1033: const char
kCategoryContentId[] = "id";
On 2016/10/07 16:21:27, Bernhard Bauer wrote:
> Hm... Shouldn't the anonymous namespace go to the top of the file?

That's certainly the common thing, though the style guide doesn't actually
enforce it as far as I can tell. In fact, from
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts:
"Note that if there is more than one class in the .cc file, it can aid clarity
to define your file-scope helpers in an anonymous namespace just above the class
that uses them, instead of at the top of the file."
(Which of course doesn't actually apply here, since it's not a separate
class...)
Oh well, moved it to the top. Consistency!

[Marc Treib, 2016-10-10 08:34:13]

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

[commit-bot: I haz the power, 2016-10-10 08:34:19]

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 09:19:20]

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

[commit-bot: I haz the power, 2016-10-10 09:19:21]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-10 09:19:55]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-10-10 09:19:56]

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

[commit-bot: I haz the power, 2016-10-10 09:20:11]

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 09:25:07]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-10 09:26:40]

Description was changed from

==========
[NTP Snippets] Persist remote categories in prefs

This is the first part of persisting non-article remote suggestions.
The next part will be persisting the suggestions themselves.

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

to

==========
[NTP Snippets] Persist remote categories in prefs

This is the first part of persisting non-article remote suggestions.
The next part will be persisting the suggestions themselves.

BUG=653476

Committed: https://crrev.com/136a7131948b4adc886e8da7ddfac54c2ae8b1b8
Cr-Commit-Position: refs/heads/master@{#424122}
==========

[commit-bot: I haz the power, 2016-10-10 09:26:41]

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