Add CategoryInfo for meta information of content suggestions Categories

Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
Committed: https://crrev.com/bd2f650a67858b6b88ddbd1c7edd4833e55fabed
Cr-Commit-Position: refs/heads/master@{#410672}

[Philipp Keck, 2016-08-03 17:27:46]

Description was changed from

==========
[WIP] Add CategoryInfo for meta information of content suggestions Categories

BUG=632320
==========

to

==========
------------------------------------------------------------------------
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
these is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

[Philipp Keck, 2016-08-03 17:27:46]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-08-04 08:11:12]

PTAL

[tschumann, 2016-08-04 08:59:38]

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

[tschumann, 2016-08-04 08:59:39]

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
it might be better to explicitly signal the lookup-miss. That way the caller can
decide how to handle this case.
Change signature to 
bool GetCategoryInfo(Category category, CategoryInfo* info);
?

[Marc Treib, 2016-08-04 09:46:30]

Please remove the "----" line from the description (the first line of the
description will show up as the commit title!)

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category_info.h (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:18: CategoryInfo&
operator=(CategoryInfo&&);
Are these required?

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:12: #include
"base/strings/string16.h"
Not required here

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
On 2016/08/04 08:59:39, tschumann wrote:
> it might be better to explicitly signal the lookup-miss. That way the caller
can
> decide how to handle this case.
> Change signature to 
> bool GetCategoryInfo(Category category, CategoryInfo* info);
> ?

Explicitly signaling the failure might be good, but I'd prefer returning a
base::Optional<CategoryInfo> - out params are ugly :)

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:14: Articles for you
Wait, where did this string come from so far?

[Philipp Keck, 2016-08-04 09:51:18]

Description was changed from

==========
------------------------------------------------------------------------
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
these is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

to

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
these is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

[Philipp Keck, 2016-08-04 11:48:18]

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category_info.h (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:18: CategoryInfo&
operator=(CategoryInfo&&);
On 2016/08/04 09:46:29, Marc Treib wrote:
> Are these required?

Seems so. Otherwise "return CategoryInfo(someString)" does not work.

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:12: #include
"base/strings/string16.h"
On 2016/08/04 09:46:29, Marc Treib wrote:
> Not required here

Done.

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
On 2016/08/04 09:46:30, Marc Treib wrote:
> On 2016/08/04 08:59:39, tschumann wrote:
> > it might be better to explicitly signal the lookup-miss. That way the caller
> can
> > decide how to handle this case.
> > Change signature to 
> > bool GetCategoryInfo(Category category, CategoryInfo* info);
> > ?
> 
> Explicitly signaling the failure might be good, but I'd prefer returning a
> base::Optional<CategoryInfo> - out params are ugly :)

base::Optional sounds good. We could so something similar for
GetSuggestionsForCategory.
It would make the subtle difference between "empty category" and "no category"
more explicit. Just as with GetCategoryInfo(), the caller can distinguish the
cases using GetCategoryStatus.

I think we would guarantee that the CategoryInfo is available when the status is
anything but NOT_PROVIDED (that requirement would also go onto
ContentSuggestionsProvider::GetCategoryInfo). And we could guarantee that the
ContentSuggestions are available when the status is one of the AVAILABLE
statuses. The caller (the snippets bridge) would then do sth like this:
if (IsAvailable(GetCategoryStatus(category))) {
  DoSomethingWith(GetCategoryInfo(category).value());
}
So it would call .value() directly, because the status implies that it's
available. Otherwise, a DCHECK fails in .value().

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:14: Articles for you
On 2016/08/04 09:46:30, Marc Treib wrote:
> Wait, where did this string come from so far?

https://codesearch.chromium.org/chromium/src/chrome/android/java/strings/andr...
https://codesearch.chromium.org/chromium/src/chrome/android/java/res/layout/n...

I assume the Android team will remove that as soon as the text comes from the
backend. Related code is landing soon:
https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr...

