📰 Make the MORE button more configurable

[NTP Client] Make the MORE button more configurable

Replace the single "has_more_button" from CategoryInfo by more
granular flags: "has_more_action", "has_reload_action",
"has_view_all_action"

Update ActionItem in the UI to have a behaviour similar to the one
of SignInPromo: an optional leaf, that doesn't need to be added and
removed from the parent, does it by itself. Added a parent class to
these 2 to implemnent that behaviour.

BUG=660030
Committed: https://crrev.com/3ad4278a7d10da110db8738252e83ad8cbc61687
Cr-Commit-Position: refs/heads/master@{#429542}

[dgn, 2016-10-31 16:48:11]

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

[dgn, 2016-10-31 16:48:12]

PTAL

[dgn, 2016-10-31 17:04:57]

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

[commit-bot: I haz the power, 2016-10-31 17:05:07]

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-10-31 17:21:03]

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

[commit-bot: I haz the power, 2016-10-31 17:21:04]

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

[dgn, 2016-10-31 18:02:45]

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

[commit-bot: I haz the power, 2016-10-31 18:03:33]

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

[dgn, 2016-10-31 18:35:49]

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

[commit-bot: I haz the power, 2016-10-31 18:36:27]

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-10-31 19:17:42]

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

[commit-bot: I haz the power, 2016-10-31 19:17:43]

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

[PEConn, 2016-11-01 11:05:44]

