📰 Add an action button that can be used in sections

[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648
Committed: https://crrev.com/131f466e0c896c7c590246f17e8f1f11009f12e4
Cr-Commit-Position: refs/heads/master@{#410706}

[dgn, 2016-08-08 15:26:43]

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

[commit-bot: I haz the power, 2016-08-08 15:27:09]

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

[dgn, 2016-08-08 15:27:27]

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

[dgn, 2016-08-08 15:27:28]

PTAL

[dgn, 2016-08-08 15:27:28]

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

[dgn, 2016-08-08 15:27:28]

PTAL

[dgn, 2016-08-08 15:32:53]

Description was changed from

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

BUG=610648
==========

to

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648
==========

[dgn, 2016-08-08 16:04:34]

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

[commit-bot: I haz the power, 2016-08-08 16:04:58]

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-08 16:59:58]

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

[commit-bot: I haz the power, 2016-08-08 16:59:59]

Dry run: This issue passed the CQ dry run.

[PEConn, 2016-08-08 18:13:11]

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

[PEConn, 2016-08-08 18:13:12]

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_action_card.xml (right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:28:
android:paddingStart="8dp"
I think the style guide says that elements that are not id or layout_* should be
a alphabetical order.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:173:
assertEquals(4 + snippets.size(), mNtpAdapter.getItemCount());
Can this be a constant?

[Bernhard Bauer, 2016-08-09 09:02:06]

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_action_card.xml (right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:7: drawable) and
adds the selectable ripple effects as foreground. The foreground
I would align the text in the comment (like in the copyright notice above).

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:23:
android:layout_width="wrap_content"
Indent two more spaces

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:34:
android:textColor="@color/light_active_color"/>
Nit: space before closing the tag

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java
(right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:31:
manager.navigateToBookmarks();
Ideally, I would like to trigger this from the SuggestionsService, which would
know what to do with each category... I guess we can do that in a followup
though.

[dgn, 2016-08-09 13:00:37]

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

[dgn, 2016-08-09 13:00:42]

PTAL

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_action_card.xml (right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:7: drawable) and
adds the selectable ripple effects as foreground. The foreground
On 2016/08/09 09:02:06, Bernhard Bauer wrote:
> I would align the text in the comment (like in the copyright notice above).

Done.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:23:
android:layout_width="wrap_content"
On 2016/08/09 09:02:06, Bernhard Bauer wrote:
> Indent two more spaces

Done.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:28:
android:paddingStart="8dp"
On 2016/08/08 18:13:12, PEConn wrote:
> I think the style guide says that elements that are not id or layout_* should
be
> a alphabetical order.

Done.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_action_card.xml:34:
android:textColor="@color/light_active_color"/>
On 2016/08/09 09:02:06, Bernhard Bauer wrote:
> Nit: space before closing the tag

Done.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java
(right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:31:
manager.navigateToBookmarks();
On 2016/08/09 09:02:06, Bernhard Bauer wrote:
> Ideally, I would like to trigger this from the SuggestionsService, which would
> know what to do with each category... I guess we can do that in a followup
> though.

Yes, I'm leaving that up to another CL, when we have bookmarks in and we do the
wiring properly.

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2223743004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:173:
assertEquals(4 + snippets.size(), mNtpAdapter.getItemCount());
On 2016/08/08 18:13:12, PEConn wrote:
> Can this be a constant?

Done.

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

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

[Bernhard Bauer, 2016-08-09 13:03:09]

lgtm

[dgn, 2016-08-09 13:46:22]

Thanks! I noticed after your LGTM that I didn't handle the wide layout use case.
Inheriting from CardViewHolder fixes that. I also disabled the button by
default. It can be easily enabled when bookmarks are added.

[dgn, 2016-08-09 13:46:27]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-08-09 13:46:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2223743004/#ps100001
(title: "Disable showing the button by default")

[commit-bot: I haz the power, 2016-08-09 13:46:52]

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 15:04:28]

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

[commit-bot: I haz the power, 2016-08-09 15:04:29]

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...)

[dgn, 2016-08-09 15:42:32]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-08-09 15:42:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2223743004/#ps120001
(title: "fix test")

[commit-bot: I haz the power, 2016-08-09 15:42:49]

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

[dgn, 2016-08-09 15:47:16]

https://codereview.chromium.org/2223743004/diff/120001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2223743004/diff/120001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:45:
* TODO(pke): Make this depend on the category info of the loaded sections
FYI: added a TODO. If your change properly runs the tests without that, I can
also take care of it once it has landed. I would mostly rewrite them to make
proper use of the sections to manipulate the internal state of the adapter
instead of a few hardcoded things here.

[commit-bot: I haz the power, 2016-08-09 16:48:32]

Description was changed from

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648
==========

to

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648
==========

[commit-bot: I haz the power, 2016-08-09 16:48:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-09 16:51:48]

Description was changed from

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648
==========

to

==========
[NTP Client] Add an action button that can be used in sections

Adds the button to the sections, that can be enabled when they are
initialised.
The current implementation of the tap handler just opens the
bookmarks manager

Preview: https://goo.gl/photos/Z6A3toNYpV649Yjx8

BUG=610648

Committed: https://crrev.com/131f466e0c896c7c590246f17e8f1f11009f12e4
Cr-Commit-Position: refs/heads/master@{#410706}
==========

[commit-bot: I haz the power, 2016-08-09 16:51:50]

Patchset 7 (id:??) landed as
https://crrev.com/131f466e0c896c7c590246f17e8f1f11009f12e4
Cr-Commit-Position: refs/heads/master@{#410706}

[Philipp Keck, 2016-08-09 17:40:47]

pke@google.com changed reviewers:
+ pke@google.com

[Philipp Keck, 2016-08-09 17:40:48]

https://codereview.chromium.org/2223743004/diff/120001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2223743004/diff/120001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:45:
* TODO(pke): Make this depend on the category info of the loaded sections
On 2016/08/09 15:47:16, dgn wrote:
> FYI: added a TODO. If your change properly runs the tests without that, I can
> also take care of it once it has landed. I would mostly rewrite them to make
> proper use of the sections to manipulate the internal state of the adapter
> instead of a few hardcoded things here.

Thanks for the heads up. I'm not really an expert with all the position-stuff in
the adapter. The tests run fine with my changes, so I'm assigning the TODO to
you.