[tschumann, 2016-08-04 11:51:24]

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
On 2016/08/04 11:48:18, Philipp Keck wrote:
> On 2016/08/04 09:46:30, Marc Treib wrote:
> > On 2016/08/04 08:59:39, tschumann wrote:
> > > it might be better to explicitly signal the lookup-miss. That way the
caller
> > can
> > > decide how to handle this case.
> > > Change signature to 
> > > bool GetCategoryInfo(Category category, CategoryInfo* info);
> > > ?
> > 
> > Explicitly signaling the failure might be good, but I'd prefer returning a
> > base::Optional<CategoryInfo> - out params are ugly :)
> 
> base::Optional sounds good. We could so something similar for
> GetSuggestionsForCategory.
> It would make the subtle difference between "empty category" and "no category"
> more explicit. Just as with GetCategoryInfo(), the caller can distinguish the
> cases using GetCategoryStatus.
> 
> I think we would guarantee that the CategoryInfo is available when the status
is
> anything but NOT_PROVIDED (that requirement would also go onto
> ContentSuggestionsProvider::GetCategoryInfo). And we could guarantee that the
> ContentSuggestions are available when the status is one of the AVAILABLE
> statuses. The caller (the snippets bridge) would then do sth like this:
> if (IsAvailable(GetCategoryStatus(category))) {
>   DoSomethingWith(GetCategoryInfo(category).value());
> }
> So it would call .value() directly, because the status implies that it's
> available. Otherwise, a DCHECK fails in .value().

While that's possible, it would be nice to not rely on this in code. IMO it's
just bad practice to not verify the Optional before accessing it's value and
it's also not a big inconvenience. You could also use the value accessor which
takes a default value.

[PEConn, 2016-08-04 12:08:38]

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

[PEConn, 2016-08-04 12:08:39]

There's a typo in the issue description:

these is meta information
   |
   V
there is meta information

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.cc:11:
CategoryInfo::CategoryInfo(CategoryInfo&&) = default;
If these are just default, why not set them to default in the *.h file?

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:16: CategoryInfo(const base::string16&
title);
Can you make this explicit?

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
What does this represent - an empty category or an invalid category? What will
happen when this case occurs - do we just pass the empty title forward?

If it's an invalid category can we have some sort of CategoryInfo.Valid method?

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:79: CategoryStatus
GetCategoryStatus(Category category) const;
Why by value instead of const reference?

[Philipp Keck, 2016-08-04 13:37:59]

Description was changed from

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
these is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

to

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

[Philipp Keck, 2016-08-04 14:17:43]

Addressed PEConn1's comments, fixed typo in issue description.

On 2016/08/04 11:51:24, tschumann wrote:
>
https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
> File components/ntp_snippets/content_suggestions_service.cc (right):
> 
>
https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
> components/ntp_snippets/content_suggestions_service.cc:55: return
> CategoryInfo(base::string16());
> On 2016/08/04 11:48:18, Philipp Keck wrote:
> > On 2016/08/04 09:46:30, Marc Treib wrote:
> > > On 2016/08/04 08:59:39, tschumann wrote:
> > > > it might be better to explicitly signal the lookup-miss. That way the
> caller
> > > can
> > > > decide how to handle this case.
> > > > Change signature to 
> > > > bool GetCategoryInfo(Category category, CategoryInfo* info);
> > > > ?
> > > 
> > > Explicitly signaling the failure might be good, but I'd prefer returning a
> > > base::Optional<CategoryInfo> - out params are ugly :)
> > 
> > base::Optional sounds good. We could so something similar for
> > GetSuggestionsForCategory.
> > It would make the subtle difference between "empty category" and "no
category"
> > more explicit. Just as with GetCategoryInfo(), the caller can distinguish
the
> > cases using GetCategoryStatus.
> > 
> > I think we would guarantee that the CategoryInfo is available when the
status
> is
> > anything but NOT_PROVIDED (that requirement would also go onto
> > ContentSuggestionsProvider::GetCategoryInfo). And we could guarantee that
the
> > ContentSuggestions are available when the status is one of the AVAILABLE
> > statuses. The caller (the snippets bridge) would then do sth like this:
> > if (IsAvailable(GetCategoryStatus(category))) {
> >   DoSomethingWith(GetCategoryInfo(category).value());
> > }
> > So it would call .value() directly, because the status implies that it's
> > available. Otherwise, a DCHECK fails in .value().
> 
> While that's possible, it would be nice to not rely on this in code. IMO it's
> just bad practice to not verify the Optional before accessing it's value and
> it's also not a big inconvenience. You could also use the value accessor which
> takes a default value.

