Unifies the additional actions for Categories

Unifies the additional actions for Categories

The Categories can have an additional action, fetch or view all.
For now they are represented as two different bool. It is unclear if they can
be set at the same time or not. They are also display on the UI with the same
text "More".
This CL creates a new API to expose them.

BUG=none

Review-Url: https://codereview.chromium.org/2751093003
Cr-Commit-Position: refs/heads/master@{#457785}
Committed: https://chromium.googlesource.com/chromium/src/+/db135527d8e98d22bbd95f3d9f79ad2d7ea9fc7e

[gambard, 2017-03-15 13:45:44]

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

[gambard, 2017-03-15 13:45:44]

For discussion, what do you think of this API change?
The UI will still decide what to do as "ViewAll" action but it would be clearer
to see that you can only have one of them.

[dgn, 2017-03-15 13:56:03]

Thanks for the suggestion! I'd be in favour of it. We already map to a similar
enum in the UI:

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

It would be nice to be consistent everywhere.

https://codereview.chromium.org/2751093003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_info.h (right):

https://codereview.chromium.org/2751093003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_info.h:33: OPEN_ALL
nit: current language choice is "view all", I'd prefer avoid having to rename a
bunch of things to be consistent with the new enum

[gambard, 2017-03-15 13:59:51]

https://codereview.chromium.org/2751093003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_info.h (right):

https://codereview.chromium.org/2751093003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_info.h:33: OPEN_ALL
On 2017/03/15 13:56:02, dgn wrote:
> nit: current language choice is "view all", I'd prefer avoid having to rename
a
> bunch of things to be consistent with the new enum

I agree but "ViewAll" refers more to "Display all the current (cached) item in
the UI using a "ViewAll" method in the ContentSuggestions than the actual
behavior in my mind.
I was looking for a "ViewAll" method :)

[Marc Treib, 2017-03-15 14:33:56]

At some point in the past, there was a design where a section could have both of
these actions at the same time. That could have made sense e.g. for Bookmarks:
"More" = "show the next 10 here", "View All" = "go to the bookmarks UI".

Then UX changed their minds, and now each section can have only one of those.
I'd be slightly concerned about them changing their minds again, in which case
we'd have to go back to the old design. But I guess that's not very likely, so
I'm okay with this change.

Thanks!

[Marc Treib, 2017-03-15 14:37:14]

Re "View all" vs "Open all": I guess "open" is slightly clearer, but IMO the
diff isn't large enough to warrant the churn of renaming. So I'm with dgn here:
Let's keep "View all".

[gambard, 2017-03-16 15:54:55]

Thanks, so we keep ViewAll but we move to an enum.
dgn@, I have never modified the Java part. Do I need to modify something which
is generating the SnippetsBridge or I should directly modify:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

And how can I generate the enum?

[dgn, 2017-03-16 16:43:19]

On 2017/03/16 15:54:55, gambard wrote:
> Thanks, so we keep ViewAll but we move to an enum.
> dgn@, I have never modified the Java part. Do I need to modify something which
> is generating the SnippetsBridge or I should directly modify:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> And how can I generate the enum?

No, the SnippetsBridge is hand written, so you would have to modify it. 

If you want you can make the CL in 2 parts: convert to an enum, using that
everywhere but in the bridge, and keep has_fetch_action() and
has_view_all_action() around only for the bridge. I can do the second part for
the Android code and just remove these 2 native methods.

If you want to do it yourself, enum generation happens through GN. Hints:
- ntp_snippets_java_enums_srcjar in
https://cs.chromium.org/chromium/src/components/ntp_snippets/BUILD.gn?l=122
- comment to specify the java package in
https://cs.chromium.org/chromium/src/components/ntp_snippets/category_status.h

[gambard, 2017-03-17 08:45:39]

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

[commit-bot: I haz the power, 2017-03-17 08:45:46]

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, 2017-03-17 08:53:58]

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

[commit-bot: I haz the power, 2017-03-17 08:53:59]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gambard, 2017-03-17 09:18:43]

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

[commit-bot: I haz the power, 2017-03-17 09:18:47]

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, 2017-03-17 09:27:59]

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

[commit-bot: I haz the power, 2017-03-17 09:28:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gambard, 2017-03-17 09:37:04]

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

[commit-bot: I haz the power, 2017-03-17 09:37:21]

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, 2017-03-17 09:48:13]

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

[commit-bot: I haz the power, 2017-03-17 09:48:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gambard, 2017-03-17 09:57:17]

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

[commit-bot: I haz the power, 2017-03-17 09:57:31]

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, 2017-03-17 10:18:25]

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

[commit-bot: I haz the power, 2017-03-17 10:18:26]

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

[gambard, 2017-03-17 10:30:34]

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

[commit-bot: I haz the power, 2017-03-17 10:30:50]

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

[gambard, 2017-03-17 10:52:27]

Description was changed from

==========
[Discussion] Unifies the additional actions for Categories

The Categories can have an additional action, fetch or view all.
For now they are represented as two different bool. It is unclear if they can
be set at the same time or not. They are also display on the UI with the same
text "More".
This CL suggests a new API to expose them.

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

to

==========
Unifies the additional actions for Categories