I do like this OptionalLeaf, it's nice!

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:23:
* reset the {@link ImpressionTracker} or configure it to fire only once.
Rewrite this comment a bit, maybe:
This will be called only once if the ImpressionTracker is initialized with
isOneShot=true or multiple times if not. Further calls can be prevented by
calling reset().

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:41:
private boolean mIsOneShot;
I think it would be nicer to have two separate classes - a
MultipleCallImpressionTracker and a OneShotImpressionTracker, but given there is
so much code overlap, maybe this is OK.

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:47:
public ImpressionTracker(@Nullable View view, Listener listener, boolean
isOneShot) {
Add some Javadoc for isOneShot?

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java:10: * A
leaf in the tree, i.e. a single item.
Change this to 'a *permanent* leaf in the tree' to make it clearer.

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:22:
public abstract class OptionalLeaf extends ChildNode implements Listener {
Write this as "implements ImpressionTracker.Listener"?

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:87:
if (position < 0 || position >= getItemCount()) {
Shouldn't this just be
if (position != 0)
?

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

https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.cc:28: CategoryInfo::~CategoryInfo() =
default;
Shouldn't these three "blah() = default" go in the header? (genuine question,
I'm not sure)

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

https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:24: class CategoryInfo {
I'm not very familiar with the C++ style guide, but this is looking to be just a
POD type. Why not just make all the fields const public and get rid of the
accessors?

[PEConn, 2016-11-01 11:20:16]

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

https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.h:24: class CategoryInfo {
On 2016/11/01 11:05:44, PEConn wrote:
> I'm not very familiar with the C++ style guide, but this is looking to be just
a
> POD type. Why not just make all the fields const public and get rid of the
> accessors?

Sorry, I realise it may not exactly be a POD (since I think they're not allowed
to have constructors), but essentially a constant struct.

[dgn, 2016-11-01 14:48:11]

PTAL

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:23:
* reset the {@link ImpressionTracker} or configure it to fire only once.
On 2016/11/01 11:05:43, PEConn wrote:
> Rewrite this comment a bit, maybe:
> This will be called only once if the ImpressionTracker is initialized with
> isOneShot=true or multiple times if not. Further calls can be prevented by
> calling reset().

Thanks! that's better yes

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:41:
private boolean mIsOneShot;
On 2016/11/01 11:05:44, PEConn wrote:
> I think it would be nicer to have two separate classes - a
> MultipleCallImpressionTracker and a OneShotImpressionTracker, but given there
is
> so much code overlap, maybe this is OK.

Ok I'll do all that in a separate CL.

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:47:
public ImpressionTracker(@Nullable View view, Listener listener, boolean
isOneShot) {
On 2016/11/01 11:05:43, PEConn wrote:
> Add some Javadoc for isOneShot?

Done.

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java:10: * A
leaf in the tree, i.e. a single item.
On 2016/11/01 11:05:44, PEConn wrote:
> Change this to 'a *permanent* leaf in the tree' to make it clearer.

Done.

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

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:22:
public abstract class OptionalLeaf extends ChildNode implements Listener {
On 2016/11/01 11:05:44, PEConn wrote:
> Write this as "implements ImpressionTracker.Listener"?

Left the impression tracker as it is.

https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:87:
if (position < 0 || position >= getItemCount()) {
On 2016/11/01 11:05:44, PEConn wrote:
> Shouldn't this just be
> if (position != 0)
> ?

getItemCount can return 0 or 1 depending on #isShown(). So we want position == 0
to be allowed only when isShown() is true. When there are no Items we should not
do any item manipulation and we would throw the exception.

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

https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
components/ntp_snippets/category_info.cc:28: CategoryInfo::~CategoryInfo() =
default;
On 2016/11/01 11:05:44, PEConn wrote:
> Shouldn't these three "blah() = default" go in the header? (genuine question,
> I'm not sure)

Yes, but I get compilation errors because our style checker doesn't like it:

../../components/ntp_snippets/category_info.h:34:3: error: [chromium-style]
Complex constructor has an inlined body.
  CategoryInfo(CategoryInfo&&) = default;


I still don't understand how this is more complex than the previous code, it's
just other plan old booleans.

[PEConn, 2016-11-01 16:00:19]

On 2016/11/01 14:48:11, dgn wrote:
> PTAL
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java
> (right):
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:23:
> * reset the {@link ImpressionTracker} or configure it to fire only once.
> On 2016/11/01 11:05:43, PEConn wrote:
> > Rewrite this comment a bit, maybe:
> > This will be called only once if the ImpressionTracker is initialized with
> > isOneShot=true or multiple times if not. Further calls can be prevented by
> > calling reset().
> 
> Thanks! that's better yes
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:41:
> private boolean mIsOneShot;
> On 2016/11/01 11:05:44, PEConn wrote:
> > I think it would be nicer to have two separate classes - a
> > MultipleCallImpressionTracker and a OneShotImpressionTracker, but given
there
> is
> > so much code overlap, maybe this is OK.
> 
> Ok I'll do all that in a separate CL.
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ImpressionTracker.java:47:
> public ImpressionTracker(@Nullable View view, Listener listener, boolean
> isOneShot) {
> On 2016/11/01 11:05:43, PEConn wrote:
> > Add some Javadoc for isOneShot?
> 
> Done.
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java
> (right):
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java:10: *
A
> leaf in the tree, i.e. a single item.
> On 2016/11/01 11:05:44, PEConn wrote:
> > Change this to 'a *permanent* leaf in the tree' to make it clearer.
> 
> Done.
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java
> (right):
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:22:
> public abstract class OptionalLeaf extends ChildNode implements Listener {
> On 2016/11/01 11:05:44, PEConn wrote:
> > Write this as "implements ImpressionTracker.Listener"?
> 
> Left the impression tracker as it is.
> 
>
https://codereview.chromium.org/2463133002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:87:
> if (position < 0 || position >= getItemCount()) {
> On 2016/11/01 11:05:44, PEConn wrote:
> > Shouldn't this just be
> > if (position != 0)
> > ?
> 
> getItemCount can return 0 or 1 depending on #isShown(). So we want position ==
0
> to be allowed only when isShown() is true. When there are no Items we should
not
> do any item manipulation and we would throw the exception.
> 
>
https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
> File components/ntp_snippets/category_info.cc (right):
> 
>
https://codereview.chromium.org/2463133002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/category_info.cc:28: CategoryInfo::~CategoryInfo() =
> default;
> On 2016/11/01 11:05:44, PEConn wrote:
> > Shouldn't these three "blah() = default" go in the header? (genuine
question,
> > I'm not sure)
> 
> Yes, but I get compilation errors because our style checker doesn't like it:
> 
> ../../components/ntp_snippets/category_info.h:34:3: error: [chromium-style]
> Complex constructor has an inlined body.
>   CategoryInfo(CategoryInfo&&) = default;
> 
> 
> I still don't understand how this is more complex than the previous code, it's
> just other plan old booleans.

LGTM

[dgn, 2016-11-02 14:39:47]

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

[dgn, 2016-11-02 14:39:48]

PTAL

[markusheintz_, 2016-11-02 14:46:02]

On 2016/11/02 14:39:48, dgn wrote:
> PTAL

components/ntp_snippets/* changes LGTM

[dgn, 2016-11-02 16:25:41]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-11-02 16:25:41]

The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org,
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/2463133002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-02 16:26:11]

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-11-02 18:45:30]

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

[commit-bot: I haz the power, 2016-11-02 18:45:32]

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-11-03 10:07:40]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-11-03 10:08:00]

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-11-03 10:49:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-03 10:52:54]

Description was changed from

==========
[NTP Client] Make the MORE button more configurable

Replace the single "has_more_button" from CategoryInfo by more
granular flags: "has_more_action", "has_reload_action",
"has_view_all_action"

Update ActionItem in the UI to have a behaviour similar to the one
of SignInPromo: an optional leaf, that doesn't need to be added and
removed from the parent, does it by itself. Added a parent class to
these 2 to implemnent that behaviour.

BUG=660030
==========

to

==========
[NTP Client] Make the MORE button more configurable

Replace the single "has_more_button" from CategoryInfo by more
granular flags: "has_more_action", "has_reload_action",
"has_view_all_action"

Update ActionItem in the UI to have a behaviour similar to the one
of SignInPromo: an optional leaf, that doesn't need to be added and
removed from the parent, does it by itself. Added a parent class to
these 2 to implemnent that behaviour.

BUG=660030

Committed: https://crrev.com/3ad4278a7d10da110db8738252e83ad8cbc61687
Cr-Commit-Position: refs/heads/master@{#429542}
==========

[commit-bot: I haz the power, 2016-11-03 10:52:55]

Patchset 6 (id:??) landed as
https://crrev.com/3ad4278a7d10da110db8738252e83ad8cbc61687
Cr-Commit-Position: refs/heads/master@{#429542}