What should the UI do in that case? Just display a section with an empty title?
And no More button?
So just have some default values? Or not display the section in this case, so:
CategoryStatus status = Get....();
Optional<CategoryInfo> info = Get....();
Optional<std::vector<ContentSuggestion>> suggestions = Get...();
if (IsAvailable(status) && info.is_set() && suggestions.is_set()) {
  // Show the section
}

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.cc:11:
CategoryInfo::CategoryInfo(CategoryInfo&&) = default;
On 2016/08/04 12:08:39, PEConn1 wrote:
> If these are just default, why not set them to default in the *.h file?

Done. (chromium-style might complain later when we add more fields to the class,
but we can always move it back to the cc file then.)

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:16: CategoryInfo(const base::string16&
title);
On 2016/08/04 12:08:39, PEConn1 wrote:
> Can you make this explicit?

Done.

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
On 2016/08/04 12:08:39, PEConn1 wrote:
> What does this represent - an empty category or an invalid category? What will
> happen when this case occurs - do we just pass the empty title forward?
> 
> If it's an invalid category can we have some sort of CategoryInfo.Valid
method?

We're discussing the possibility of returning an Optional<CategoryInfo>. See the
discussion there.

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

https://codereview.chromium.org/2207493002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:79: CategoryStatus
GetCategoryStatus(Category category) const;
On 2016/08/04 12:08:39, PEConn1 wrote:
> Why by value instead of const reference?

Because CategoryStatus is an enum.

In case you meant the CategoryInfo below:
That's currently directly handed through from the provider, which might create
it on stack and return it immediately, so references wouldn't work.
We were thinking about storing the CategoryInfo information somewhere else
though (to enforce it being constant and in order not to loose it when the
provider shuts down the category). But that hasn't happened yet and maybe won't
happen.

[Philipp Keck, 2016-08-08 13:24:57]

Description was changed from

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title.

Implement a new CategoryInfo struct (with currently only the title)
and corresponding method to hand the information from the
ContentSuggestionsProviders to the ContentSuggestionsService and out
towards the UI.
Implement those methods in the two providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

to

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

[Philipp Keck, 2016-08-08 13:25:39]

PTAL again.
Added card_layout, implemented the new methods for the
BookmarkSuggestionsProvider.

