📰 Implement offline badge refresh via partial bind

[NTP Client] Implement offline badge refresh via partial bind

The RecyclerView library has a mechanism to perform partial refresh
of views from the adapter via partial binds[1]. Added support for it
in the NewTabPageAdapter and related classes, and rewrote the refresh
of the offline badge using that.

This patch also fixes an issue where the badge was not removed when an
offline page was deleted

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

BUG=616090
Review-Url: https://codereview.chromium.org/2622793003
Cr-Commit-Position: refs/heads/master@{#443236}
Committed: https://chromium.googlesource.com/chromium/src/+/be82572903bce4c189a2c23548343bd03c3af14a

[dgn, 2017-01-10 18:31:37]

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

[commit-bot: I haz the power, 2017-01-10 18:32:25]

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

[dgn, 2017-01-10 18:35:23]

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

[dgn, 2017-01-10 18:35:24]

PTAL.

Note: the flicker of the card in the video is because its height changes. the
other cards move to accomodate for it.

https://codereview.chromium.org/2622793003/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/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:173:
notifyItemChanged(mSuggestions.indexOf(article), "offlineId");
|payload| is currently unused but its presence is used to mark the partial bind.
We could probably switch it for an Integer if we have more uses for partial
bind.

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(left):

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:268:
mOfflineBadge.setVisibility(View.VISIBLE);
The issue was here: the callback did not set the visibility back to View.GONE.

[commit-bot: I haz the power, 2017-01-10 19:07:17]

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

[commit-bot: I haz the power, 2017-01-10 19:07:18]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-01-11 11:01:16]

lgtm

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

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:13:
* have changed with an optional payload object
nitty nit: end with a period.

[dgn, 2017-01-11 12:01:01]

thanks!

[dgn, 2017-01-11 12:01:14]

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

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:13:
* have changed with an optional payload object
On 2017/01/11 11:01:16, Michael van Ouwerkerk wrote:
> nitty nit: end with a period.

Done.

[dgn, 2017-01-11 12:01:28]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-11 12:01:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2622793003/#ps20001
(title: "rebased and added three dots")

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

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-01-11 13:30:00]

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

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

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

[Bernhard Bauer, 2017-01-11 13:58:57]

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

[Bernhard Bauer, 2017-01-11 13:58:58]

https://codereview.chromium.org/2622793003/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/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:173:
notifyItemChanged(mSuggestions.indexOf(article), "offlineId");
Extract "offlineId" into a constant?

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

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java:46:
void onBindViewHolder(NewTabPageViewHolder holder, final int position);
Can we just get rid of this method? It shouldn't be very difficult to implement
the one below and ignore |payloads| if necessary.

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

https://codereview.chromium.org/2622793003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:32:
void onItemRangeChanged(TreeNode child, int index, int count);
Can we get rid of this method as well? We can have the convenience methods in
ChildNode call the longer one.

[dgn, 2017-01-11 15:10:43]

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

[dgn, 2017-01-11 15:11:14]

Thanks! PTAL

https://codereview.chromium.org/2622793003/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/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:173:
notifyItemChanged(mSuggestions.indexOf(article), "offlineId");
On 2017/01/11 13:58:57, Bernhard Bauer wrote:
> Extract "offlineId" into a constant?

Done.

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

https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java:46:
void onBindViewHolder(NewTabPageViewHolder holder, final int position);
On 2017/01/11 13:58:57, Bernhard Bauer wrote:
> Can we just get rid of this method? It shouldn't be very difficult to
implement
> the one below and ignore |payloads| if necessary.

Done.

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

https://codereview.chromium.org/2622793003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:32:
void onItemRangeChanged(TreeNode child, int index, int count);
On 2017/01/11 13:58:57, Bernhard Bauer wrote:
> Can we get rid of this method as well? We can have the convenience methods in
> ChildNode call the longer one.

