Ntp: use callbacks for partial binding, they can carry more detail.

Ntp: use callbacks for partial binding, they can carry more detail.

BUG=677672
Review-Url: https://codereview.chromium.org/2665983005
Cr-Commit-Position: refs/heads/master@{#447754}
Committed: https://chromium.googlesource.com/chromium/src/+/28b57586a20a4c53cac5fd13d7c0b99b7909aaeb

[Michael van Ouwerkerk, 2017-02-01 11:55:19]

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

[commit-bot: I haz the power, 2017-02-01 11:55:33]

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, 2017-02-01 11:56:07]

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

[Michael van Ouwerkerk, 2017-02-01 11:56:08]

Nicolas, could you take a look please?

[commit-bot: I haz the power, 2017-02-01 13:19:41]

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

[commit-bot: I haz the power, 2017-02-01 13:19:42]

Dry run: 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, 2017-02-01 14:22:02]

https://codereview.chromium.org/2665983005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2665983005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:233:
notifyItemChanged(index, new PartialBindCallback() {
I could see these calls being made as 
notifyItemChanged(index, SnippetArticleViewHolder.offlineBadge())

or something similar. The viewholders would specify the kind of partial bind
callbacks they support through static methods and we can just use that, rather
than having the TreeNodes know about what code the viewholder has to run to
perform a specific update. WDYT?

[Michael van Ouwerkerk, 2017-02-01 15:10:51]

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

[commit-bot: I haz the power, 2017-02-01 15:11:13]

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, 2017-02-01 15:19:46]

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

[Michael van Ouwerkerk, 2017-02-01 15:19:47]

All done. Please take a look.

https://codereview.chromium.org/2665983005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2665983005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:233:
notifyItemChanged(index, new PartialBindCallback() {
On 2017/02/01 14:22:02, dgn wrote:
> I could see these calls being made as 
> notifyItemChanged(index, SnippetArticleViewHolder.offlineBadge())
> 
> or something similar. The viewholders would specify the kind of partial bind
> callbacks they support through static methods and we can just use that, rather
> than having the TreeNodes know about what code the viewholder has to run to
> perform a specific update. WDYT?

I suppose using inner callback classes of the view holders is a little cleaner.
It's all quite verbose for our current use cases, but that's also kind of a Java
thing. Done.

[dgn, 2017-02-01 15:29:40]

thanks! lgtm

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:699:
same(section), eq(1), eq(1), any(PartialBindCallback.class));
verify the right type of callback?

[Bernhard Bauer, 2017-02-01 15:44:11]

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:24:
public abstract void onResult(NewTabPageViewHolder holder);
You don't even need to do this then, do you? Just make an empty subclass and add
a comment that this is to strengthen the typing.

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:58:
public static class UpdateLayoutParamsCallback extends PartialBindCallback {
Oh hey, would it make sense to make this a constant? It doesn't actually have
any state that would require constructing a new one every time.

[commit-bot: I haz the power, 2017-02-01 16:19:04]

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

[commit-bot: I haz the power, 2017-02-01 16:19:05]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-02-02 11:13:55]

All done. Please take another look.

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:24:
public abstract void onResult(NewTabPageViewHolder holder);
On 2017/02/01 15:44:10, Bernhard Bauer wrote:
> You don't even need to do this then, do you? Just make an empty subclass and
add
> a comment that this is to strengthen the typing.

Done.

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:58:
public static class UpdateLayoutParamsCallback extends PartialBindCallback {
On 2017/02/01 15:44:10, Bernhard Bauer wrote:
> Oh hey, would it make sense to make this a constant? It doesn't actually have
> any state that would require constructing a new one every time.

It could be. That would come at a small startup cost, no? Plus, the main reason
for using callbacks is that they can be subclassed and carry additional members
in their instances. I'm not doing that yet in this CL, but will do that in the
next one.

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:699:
same(section), eq(1), eq(1), any(PartialBindCallback.class));
On 2017/02/01 15:29:40, dgn wrote:
> verify the right type of callback?

Done.

[Michael van Ouwerkerk, 2017-02-02 11:14:01]

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

[commit-bot: I haz the power, 2017-02-02 11:14:16]

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

[Bernhard Bauer, 2017-02-02 11:39:05]

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:58:
public static class UpdateLayoutParamsCallback extends PartialBindCallback {
On 2017/02/02 11:13:55, Michael van Ouwerkerk wrote:
> On 2017/02/01 15:44:10, Bernhard Bauer wrote:
> > Oh hey, would it make sense to make this a constant? It doesn't actually
have
> > any state that would require constructing a new one every time.
> 
> It could be. That would come at a small startup cost, no? 

Yes, but only when the class it's in is loaded, which will be when we open a
NTP, I think. There's also the
https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom, which would
create the instance only if it's actually accessed. 

> Plus, the main reason
> for using callbacks is that they can be subclassed and carry additional
members
> in their instances. I'm not doing that yet in this CL, but will do that in the
> next one.

Oh sure, but for those we can create instances as needed. But for callbacks that
have no state at all, it just seems unnecessary.

[Michael van Ouwerkerk, 2017-02-02 12:00:36]

Please take another look.

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2665983005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:58:
public static class UpdateLayoutParamsCallback extends PartialBindCallback {
On 2017/02/02 11:39:05, Bernhard Bauer wrote:
> On 2017/02/02 11:13:55, Michael van Ouwerkerk wrote:
> > On 2017/02/01 15:44:10, Bernhard Bauer wrote:
> > > Oh hey, would it make sense to make this a constant? It doesn't actually
> have
> > > any state that would require constructing a new one every time.
> > 
> > It could be. That would come at a small startup cost, no? 
> 
> Yes, but only when the class it's in is loaded, which will be when we open a
> NTP, I think. There's also the
> https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom, which
would
> create the instance only if it's actually accessed. 
> 
> > Plus, the main reason
> > for using callbacks is that they can be subclassed and carry additional
> members
> > in their instances. I'm not doing that yet in this CL, but will do that in
the
> > next one.
> 
> Oh sure, but for those we can create instances as needed. But for callbacks
that
> have no state at all, it just seems unnecessary.

Done.

[Michael van Ouwerkerk, 2017-02-02 12:01:04]

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

[commit-bot: I haz the power, 2017-02-02 12:01:14]

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

[Bernhard Bauer, 2017-02-02 13:00:01]

lgtm

[Michael van Ouwerkerk, 2017-02-02 13:00:29]

The CQ bit was unchecked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-02 13:00:38]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-02 13:00:38]

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

[commit-bot: I haz the power, 2017-02-02 13:00:43]

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-02-02 13:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486040438151870,
"parent_rev": "f5a30c08e753738a71894ed1f43c07a6d0421dc6", "commit_rev":
"28b57586a20a4c53cac5fd13d7c0b99b7909aaeb"}

[commit-bot: I haz the power, 2017-02-02 13:05:38]

Description was changed from

==========
Ntp: use callbacks for partial binding, they can carry more detail.

BUG=677672
==========

to

==========
Ntp: use callbacks for partial binding, they can carry more detail.

BUG=677672

Review-Url: https://codereview.chromium.org/2665983005
Cr-Commit-Position: refs/heads/master@{#447754}
Committed:
https://chromium.googlesource.com/chromium/src/+/28b57586a20a4c53cac5fd13d7c0...
==========

[commit-bot: I haz the power, 2017-02-02 13:05:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/28b57586a20a4c53cac5fd13d7c0...