[PEConn, 2016-08-08 13:50:05]

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

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_info.h:15: enum class
ContentSuggestionsCardLayout {
Was there more of a discussion about this approach? I was under the opinoin we'd
be using:
bool hasThumbnail
bool hasSnippetText

[Philipp Keck, 2016-08-08 13:53:49]

On 2016/08/08 13:50:05, PEConn1 wrote:
>
https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
> File components/ntp_snippets/category_info.h (right):
> 
>
https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
> components/ntp_snippets/category_info.h:15: enum class
> ContentSuggestionsCardLayout {
> Was there more of a discussion about this approach? I was under the opinoin
we'd
> be using:
> bool hasThumbnail
> bool hasSnippetText

Yes, just open Hangouts, I'm happy with either solution.

[Marc Treib, 2016-08-08 14:11:27]

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

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:53: CategoryInfo
ContentSuggestionsService::GetCategoryInfo(
Didn't we agree on returning a base::Optional?

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:56: if (iterator ==
providers_by_category_.end())
Braces if the body doesn't fit on one line

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:13: <message
name="IDS_NTP_ARTICLE_SUGGESTIONS_SECTION_HEADER" desc="Header of the articles
section, which is a list of news articles displayed as cards on the ntp.">
The description probably need a bit more work, otherwise you'll get complaints
from translators. What "the articles section"? Also spell out New Tab Page,
translators don't know what "ntp" means. (I'm speaking from experience here :D)

[Philipp Keck, 2016-08-08 14:50:18]

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2207493002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:55: return
CategoryInfo(base::string16());
On 2016/08/04 11:51:24, tschumann wrote:
> On 2016/08/04 11:48:18, Philipp Keck wrote:
> > On 2016/08/04 09:46:30, Marc Treib wrote:
> > > On 2016/08/04 08:59:39, tschumann wrote:
> > > > it might be better to explicitly signal the lookup-miss. That way the
> caller
> > > can
> > > > decide how to handle this case.
> > > > Change signature to 
> > > > bool GetCategoryInfo(Category category, CategoryInfo* info);
> > > > ?
> > > 
> > > Explicitly signaling the failure might be good, but I'd prefer returning a
> > > base::Optional<CategoryInfo> - out params are ugly :)
> > 
> > base::Optional sounds good. We could so something similar for
> > GetSuggestionsForCategory.
> > It would make the subtle difference between "empty category" and "no
category"
> > more explicit. Just as with GetCategoryInfo(), the caller can distinguish
the
> > cases using GetCategoryStatus.
> > 
> > I think we would guarantee that the CategoryInfo is available when the
status
> is
> > anything but NOT_PROVIDED (that requirement would also go onto
> > ContentSuggestionsProvider::GetCategoryInfo). And we could guarantee that
the
> > ContentSuggestions are available when the status is one of the AVAILABLE
> > statuses. The caller (the snippets bridge) would then do sth like this:
> > if (IsAvailable(GetCategoryStatus(category))) {
> >   DoSomethingWith(GetCategoryInfo(category).value());
> > }
> > So it would call .value() directly, because the status implies that it's
> > available. Otherwise, a DCHECK fails in .value().
> 
> While that's possible, it would be nice to not rely on this in code. IMO it's
> just bad practice to not verify the Optional before accessing it's value and
> it's also not a big inconvenience. You could also use the value accessor which
> takes a default value.

Done.

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

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:53: CategoryInfo
ContentSuggestionsService::GetCategoryInfo(
On 2016/08/08 14:11:26, Marc Treib wrote:
> Didn't we agree on returning a base::Optional?

Done.

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:56: if (iterator ==
providers_by_category_.end())
On 2016/08/08 14:11:26, Marc Treib wrote:
> Braces if the body doesn't fit on one line

Done.

That's one of the many reasons why I'd always use braces everywhere...
Otherwise you do some search-and-replace and then use the formatter and 
suddenly the result of the auto-formatting violates the styleguide.

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:13: <message
name="IDS_NTP_ARTICLE_SUGGESTIONS_SECTION_HEADER" desc="Header of the articles
section, which is a list of news articles displayed as cards on the ntp.">
On 2016/08/08 14:11:27, Marc Treib wrote:
> The description probably need a bit more work, otherwise you'll get complaints
> from translators. What "the articles section"? Also spell out New Tab Page,
> translators don't know what "ntp" means. (I'm speaking from experience here
:D)

Spelled out "NTP" in the whole file.

It's not obvious, of course, what the "articles section" is, but the
"which"-clause right after is there to explain it. Anything else I could add
there?

[Marc Treib, 2016-08-08 14:54:41]

LGTM from my end. The "enum vs two bools" discussion still needs to be resolved.

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2207493002/diff/80001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:13: <message
name="IDS_NTP_ARTICLE_SUGGESTIONS_SECTION_HEADER" desc="Header of the articles
section, which is a list of news articles displayed as cards on the ntp.">
On 2016/08/08 14:50:18, Philipp Keck wrote:
> On 2016/08/08 14:11:27, Marc Treib wrote:
> > The description probably need a bit more work, otherwise you'll get
complaints
> > from translators. What "the articles section"? Also spell out New Tab Page,
> > translators don't know what "ntp" means. (I'm speaking from experience here
> :D)
> 
> Spelled out "NTP" in the whole file.
> 
> It's not obvious, of course, what the "articles section" is, but the
> "which"-clause right after is there to explain it. Anything else I could add
> there?

Yeah, this is probably fine.

[Philipp Keck, 2016-08-09 08:02:08]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-09 08:02:09]

bauerb@chromium.org: Please review changes in 
chrome/browser/ui/webui/snippets_internals_message_handler.cc

[Philipp Keck, 2016-08-09 08:02:51]

Conclusion from Hangouts chat: We're using the enums as implemented above for
now.

[Bernhard Bauer, 2016-08-09 09:18:33]

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:323: if (!info)
This shouldn't really happen if we're iterating over the categories, no? In that
case, a DCHECK would be more appropriate (and I would probably move it right
after the call to GetCategoryInfo).

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:83:
base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;
Hum. You go to all these lengths to implement a move constructor for
CategoryInfo (presumably because copying it expensive), and then you return an
Optional here, which makes a copy of the value. Why not just return a pointer?
Really, that *is* the common idiom in Chromium C++ for returning optional "big"
values.

[Marc Treib, 2016-08-09 09:45:12]

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:83:
base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;
On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> Hum. You go to all these lengths to implement a move constructor for
> CategoryInfo (presumably because copying it expensive), and then you return an
> Optional here, which makes a copy of the value. Why not just return a pointer?
> Really, that *is* the common idiom in Chromium C++ for returning optional
"big"
> values.

CategoryInfo doesn't have a copy ctor, so it can't make a copy :)

