Zine: support multiple sections in the ui

Zine: support multiple sections in the ui

(This CL was continued in https://codereview.chromium.org/2196273002)

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=616090

[Michael van Ouwerkerk, 2016-07-28 16:32:15]

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

[commit-bot: I haz the power, 2016-07-28 16:32:39]

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

[Michael van Ouwerkerk, 2016-07-28 16:47:08]

Description was changed from

==========
Zine: support multiple sections in the ui.

BUG=
==========

to

==========
Zine: support multiple sections in the ui.

* The adapter now manages groups of items.
* A SuggestionsSection is a group holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=
==========

[Michael van Ouwerkerk, 2016-07-28 16:47:40]

Description was changed from

==========
Zine: support multiple sections in the ui.

* The adapter now manages groups of items.
* A SuggestionsSection is a group holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=
==========

to

==========
Zine: support multiple sections in the ui

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=
==========

[Michael van Ouwerkerk, 2016-07-28 16:48:43]

Description was changed from

==========
Zine: support multiple sections in the ui

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=
==========

to

==========
Zine: support multiple sections in the ui

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=616090
==========

[Michael van Ouwerkerk, 2016-07-28 16:50:34]

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

[Michael van Ouwerkerk, 2016-07-28 16:50:35]

Bernhard and Philipp, could you take a look please?

[Michael van Ouwerkerk, 2016-07-28 16:50:46]

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

[commit-bot: I haz the power, 2016-07-28 16:51:15]

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-07-28 17:09:19]

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

[commit-bot: I haz the power, 2016-07-28 17:09:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[PEConn, 2016-07-29 10:01:42]

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

[PEConn, 2016-07-29 10:01:43]

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:134:
private void onSuggestionsAvailable(int category, String[] ids, String[] titles,
String[] urls,
Will all the different categories be providing the same data here? Maybe offline
documents care about both URLs and AMP URLs but for future types of suggestion
this doesn't seem very general.

I think the reason we're passing the parameters like this (as a lists of fields
as opposed to just a list of objects) is that it's a pain to build Java objects
in native code. Ideally what we'd have is something like:

@CalledByNative
ArticleSnippet createSnippet(String id, String title, String url, ...) {...}

@CalledByNative
OfflinePage createOfflinePage(String id, String title, ...) {...}

Which are called to create the list of objects in native code, then
|onSuggestionsAvailable| just takes a category and a list of NewTabPageListItem.

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java
(right):

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java:12:
@IntDef({ContentSuggestionsCategory.ARTICLES,
ContentSuggestionsCategory.OFFLINE_PAGES})
As an aside, I should hopefully soon be landing a CL that creates Java
annotations for the enums generated from C++.

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategoryStatus.java
(right):

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategoryStatus.java:12:
@IntDef({ContentSuggestionsCategoryStatus.INITIALIZING,
ContentSuggestionsCategoryStatus.AVAILABLE,
nit: Just put one on each line?

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

https://codereview.chromium.org/2194433002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:21: COUNT
I think the convention is to have the last element called MAX, eg:
https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_me...

[Bernhard Bauer, 2016-08-01 15:01:34]

FYI, I'm continuing this CL over at https://codereview.chromium.org/2196273002.

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:134:
private void onSuggestionsAvailable(int category, String[] ids, String[] titles,
String[] urls,
On 2016/07/29 10:01:43, PEConn1 wrote:
> Will all the different categories be providing the same data here? Maybe
offline
> documents care about both URLs and AMP URLs but for future types of suggestion
> this doesn't seem very general.
> 
> I think the reason we're passing the parameters like this (as a lists of
fields
> as opposed to just a list of objects) is that it's a pain to build Java
objects
> in native code. Ideally what we'd have is something like:
> 
> @CalledByNative
> ArticleSnippet createSnippet(String id, String title, String url, ...) {...}
> 
> @CalledByNative
> OfflinePage createOfflinePage(String id, String title, ...) {...}
> 
> Which are called to create the list of objects in native code, then
> |onSuggestionsAvailable| just takes a category and a list of
NewTabPageListItem.

I think the general idea is indeed that all categories are going to provide
(most of) the same data. We also don't want to use different classes for content
suggestions from different categories, as they would have the same UI anyway. I
do like constructing the Java suggestions via a create() method though -- the
arrays we pass in here have to be converted in a loop each as well, so it's not
like that actually saves overhead.

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategoryStatus.java
(right):

https://codereview.chromium.org/2194433002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategoryStatus.java:12:
@IntDef({ContentSuggestionsCategoryStatus.INITIALIZING,
ContentSuggestionsCategoryStatus.AVAILABLE,
On 2016/07/29 10:01:43, PEConn1 wrote:
> nit: Just put one on each line?

Done.

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

https://codereview.chromium.org/2194433002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:21: COUNT
On 2016/07/29 10:01:43, PEConn1 wrote:
> I think the convention is to have the last element called MAX, eg:
>
https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_me...

MAX would be the maximum allowed value, which would really be OFFLINE_PAGES in
this case :)

[Michael van Ouwerkerk, 2016-08-09 14:04:04]

Description was changed from

==========
Zine: support multiple sections in the ui

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=616090
==========

to

==========
Zine: support multiple sections in the ui

(This CL was continued in https://codereview.chromium.org/2196273002)

* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status
card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.

BUG=616090
==========