The Categories can have an additional action, fetch or view all.
For now they are represented as two different bool. It is unclear if they can
be set at the same time or not. They are also display on the UI with the same
text "More".
This CL creates a new API to expose them.

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

[commit-bot: I haz the power, 2017-03-17 11:15:54]

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

[commit-bot: I haz the power, 2017-03-17 11:15:54]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-03-17 12:17:11]

PTAL.
The rest of the Java will need to be modified accordingly.

[Marc Treib, 2017-03-17 12:31:33]

components/ntp_snippets/ lgtm with some comments/requests for things that aren't
your fault :)

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

https://codereview.chromium.org/2751093003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:293:
int cardLayout, @ContentSuggestionsAdditionalActionEnum int additionalAction,
Should cardLayout also have an annotation?

https://codereview.chromium.org/2751093003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2751093003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:212: CategoryInfo
BuildArticleCategoryInfo(
...wait, why is this and BuildRemoteCategoryInfo below duplicated here and in
json_request.cc? Can we remove one of the two?

(Not your fault of course. So if removing one doesn't work immediately, please
just put a TODO in both places. Thanks!)

[gambard, 2017-03-17 13:39:59]

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

[gambard, 2017-03-17 13:40:11]

Thanks!

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

https://codereview.chromium.org/2751093003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:293:
int cardLayout, @ContentSuggestionsAdditionalActionEnum int additionalAction,
On 2017/03/17 12:31:33, Marc Treib wrote:
> Should cardLayout also have an annotation?

Done.

https://codereview.chromium.org/2751093003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2751093003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:212: CategoryInfo
BuildArticleCategoryInfo(
On 2017/03/17 12:31:33, Marc Treib wrote:
> ...wait, why is this and BuildRemoteCategoryInfo below duplicated here and in
> json_request.cc? Can we remove one of the two?
> 
> (Not your fault of course. So if removing one doesn't work immediately, please
> just put a TODO in both places. Thanks!)

Done.

[commit-bot: I haz the power, 2017-03-17 13:40:17]

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

[dgn, 2017-03-17 13:45:27]

lgtm % comments

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

https://codereview.chromium.org/2751093003/diff/120001/components/ntp_snippet...
components/ntp_snippets/category_info.h:24: // GENERATED_JAVA_ENUM_PACKAGE:
org.chromium.chrome.browser.ntp.snippets
I'd prefer if you used org.chromium.chrome.browser.suggestions as package.

https://codereview.chromium.org/2751093003/diff/120001/components/ntp_snippet...
components/ntp_snippets/category_info.h:29: // More suggestions can be fetch
using the Fetch methods with this category.
s/can be fetch/can be fetched/

[gambard, 2017-03-17 14:05:08]

Thanks!

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

https://codereview.chromium.org/2751093003/diff/120001/components/ntp_snippet...
components/ntp_snippets/category_info.h:24: // GENERATED_JAVA_ENUM_PACKAGE:
org.chromium.chrome.browser.ntp.snippets
On 2017/03/17 13:45:27, dgn wrote:
> I'd prefer if you used org.chromium.chrome.browser.suggestions as package.

Done.
I took the same package as ContentSuggestionsCardLayout.

https://codereview.chromium.org/2751093003/diff/120001/components/ntp_snippet...
components/ntp_snippets/category_info.h:29: // More suggestions can be fetch
using the Fetch methods with this category.
On 2017/03/17 13:45:26, dgn wrote:
> s/can be fetch/can be fetched/

Done.

[gambard, 2017-03-17 14:05:13]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-17 14:05:13]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2751093003/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2017-03-17 14:05:27]

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, 2017-03-17 14:28:01]

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

[commit-bot: I haz the power, 2017-03-17 14:28:02]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gambard, 2017-03-17 15:18:00]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-17 15:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2751093003/#ps160001
(title: "Fix compilation on Android (hopefully)")

[commit-bot: I haz the power, 2017-03-17 15:18:23]

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, 2017-03-17 16:07:01]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489763880047870,
"parent_rev": "e6967d7ed906839db8acfdc287bfd209e481062c", "commit_rev":
"db135527d8e98d22bbd95f3d9f79ad2d7ea9fc7e"}

[commit-bot: I haz the power, 2017-03-17 16:07:49]

Description was changed from

==========
Unifies the additional actions for Categories

The Categories can have an additional action, fetch or view all.
For now they are represented as two different bool. It is unclear if they can
be set at the same time or not. They are also display on the UI with the same
text "More".
This CL creates a new API to expose them.

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

to

==========
Unifies the additional actions for Categories

The Categories can have an additional action, fetch or view all.
For now they are represented as two different bool. It is unclear if they can
be set at the same time or not. They are also display on the UI with the same
text "More".
This CL creates a new API to expose them.

BUG=none

Review-Url: https://codereview.chromium.org/2751093003
Cr-Commit-Position: refs/heads/master@{#457785}
Committed:
https://chromium.googlesource.com/chromium/src/+/db135527d8e98d22bbd95f3d9f79...
==========

[commit-bot: I haz the power, 2017-03-17 16:07:51]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/db135527d8e98d22bbd95f3d9f79...