Anyway, I'd argue "pointer" is the common idiom only because Optional hasn't
been around for long. It's certainly the clearer thing.

[Philipp Keck, 2016-08-09 10:03:06]

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:83:
base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;
On 2016/08/09 09:45:12, Marc Treib wrote:
> On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> > Hum. You go to all these lengths to implement a move constructor for
> > CategoryInfo (presumably because copying it expensive), and then you return
an
> > Optional here, which makes a copy of the value. Why not just return a
pointer?
> > Really, that *is* the common idiom in Chromium C++ for returning optional
> "big"
> > values.
> 
> CategoryInfo doesn't have a copy ctor, so it can't make a copy :)
> 
> Anyway, I'd argue "pointer" is the common idiom only because Optional hasn't
> been around for long. It's certainly the clearer thing.

Indeed, the only relevant call we have
(https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...)
results in a call to the move constructor. I agree that base::Optional is more
readable, plus it works as intended.

[Bernhard Bauer, 2016-08-09 10:25:41]

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:69: return
CategoryInfo(base::ASCIIToUTF16("Recent bookmarks"),
I think you could already now use a localized string.

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:83:
base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;
On 2016/08/09 10:03:06, Philipp Keck wrote:
> On 2016/08/09 09:45:12, Marc Treib wrote:
> > On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> > > Hum. You go to all these lengths to implement a move constructor for
> > > CategoryInfo (presumably because copying it expensive), and then you
return
> an
> > > Optional here, which makes a copy of the value. Why not just return a
> pointer?
> > > Really, that *is* the common idiom in Chromium C++ for returning optional
> > "big"
> > > values.
> > 
> > CategoryInfo doesn't have a copy ctor, so it can't make a copy :)
> > 
> > Anyway, I'd argue "pointer" is the common idiom only because Optional hasn't
> > been around for long. It's certainly the clearer thing.

If you are saying that over time Optional<> will replace pointers for passing
optional non-copyable things around, I'm fairly certain that will not be the
case. To me, Optional<T> signifies optional _values_, and T* signifies optional
(non-copyable) objects.

> Indeed, the only relevant call we have
>
(https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...)
> results in a call to the move constructor. 

Ah right. It still feels a bit strange to construct a new string every time we
call GetCategoryInfo, even if it's not copied around...

> I agree that base::Optional is more
> readable, plus it works as intended.

Meh. It's very explicit about the fact that the lookup might fail, but at the
only current callsite we are iterating over the categories, so we should never
call it with a missing category. Really, this smacks a bit of handling
conditions that are not supposed to happen, which the style guide explicitly
argues against.

[Philipp Keck, 2016-08-09 11:24:27]

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:83:
base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;
On 2016/08/09 10:25:41, Bernhard Bauer wrote:
> On 2016/08/09 10:03:06, Philipp Keck wrote:
> > On 2016/08/09 09:45:12, Marc Treib wrote:
> > > On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> > > > Hum. You go to all these lengths to implement a move constructor for
> > > > CategoryInfo (presumably because copying it expensive), and then you
> return
> > an
> > > > Optional here, which makes a copy of the value. Why not just return a
> > pointer?
> > > > Really, that *is* the common idiom in Chromium C++ for returning
optional
> > > "big"
> > > > values.
> > > 
> > > CategoryInfo doesn't have a copy ctor, so it can't make a copy :)
> > > 
> > > Anyway, I'd argue "pointer" is the common idiom only because Optional
hasn't
> > > been around for long. It's certainly the clearer thing.
> 
> If you are saying that over time Optional<> will replace pointers for passing
> optional non-copyable things around, I'm fairly certain that will not be the
> case. To me, Optional<T> signifies optional _values_, and T* signifies
optional
> (non-copyable) objects.
> 
> > Indeed, the only relevant call we have
> >
>
(https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...)
> > results in a call to the move constructor. 
> 
> Ah right. It still feels a bit strange to construct a new string every time we
> call GetCategoryInfo, even if it's not copied around...
> 
> > I agree that base::Optional is more
> > readable, plus it works as intended.
> 
> Meh. It's very explicit about the fact that the lookup might fail, but at the
> only current callsite we are iterating over the categories, so we should never
> call it with a missing category. Really, this smacks a bit of handling
> conditions that are not supposed to happen, which the style guide explicitly
> argues against.

