[NTP Snippets] Pass "no suggestions" message from the backend to the UI

[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008
Committed: https://crrev.com/c8a8ed14c688ed2a049e0e10cce036df3f6457cb
Cr-Commit-Position: refs/heads/master@{#427733}

[Marc Treib, 2016-10-26 12:39:01]

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

[Marc Treib, 2016-10-26 12:39:02]

PTAL!

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:45: bool has_more_button() const {
return has_more_button_; }
(Unrelated to this change, but:) Both the comment and the name don't really
match what the UI does with this flag. It's really more of a
"show_more_button_if_not_empty" - should we rename it and update the
description? Also, should we maybe differentiate between "view all" (what we
have so far) and the actual "more" that fhorschig is working on in
https://codereview.chromium.org/2421463002/ ?

[dgn, 2016-10-26 14:05:40]

You can use 656008 as the issue number

https://codereview.chromium.org/2447163004/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2447163004/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:178:
ContentSuggestionsCardLayout.FULL_CARD, false, true, "No suggestions"));
Just use ContentSuggestionsTestUtils#createInfo here instead ?

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:45: bool has_more_button() const {
return has_more_button_; }
On 2016/10/26 12:39:01, Marc Treib wrote:
> (Unrelated to this change, but:) Both the comment and the name don't really
> match what the UI does with this flag. It's really more of a
> "show_more_button_if_not_empty" - should we rename it and update the
> description? Also, should we maybe differentiate between "view all" (what we
> have so far) and the actual "more" that fhorschig is working on in
> https://codereview.chromium.org/2421463002/ ?

Yes, that would be a nice follow up CL. or "always_show_more_button"?

"More" is supposed to 3 things:

1. Go to some different UI to show all the [Bookmarks|Downloads|etc]
2. Fetch more suggestions, if we already have some
3. Reload the suggestions. Currently used only when we don't have any
suggestions.

I suppose 2. and 3. are more or less the same thing, but I don't know if we want
to let the categoryInfo specify things with such detail. We currently have the
behaviours hardcoded in the UI for each category.

And while we're on it, the UI does not do anything with card_layout from
categoryInfo at the moment.

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc
(right):

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:76: if
(category == provided_category_) {
why not a DCHECK here like the others?

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:11: <message
name="IDS_NTP_STATUS_CARD_TITLE_NO_SUGGESTIONS" desc="On the New Tab Page, title
of the status card explaining that there is no new content available in the
section." formatter_data="android_java">
So we assume that this is going to be static all the time. Is that for sure?

[Marc Treib, 2016-10-26 14:09:19]

Description was changed from

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=none
==========

to

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008
==========

[Marc Treib, 2016-10-26 14:29:09]

Thanks! PTAL again.

https://codereview.chromium.org/2447163004/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2447163004/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:178:
ContentSuggestionsCardLayout.FULL_CARD, false, true, "No suggestions"));
On 2016/10/26 14:05:40, dgn wrote:
> Just use ContentSuggestionsTestUtils#createInfo here instead ?

That's in junit/..., while this file is in javatests/..., so I can't simply use
it. :(
Is there some common place where I could move it? (And is that worth the churn?)

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:45: bool has_more_button() const {
return has_more_button_; }
On 2016/10/26 14:05:40, dgn wrote:
> On 2016/10/26 12:39:01, Marc Treib wrote:
> > (Unrelated to this change, but:) Both the comment and the name don't really
> > match what the UI does with this flag. It's really more of a
> > "show_more_button_if_not_empty" - should we rename it and update the
> > description? Also, should we maybe differentiate between "view all" (what we
> > have so far) and the actual "more" that fhorschig is working on in
> > https://codereview.chromium.org/2421463002/ ?
> 
> Yes, that would be a nice follow up CL. or "always_show_more_button"?

I updated the comment and added a TODO for now.

> "More" is supposed to 3 things:
> 
> 1. Go to some different UI to show all the [Bookmarks|Downloads|etc]
> 2. Fetch more suggestions, if we already have some
> 3. Reload the suggestions. Currently used only when we don't have any
> suggestions.

Exactly - I'm worried that it's super confusing when we use one term/flag for
three different things.

> I suppose 2. and 3. are more or less the same thing, but I don't know if we
want
> to let the categoryInfo specify things with such detail. We currently have the
> behaviours hardcoded in the UI for each category.

2 and 3 aren't quite the same thing. I'm not sure yet if we can treat them the
same here.
Anyway, that's for another day :)

> And while we're on it, the UI does not do anything with card_layout from
> categoryInfo at the moment.

Huh interesting - so how does the UI decide which layout to use? (I assume we
still want a reduced layout e.g. for downloads? Or is that just not
implemented?)

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc
(right):

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:76: if
(category == provided_category_) {
On 2016/10/26 14:05:40, dgn wrote:
> why not a DCHECK here like the others?

No reason :)  Done.

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:11: <message
name="IDS_NTP_STATUS_CARD_TITLE_NO_SUGGESTIONS" desc="On the New Tab Page, title
of the status card explaining that there is no new content available in the
section." formatter_data="android_java">
On 2016/10/26 14:05:40, dgn wrote:
> So we assume that this is going to be static all the time. Is that for sure?

Nothing is for sure :)
But it is static for now. I'd say we move it somewhere else if it ever becomes
non-static.

