Support server-provided category names.

Support server-provided category names.

BUG=633613

[sfiera, 2016-08-25 16:00:01]

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

[commit-bot: I haz the power, 2016-08-25 16:00:21]

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

[sfiera, 2016-08-25 16:02:49]

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

[sfiera, 2016-08-25 16:02:50]

Small missing thing! Have you gone home or just to dinner?

[Marc Treib, 2016-08-25 16:33:12]

lgtm

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:162:
operator=(FetchedCategory&&) = default;
Is this the correct way to break the line?

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:564: NTPSnippet::PtrVector&
new_snippets = fetched_category.snippets;
This isn't really necessary anymore, now that we're not dealing with an
std::pair.

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:566: if (category !=
articles_category_)
Braces if the body doesn't fit on one line

[tschumann, 2016-08-25 16:46:31]

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:299: // running UI language.
Ignored for categories known to the client (i.e.
hm... the last sentence is a bit confusing. For articles you also set this
property. It would be nice if we could reduce the implications of
server-provided categories to a minimum. So simply enforce this to be always
present?

[sfiera, 2016-08-26 09:16:15]

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:162:
operator=(FetchedCategory&&) = default;
On 2016/08/25 16:33:11, Marc Treib wrote:
> Is this the correct way to break the line?

Apparently. I tried breaking it after the return type, but git cl format changed
it back.

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:564: NTPSnippet::PtrVector&
new_snippets = fetched_category.snippets;
On 2016/08/25 16:33:11, Marc Treib wrote:
> This isn't really necessary anymore, now that we're not dealing with an
> std::pair.

Removed it. Kept category, because it's referred to enough times to make it
annoying.

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:566: if (category !=
articles_category_)
On 2016/08/25 16:33:11, Marc Treib wrote:
> Braces if the body doesn't fit on one line

Done.

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2276383002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:299: // running UI language.
Ignored for categories known to the client (i.e.
On 2016/08/25 16:46:31, tschumann wrote:
> hm... the last sentence is a bit confusing. For articles you also set this
> property. It would be nice if we could reduce the implications of
> server-provided categories to a minimum. So simply enforce this to be always
> present?

Ah, actually, it's a mistake for this comment to be here. It's in
FetchedCategory that localized_title is ignored. Fixed, thanks.

[tschumann, 2016-08-29 09:59:53]

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

https://codereview.chromium.org/2276383002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:565: if (category !=
articles_category_) {
note: special casing the articles category like this is probably fine for now,
but we should find a better way at some point (right now, we hard-code this
here, in the constructor and maybe in other places).

It might also be cleaner to make the original intent clear,like only updating
titles for server-side categories. e.g.
if (!category.IsKnownCategory(KnownCategories::ARTICLES)) {
  // Only update titles from server-side provided categories.
  ...
}

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

https://codereview.chromium.org/2276383002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:295: struct CategoryContent {
this struct has some overlap with CategoryInfo.
Would be cleaner to store a CategoryInfo object inside the CategoryContent IMO.

[Marc Treib, 2016-09-08 10:38:04]

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

[commit-bot: I haz the power, 2016-09-08 10:38: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-09-08 12:01:49]

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

[commit-bot: I haz the power, 2016-09-08 12:01:50]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2016-09-08 13:03:27]

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

[Michael van Ouwerkerk, 2016-09-08 13:03:27]

I'm continuing this CL in:
https://codereview.chromium.org/2325473003

Thanks!

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

https://codereview.chromium.org/2276383002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:565: if (category !=
articles_category_) {
On 2016/08/29 09:59:53, tschumann wrote:
> note: special casing the articles category like this is probably fine for now,
> but we should find a better way at some point (right now, we hard-code this
> here, in the constructor and maybe in other places).
> 
> It might also be cleaner to make the original intent clear,like only updating
> titles for server-side categories. e.g.
> if (!category.IsKnownCategory(KnownCategories::ARTICLES)) {
>   // Only update titles from server-side provided categories.
>   ...
> }

The form of "category != articles_category_" is used throughout this file, so
for consistency I'll keep that. The comment seems a good idea, done.

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

https://codereview.chromium.org/2276383002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:295: struct CategoryContent {
On 2016/08/29 09:59:53, tschumann wrote:
> this struct has some overlap with CategoryInfo.
> Would be cleaner to store a CategoryInfo object inside the CategoryContent
IMO.

I've added a TODO to consider this for a followup CL.

[Michael van Ouwerkerk, 2016-09-12 09:02:17]

Description was changed from

==========
Support server-provided category names.

BUG=633613
==========

to

==========
Support server-provided category names.

BUG=633613
==========