The callsites will change soon:
https://codereview.chromium.org/2230473002/diff/1/chrome/browser/android/ntp/...
https://codereview.chromium.org/2230473002/diff/1/chrome/android/java/src/org...
That could be called at any time, though currently it is still called right
after getCategories(), I think.

That being said, I'm also fine with using a pointer (and nullptr indicating
unavailability). That's also how it's treated behind the bridge: null means not
available.

[Philipp Keck, 2016-08-09 12:26:58]

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:69: return
CategoryInfo(base::ASCIIToUTF16("Recent bookmarks"),
On 2016/08/09 10:25:41, Bernhard Bauer wrote:
> I think you could already now use a localized string.

Done.

[Bernhard Bauer, 2016-08-09 12:38:40]

On 2016/08/09 11:24:27, Philipp Keck wrote:
> The callsites will change soon:
>
https://codereview.chromium.org/2230473002/diff/1/chrome/browser/android/ntp/...
>
https://codereview.chromium.org/2230473002/diff/1/chrome/android/java/src/org...
> That could be called at any time, though currently it is still called right
> after getCategories(), I think.

OK, thanks for the heads-up! It would also be worth at the very least
documenting the contract here, because I would again imagine that we should
never try to get CategoryInfo for a missing category in the UI either.

> That being said, I'm also fine with using a pointer (and nullptr indicating
> unavailability). That's also how it's treated behind the bridge: null means
not
> available.

OK. I guess I'd be okay with leaving the final decision up to Marc. I would like
the one thing below to be fixed though:

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:323: if (!info)
On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> This shouldn't really happen if we're iterating over the categories, no? In
that
> case, a DCHECK would be more appropriate (and I would probably move it right
> after the call to GetCategoryInfo).

^^^

[Philipp Keck, 2016-08-09 12:45:14]

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2207493002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:323: if (!info)
On 2016/08/09 09:18:33, Bernhard Bauer wrote:
> This shouldn't really happen if we're iterating over the categories, no? In
that
> case, a DCHECK would be more appropriate (and I would probably move it right
> after the call to GetCategoryInfo).

Done.

[Philipp Keck, 2016-08-09 12:56:37]

pke@google.com changed reviewers:
+ sadrul@chromium.org

[Philipp Keck, 2016-08-09 12:56:37]

sadrul@chromium.org: Please review the changes to DEPS.

[Bernhard Bauer, 2016-08-09 12:59:11]

lgtm

[Philipp Keck, 2016-08-09 13:00:08]

The CQ bit was checked by pke@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 13:00:39]

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-08-09 13:58:01]

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

[commit-bot: I haz the power, 2016-08-09 13:58:02]

Dry run: This issue passed the CQ dry run.

[PEConn, 2016-08-09 14:00:00]

LGTM

[sadrul, 2016-08-09 14:46:52]

On 2016/08/09 12:56:37, Philipp Keck wrote:
> mailto:sadrul@chromium.org: Please review the changes to DEPS.

lgtm

[Philipp Keck, 2016-08-09 14:49:19]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-09 14:49:19]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2207493002/#ps180001
(title: "DCHECK against empty base::Optional<CategoryInfo> in
SnippetsInternals")

[commit-bot: I haz the power, 2016-08-09 14:49:44]

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-08-09 14:54:09]

Description was changed from

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

to

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

[commit-bot: I haz the power, 2016-08-09 14:54:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-09 14:59:04]

Description was changed from

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320
==========

to

==========
Add CategoryInfo for meta information of content suggestions Categories

Apart from the ContentSuggestions in a Category and its current status,
there is meta information assigned with a Category that does not change
even for dynamically added categories. This information includes the
title, for example.

Implement a new CategoryInfo struct (with currently only the title
and a card_layout field) and corresponding methods to hand the
information from the ContentSuggestionsProviders to the
ContentSuggestionsService and out towards the UI.
Implement those methods in the three providers we currently have.

For the offline pages, only use a placeholder string at the moment,
because it will provide two distinct categories with different titles
in the future.

BUG=632320

Committed: https://crrev.com/bd2f650a67858b6b88ddbd1c7edd4833e55fabed
Cr-Commit-Position: refs/heads/master@{#410672}
==========

[commit-bot: I haz the power, 2016-08-09 14:59:05]

Patchset 10 (id:??) landed as
https://crrev.com/bd2f650a67858b6b88ddbd1c7edd4833e55fabed
Cr-Commit-Position: refs/heads/master@{#410672}