Done.

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

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:142:
public void onBindViewHolder(NewTabPageViewHolder holder, final int position) {
this one is abstract on RecyclerView.Adapter so I have to implement it. In the
default implemtation the one above calls the one without payload. So here we
flip that since we accept payload everywhere.

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

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-01-11 15:30:14]

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

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:30:
if (mParent != null) mParent.onItemRangeChanged(this, index, count, null);
Just call notifyItemRangeChanged(index, count, null) so everything goes through
the same method?

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:228:
if (!payloads.isEmpty() && article.equals(mArticle)) {
You could check whether |payloads| contains |PARTIAL_UPDATE_OFFLINE_ID|?

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

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:82:
mInnerNode.onBindViewHolder(holder, 0, null);
Is the payload allowed to be null? If it is, it would be nice to have
SnippetArticleViewHolder accept that; if it isn't, we should pass an empty list
here. Right now it looks a bit like this only happens not to crash by accident
:)

[commit-bot: I haz the power, 2017-01-11 16:20:49]

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

[commit-bot: I haz the power, 2017-01-11 16:20:50]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-01-11 18:22:57]

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

[commit-bot: I haz the power, 2017-01-11 18:23: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, 2017-01-11 19:09:54]

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

[commit-bot: I haz the power, 2017-01-11 19:09:55]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-01-11 19:28:47]

PTAL

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

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:30:
if (mParent != null) mParent.onItemRangeChanged(this, index, count, null);
On 2017/01/11 15:30:14, Bernhard Bauer wrote:
> Just call notifyItemRangeChanged(index, count, null) so everything goes
through
> the same method?

Done.

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:228:
if (!payloads.isEmpty() && article.equals(mArticle)) {
On 2017/01/11 15:30:14, Bernhard Bauer wrote:
> You could check whether |payloads| contains |PARTIAL_UPDATE_OFFLINE_ID|?

Done.

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

https://codereview.chromium.org/2622793003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:82:
mInnerNode.onBindViewHolder(holder, 0, null);
On 2017/01/11 15:30:14, Bernhard Bauer wrote:
> Is the payload allowed to be null? If it is, it would be nice to have
> SnippetArticleViewHolder accept that; if it isn't, we should pass an empty
list
> here. Right now it looks a bit like this only happens not to crash by accident
> :)

Oh right, it should be an empty list. Thanks!

[Bernhard Bauer, 2017-01-12 10:07:50]

lgtm

[dgn, 2017-01-12 10:27:58]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-12 10:27:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2622793003/#ps60001
(title: "address comments")

[dgn, 2017-01-12 10:28:05]

The CQ bit was unchecked by dgn@chromium.org

[commit-bot: I haz the power, 2017-01-12 10:28:08]

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

[dgn, 2017-01-12 15:36:49]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2017-01-12 15:37:13]

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

[dgn, 2017-01-12 15:37:25]

Will add tests separately, to cover also the offline feature in another patch.

[commit-bot: I haz the power, 2017-01-12 15:42:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484235409787290,
"parent_rev": "48c86fea4f973ef4eb12e61085ad9bfdd83996bf", "commit_rev":
"be82572903bce4c189a2c23548343bd03c3af14a"}

[commit-bot: I haz the power, 2017-01-12 15:42:49]

Description was changed from

==========
[NTP Client] Implement offline badge refresh via partial bind

The RecyclerView library has a mechanism to perform partial refresh
of views from the adapter via partial binds[1]. Added support for it
in the NewTabPageAdapter and related classes, and rewrote the refresh
of the offline badge using that.

This patch also fixes an issue where the badge was not removed when an
offline page was deleted

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

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

to

==========
[NTP Client] Implement offline badge refresh via partial bind

The RecyclerView library has a mechanism to perform partial refresh
of views from the adapter via partial binds[1]. Added support for it
in the NewTabPageAdapter and related classes, and rewrote the refresh
of the offline badge using that.

This patch also fixes an issue where the badge was not removed when an
offline page was deleted

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

BUG=616090

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

[commit-bot: I haz the power, 2017-01-12 15:42:49]

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