Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(871)

Issue 2622793003: 📰 Implement offline badge refresh via partial bind (Closed)

Created:
3 years, 11 months ago by dgn
Modified:
3 years, 11 months ago
CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 8

Patch Set 2 : rebased and added three dots #

Total comments: 2

Patch Set 3 : address bauerb@'s comment #

Total comments: 7

Patch Set 4 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -73 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java View 1 2 3 2 chunks +9 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java View 1 2 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java View 1 2 3 chunks +12 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java View 1 2 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java View 1 2 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java View 1 2 3 chunks +14 lines, -7 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java View 1 2 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java View 3 chunks +0 lines, -18 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java View 1 2 3 4 chunks +24 lines, -16 lines 0 comments Download
M chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java View 1 2 3 4 chunks +18 lines, -14 lines 0 comments Download

Messages

Total messages: 38 (23 generated)
dgn
PTAL. Note: the flicker of the card in the video is because its height changes. ...
3 years, 11 months ago (2017-01-10 18:35:24 UTC) #4
Michael van Ouwerkerk
lgtm https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java 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/chromium/chrome/browser/ntp/cards/NodeParent.java#newcode13 chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:13: * have changed with an optional payload object ...
3 years, 11 months ago (2017-01-11 11:01:16 UTC) #7
dgn
thanks!
3 years, 11 months ago (2017-01-11 12:01:01 UTC) #8
dgn
https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java 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/chromium/chrome/browser/ntp/cards/NodeParent.java#newcode13 chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeParent.java:13: * have changed with an optional payload object On ...
3 years, 11 months ago (2017-01-11 12:01:14 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2622793003/20001
3 years, 11 months ago (2017-01-11 12:02:40 UTC) #12
commit-bot: I haz the power
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_swarming_rel/builds/98056)
3 years, 11 months ago (2017-01-11 13:30:02 UTC) #14
Bernhard Bauer
https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java 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/chromium/chrome/browser/ntp/cards/SuggestionsSection.java#newcode173 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/chromium/chrome/browser/ntp/cards/TreeNode.java File ...
3 years, 11 months ago (2017-01-11 13:58:58 UTC) #16
dgn
Thanks! PTAL https://codereview.chromium.org/2622793003/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java 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/chromium/chrome/browser/ntp/cards/SuggestionsSection.java#newcode173 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 ...
3 years, 11 months ago (2017-01-11 15:11:14 UTC) #18
Bernhard Bauer
https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java 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/org/chromium/chrome/browser/ntp/cards/ChildNode.java#newcode30 chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:30: if (mParent != null) mParent.onItemRangeChanged(this, index, count, null); Just ...
3 years, 11 months ago (2017-01-11 15:30:14 UTC) #20
dgn
PTAL https://codereview.chromium.org/2622793003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java 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/org/chromium/chrome/browser/ntp/cards/ChildNode.java#newcode30 chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:30: if (mParent != null) mParent.onItemRangeChanged(this, index, count, null); ...
3 years, 11 months ago (2017-01-11 19:28:47 UTC) #27
Bernhard Bauer
lgtm
3 years, 11 months ago (2017-01-12 10:07:50 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2622793003/60001
3 years, 11 months ago (2017-01-12 10:28:08 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2622793003/60001
3 years, 11 months ago (2017-01-12 15:37:13 UTC) #34
dgn
Will add tests separately, to cover also the offline feature in another patch.
3 years, 11 months ago (2017-01-12 15:37:25 UTC) #35
commit-bot: I haz the power
3 years, 11 months ago (2017-01-12 15:42:49 UTC) #38
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/be82572903bce4c189a2c2354834...

Powered by Google App Engine
This is Rietveld 408576698