[dgn, 2016-10-26 16:00:33]

lgtm, provided the comment is fixed

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

https://codereview.chromium.org/2447163004/diff/20001/components/ntp_snippets...
components/ntp_snippets/category_info.h:45: bool has_more_button() const {
return has_more_button_; }
On 2016/10/26 14:29:09, Marc Treib wrote:
> On 2016/10/26 14:05:40, dgn wrote:
> > On 2016/10/26 12:39:01, Marc Treib wrote:
> > > (Unrelated to this change, but:) Both the comment and the name don't
really
> > > match what the UI does with this flag. It's really more of a
> > > "show_more_button_if_not_empty" - should we rename it and update the
> > > description? Also, should we maybe differentiate between "view all" (what
we
> > > have so far) and the actual "more" that fhorschig is working on in
> > > https://codereview.chromium.org/2421463002/ ?
> > 
> > Yes, that would be a nice follow up CL. or "always_show_more_button"?
> 
> I updated the comment and added a TODO for now.
> 
> > "More" is supposed to 3 things:
> > 
> > 1. Go to some different UI to show all the [Bookmarks|Downloads|etc]
> > 2. Fetch more suggestions, if we already have some
> > 3. Reload the suggestions. Currently used only when we don't have any
> > suggestions.
> 
> Exactly - I'm worried that it's super confusing when we use one term/flag for
> three different things.
> 
> > I suppose 2. and 3. are more or less the same thing, but I don't know if we
> want
> > to let the categoryInfo specify things with such detail. We currently have
the
> > behaviours hardcoded in the UI for each category.
> 
> 2 and 3 aren't quite the same thing. I'm not sure yet if we can treat them the
> same here.
> Anyway, that's for another day :)
> 
> > And while we're on it, the UI does not do anything with card_layout from
> > categoryInfo at the moment.
> 
> Huh interesting - so how does the UI decide which layout to use? (I assume we
> still want a reduced layout e.g. for downloads? Or is that just not
> implemented?)

Oh nvm. It is directly provided from c++ when creating the suggestion. Only the
java field is not used.

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

https://codereview.chromium.org/2447163004/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:42: // Whether the category should show
a "More" button even if it's empty. The
"even if it's not empty". We already always show it when empty.

[Marc Treib, 2016-10-26 16:35:28]

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

https://codereview.chromium.org/2447163004/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:42: // Whether the category should show
a "More" button even if it's empty. The
On 2016/10/26 16:00:33, dgn wrote:
> "even if it's not empty". We already always show it when empty.

Good catch, thanks! Done.

[Marc Treib, 2016-10-26 16:35:37]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-10-26 16:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2447163004/#ps60001
(title: "fix comment")

[commit-bot: I haz the power, 2016-10-26 16:35:58]

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-26 17:36:25]

Description was changed from

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008
==========

to

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008
==========

[commit-bot: I haz the power, 2016-10-26 17:36:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-26 17:50:56]

Description was changed from

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008
==========

to

==========
[NTP Snippets] Pass "no suggestions" message from the backend to the UI

BUG=656008

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

[commit-bot: I haz the power, 2016-10-26 17:50:57]

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