Show content suggestions from all categories on the NTP

Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate sections.

BUG=619560, 616090
Committed: https://crrev.com/cc3ed5d9ca70635cf707d94cbdac78f9902b96ef
Cr-Commit-Position: refs/heads/master@{#408078}

[Philipp Keck, 2016-07-26 11:45:59]

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

[Philipp Keck, 2016-07-26 11:46:00]

PTAL

[Marc Treib, 2016-07-26 12:54:09]

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:151: // Show all other
sections before ARTICLES, at most 5 suggestions per section.
Can you please add a TODO that this is a temporary debug thingie?

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:162: int counter = 0;
Why the "at most 5" logic? Since this will only happen if the user manually
enables a flag, it doesn't really matter if there are many. And the logic for
restricting the number should live in the provider.

[Philipp Keck, 2016-07-26 13:55:58]

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:151: // Show all other
sections before ARTICLES, at most 5 suggestions per section.
On 2016/07/26 12:54:08, Marc Treib wrote:
> Can you please add a TODO that this is a temporary debug thingie?

Done.

https://codereview.chromium.org/2184683002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:162: int counter = 0;
On 2016/07/26 12:54:08, Marc Treib wrote:
> Why the "at most 5" logic? Since this will only happen if the user manually
> enables a flag, it doesn't really matter if there are many. And the logic for
> restricting the number should live in the provider.

Ok I moved it there. Testing how an empty NTP behaves or what happens when you
dismiss the last suggestion is annoying if you have to swipe away hundreds of
pages that were offlined in the background -- that's why I introduced the limit
here. It's now in the offline pages provider.

[Marc Treib, 2016-07-26 14:00:00]

Please update the CL description (remove the "at most 5" part). Also this
probably shouldn't go on the multi-section UI bug?

https://codereview.chromium.org/2184683002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2184683002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:151: // Show all other
sections before ARTICLES, at most 5 suggestions per section.
The "at most 5" part isn't applicable anymore.

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

https://codereview.chromium.org/2184683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:118:
// a constant?
Yup, you should probably introduce a constant :)
Also, you don't need a counter here, you can just check suggestions.size()

[Philipp Keck, 2016-07-26 14:29:34]

Description was changed from

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories (all ARTICLES and at most 5 from every other section) are
displayed in a single category to allow experimenting with new
ContentSuggestionProviders until the UI supports separate sections.

BUG=619560,616090
==========

to

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate
sections.

BUG=619560,616090
==========

[Philipp Keck, 2016-07-26 15:47:20]

Fixed Marc's comments in Patch Set 3.
Patch Set 4 merges the new loop in the snippets bridge with the existing one.
It's less code duplication like this.

https://codereview.chromium.org/2184683002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2184683002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:151: // Show all other
sections before ARTICLES, at most 5 suggestions per section.
On 2016/07/26 14:00:00, Marc Treib wrote:
> The "at most 5" part isn't applicable anymore.

Done.

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

https://codereview.chromium.org/2184683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:118:
// a constant?
On 2016/07/26 14:00:00, Marc Treib wrote:
> Yup, you should probably introduce a constant :)
> Also, you don't need a counter here, you can just check suggestions.size()

Good idea. Constant is there, I put a question about the desired value in the
PRD.

[Marc Treib, 2016-07-26 16:37:31]

lgtm

[Philipp Keck, 2016-07-27 08:01:15]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-07-27 08:01:49]

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-07-27 08:47:23]

Description was changed from

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate
sections.

BUG=619560,616090
==========

to

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate
sections.

BUG=619560,616090
==========

[commit-bot: I haz the power, 2016-07-27 08:47:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 08:49:19]

Description was changed from

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate
sections.

BUG=619560,616090
==========

to

==========
Show content suggestions from all categories on the NTP

Currently, the NTP UI only supports a single section of content
suggestions. Therefore, the available content suggestions from all
categories are displayed in a single category to allow
experimenting with new ContentSuggestionProviders until the UI supports separate
sections.

BUG=619560,616090

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

[commit-bot: I haz the power, 2016-07-27 08:49:20]

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