📰 Fix card style when a neighbour is added or removed

[NTP Client] Fix card style when a neighbour is added or removed

BUG=683972
Review-Url: https://codereview.chromium.org/2651743002
Cr-Commit-Position: refs/heads/master@{#446711}
Committed: https://chromium.googlesource.com/chromium/src/+/60311d47f627e94d7d47dd5548ac684263ff0762

[dgn, 2017-01-23 17:42:01]

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

[dgn, 2017-01-23 17:42:02]

PTAL

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

https://codereview.chromium.org/2651743002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:230:
public void onBindViewHolder(
So this is the meh part: we redefine onBindViewHolder for each subclass that
needs different input parameters. and we have to call the regular implementation
from that.

[Bernhard Bauer, 2017-01-24 11:18:49]

Another way to do this would be to dispatch the partial update for the card
background directly to the CardViewHolder, as this is pretty much independent of
what item it represents. I'm not sure that approach would generalize to other
partial updates we might add in the future though.

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

https://codereview.chromium.org/2651743002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusCardViewHolder.java:69:
super.onBindViewHolder(Collections.emptyList());
This means we will never perform a partial update for a status card, which is
not what we want, right? I think we'll just have to plumb the payload list
through to here.

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

https://codereview.chromium.org/2651743002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:230:
public void onBindViewHolder(
On 2017/01/23 17:42:02, dgn wrote:
> So this is the meh part: we redefine onBindViewHolder for each subclass that
> needs different input parameters. and we have to call the regular
implementation
> from that. 

I wonder whether we can simplify this a bit: We just have a single
onBindViewHolder() method that internally decides whether to do a full or a
partial bind. The override in subclasses always calls the super method, then
does its own binding (again, either full or partial).

[dgn, 2017-01-24 17:11:44]

PTAL

[Bernhard Bauer, 2017-01-24 17:20:05]

lgtm

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

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:164:
if (payloads.contains(PartialUpdateId.CARD_BACKGROUND)) {
Move this into the switch statement?

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java
(right):

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java:23:
/** Marks a request to update a card's background */
Maybe add a comment that this might be dispatched to view holders that are not
cards, but is a no-op in that case.

[dgn, 2017-01-24 20:04:17]

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

[commit-bot: I haz the power, 2017-01-24 20:04:46]

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-24 20:04:59]

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

[commit-bot: I haz the power, 2017-01-24 20:05:13]

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-24 20:09:06]

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

[dgn, 2017-01-24 20:09:53]

PTAL again. I tried to add logic to avoid sending unecessary updates. It's
assumes a lot of things, I'm not sure it's better than just sending the updates.

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

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:164:
if (payloads.contains(PartialUpdateId.CARD_BACKGROUND)) {
On 2017/01/24 17:20:05, Bernhard Bauer wrote:
> Move this into the switch statement?

Ah yes, done

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java
(right):

https://codereview.chromium.org/2651743002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java:23:
/** Marks a request to update a card's background */
On 2017/01/24 17:20:05, Bernhard Bauer wrote:
> Maybe add a comment that this might be dispatched to view holders that are not
> cards, but is a no-op in that case.

Done.

[commit-bot: I haz the power, 2017-01-24 20:09: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, 2017-01-24 20:33:58]

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

[commit-bot: I haz the power, 2017-01-24 20:33:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dgn, 2017-01-25 12:09:02]

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

[commit-bot: I haz the power, 2017-01-25 12:09:13]

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-25 12:41:52]

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

[commit-bot: I haz the power, 2017-01-25 12:41:53]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-01-26 17:13:40]

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

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:265:
// and last items don't change. If the items was a status card it will be
replaced instead.
Hm. If we do this, can we add asserts or tests for these assumptions? If they
fail (maybe at some point in the future), it would be good to learn about it.

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:197:
public static String stringify(TreeNode root) {
Where is this used?

[dgn, 2017-01-27 16:44:50]

PTAL. mostly went back to previous PS

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

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:265:
// and last items don't change. If the items was a status card it will be
replaced instead.
On 2017/01/26 17:13:39, Bernhard Bauer wrote:
> Hm. If we do this, can we add asserts or tests for these assumptions? If they
> fail (maybe at some point in the future), it would be good to learn about it. 

In the end I reverted to the logic in the previous patchset. This is adding
complexity and brittleness for not much added benefit. #prematureoptimisation. I
looked into extracting logic around a "CardCluster" to avoid changing the layout
params too often, but all we would be saving is a few getItemViewType/tree
climbing calls, so ¯\_(ツ)_/¯

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2651743002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:197:
public static String stringify(TreeNode root) {
On 2017/01/26 17:13:40, Bernhard Bauer wrote:
> Where is this used?

nowhere atm but I usually use it when debugging tests.

[Bernhard Bauer, 2017-01-27 16:51:37]

lgtm

[dgn, 2017-01-27 16:55:43]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-27 16:55:55]

thanks!

[commit-bot: I haz the power, 2017-01-27 16:56:18]

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-27 18:19:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485536143020600,
"parent_rev": "a26e439d02baa9514b09c637bf5fa9f6790c4c3e", "commit_rev":
"60311d47f627e94d7d47dd5548ac684263ff0762"}

[commit-bot: I haz the power, 2017-01-27 18:20:05]

Description was changed from

==========
[NTP Client] Fix card style when a neighbour is added or removed

BUG=683972
==========

to

==========
[NTP Client] Fix card style when a neighbour is added or removed

BUG=683972

Review-Url: https://codereview.chromium.org/2651743002
Cr-Commit-Position: refs/heads/master@{#446711}
Committed:
https://chromium.googlesource.com/chromium/src/+/60311d47f627e94d7d47dd5548ac...
==========

[commit-bot: I haz the power, 2017-01-27 18:20:06]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/60311d47f627e94d7d47dd5548ac...