[Android NTP] Move more of the dismissal logic into the tree.

[Android NTP] Move more of the dismissal logic into the tree.

Specifically, add a method getItemDismissalGroup() that unifies
specifying both whether an item can be removed and what other items
should be animated along with it (replacing getItemDismissalSibling()).

The UI-specific logic about whether an item can be dismissed (only non-
peeking cards can be dismissed) stays in the ViewHolder classes.

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

[Bernhard Bauer, 2017-01-06 16:59:22]

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

[commit-bot: I haz the power, 2017-01-06 16:59:45]

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-06 17:24:18]

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

[commit-bot: I haz the power, 2017-01-06 17:24:19]

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

[Bernhard Bauer, 2017-01-10 11:46:29]

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

[commit-bot: I haz the power, 2017-01-10 11:46:46]

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-10 11:52:06]

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

[commit-bot: I haz the power, 2017-01-10 11:52:19]

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-10 12:26:10]

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

[commit-bot: I haz the power, 2017-01-10 12:26:11]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-01-10 13:33:30]

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

[commit-bot: I haz the power, 2017-01-10 13:33:43]

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-10 14:04:08]

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

[commit-bot: I haz the power, 2017-01-10 14:04:09]

Dry run: 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-17 15:37:04]

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

[commit-bot: I haz the power, 2017-01-17 15:37:16]

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-17 15:38:54]

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

[commit-bot: I haz the power, 2017-01-17 15:38:55]

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

[Bernhard Bauer, 2017-01-20 17:00:49]

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

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

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-20 17:02:22]

Description was changed from

==========
[Android NTP] Move more of the dismissal logic into the tree.

Specifically, add a method getItemDismissalGroup() that unifies
specifying both whether an item can be removed and what other items
should be animated along with it (replacing getItemDismissalSibling()).

The UI-specific logic about whether an item can be dismissed (only
non-peeking card) stays in the ViewHolder classes.

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

to

==========
[Android NTP] Move more of the dismissal logic into the tree.

Specifically, add a method getItemDismissalGroup() that unifies
specifying both whether an item can be removed and what other items
should be animated along with it (replacing getItemDismissalSibling()).

The UI-specific logic about whether an item can be dismissed (only non-
peeking cards can be dismissed) stays in the ViewHolder classes.

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

[Bernhard Bauer, 2017-01-20 17:07:09]

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

[Bernhard Bauer, 2017-01-20 17:07:10]

Please take a look!

[commit-bot: I haz the power, 2017-01-20 17:42:42]

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

[commit-bot: I haz the power, 2017-01-20 17:42:43]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-01-23 13:39:09]

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java
(right):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:29:
private Set<Integer> offsetBy(Set<Integer> set, int offset) {
nit: make static?
nitty opinion nit: move to the bottom? I prefer when utility functions or things
not part of the interface are at the bottom :/

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:529:
if (!((NewTabPageViewHolder) viewHolder).isDismissable()) {
I am uncomfortable with the fact that we are calculating the dismissal group
here, and then again right below. The difference only seems to be the
isPeeking() check. Could we split this, have raw checks that are not involving
the dismissal groups made here, then handle the dismissal group being empty
below?

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

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:455:
private Set<Integer> getSectionDismissalRange() {
the concept is a bit strange when missing context, would you mind explaining it
in the doc here?

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java
(left):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java:19:
* The node may notify its parent about changes that happen during
initialization.
oops, thanks

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

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:140:
return true;
the control flow in this function is kind of weird. Possibily an argument for
handling the different suggestion types separately. But anyway, I think handling
download suggestions should move below recent tabs, and fall through instead of
returning true here.

[Bernhard Bauer, 2017-01-24 14:26:34]

PTAL!

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java
(right):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:29:
private Set<Integer> offsetBy(Set<Integer> set, int offset) {
On 2017/01/23 13:39:08, dgn wrote:
> nit: make static?
> nitty opinion nit: move to the bottom? I prefer when utility functions or
things
> not part of the interface are at the bottom :/

Done.

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:529:
if (!((NewTabPageViewHolder) viewHolder).isDismissable()) {
On 2017/01/23 13:39:08, dgn wrote:
> I am uncomfortable with the fact that we are calculating the dismissal group
> here, and then again right below. The difference only seems to be the
> isPeeking() check. Could we split this, have raw checks that are not involving
> the dismissal groups made here, then handle the dismissal group being empty
> below?

That is true. I'm actually not quite sure what to do when the card is peeking --
it may make more sense to still allow dismissing the item here, as that would be
consistent with the case where a peeking card is a dismissal sibling (which we
currently don't check either). Essentially, the isPeeking() check (which is the
only part of dismissal that lives in the UI) would only apply when initiating a
dismissal from the UI (swiping or dismissing from the context menu), but not
later when the context menu item is selected.

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

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:455:
private Set<Integer> getSectionDismissalRange() {
On 2017/01/23 13:39:08, dgn wrote:
> the concept is a bit strange when missing context, would you mind explaining
it
> in the doc here?

Done.

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

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:140:
return true;
On 2017/01/23 13:39:08, dgn wrote:
> the control flow in this function is kind of weird. Possibily an argument for
> handling the different suggestion types separately. But anyway, I think
handling
> download suggestions should move below recent tabs, and fall through instead
of
> returning true here.

Done, and moved to SuggestionsCategoryInfo for easier unit testing.

[dgn, 2017-01-24 16:39:23]

thanks! lgtm

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2617133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:529:
if (!((NewTabPageViewHolder) viewHolder).isDismissable()) {
On 2017/01/24 14:26:34, Bernhard Bauer wrote:
> On 2017/01/23 13:39:08, dgn wrote:
> > I am uncomfortable with the fact that we are calculating the dismissal group
> > here, and then again right below. The difference only seems to be the
> > isPeeking() check. Could we split this, have raw checks that are not
involving
> > the dismissal groups made here, then handle the dismissal group being empty
> > below?
> 
> That is true. I'm actually not quite sure what to do when the card is peeking
--
> it may make more sense to still allow dismissing the item here, as that would
be
> consistent with the case where a peeking card is a dismissal sibling (which we
> currently don't check either). Essentially, the isPeeking() check (which is
the
> only part of dismissal that lives in the UI) would only apply when initiating
a
> dismissal from the UI (swiping or dismissing from the context menu), but not
> later when the context menu item is selected.

Ack. if we don't like this we can always stop the context menu from showing when
we are peeking. Which I think we do already.

[Bernhard Bauer, 2017-01-24 16:43:06]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-24 16:43:17]

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 16:47:30]

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

[commit-bot: I haz the power, 2017-01-24 16:47: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...)

[Bernhard Bauer, 2017-01-25 11:07:10]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-25 11:07:36]

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

[Bernhard Bauer, 2017-01-25 17:48:58]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-01-25 17:48:58]

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/2617133002/#ps180001
(title: "nullable")

[commit-bot: I haz the power, 2017-01-25 17:49:30]

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 19:03:26]

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

[commit-bot: I haz the power, 2017-01-25 19:03:27]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Bernhard Bauer, 2017-01-26 10:19:02]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-01-26 10:19:02]

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/2617133002/#ps200001
(title: "annotation")

[commit-bot: I haz the power, 2017-01-26 10:19:14]

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-26 11:50:28]

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

[commit-bot: I haz the power, 2017-01-26 11:50:29]

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

[Bernhard Bauer, 2017-01-26 12:31:16]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-26 12:31:36]

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-26 14:06:21]

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

[commit-bot: I haz the power, 2017-01-26 14:06:22]

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

[Bernhard Bauer, 2017-01-26 15:11:35]

The CQ bit was checked by bauerb@chromium.org

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

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-26 16:11:19]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485443495588680,
"parent_rev": "47b929861e84d93735a00b76a79faf1c40097078", "commit_rev":
"65a19f0cb5910a85ed5cb992fb7c9e0563b33bab"}

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

Description was changed from

==========
[Android NTP] Move more of the dismissal logic into the tree.

Specifically, add a method getItemDismissalGroup() that unifies
specifying both whether an item can be removed and what other items
should be animated along with it (replacing getItemDismissalSibling()).

The UI-specific logic about whether an item can be dismissed (only non-
peeking cards can be dismissed) stays in the ViewHolder classes.

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

to

==========
[Android NTP] Move more of the dismissal logic into the tree.

Specifically, add a method getItemDismissalGroup() that unifies
specifying both whether an item can be removed and what other items
should be animated along with it (replacing getItemDismissalSibling()).

The UI-specific logic about whether an item can be dismissed (only non-
peeking cards can be dismissed) stays in the ViewHolder classes.

BUG=616090

Review-Url: https://codereview.chromium.org/2617133002
Cr-Commit-Position: refs/heads/master@{#446338}
Committed:
https://chromium.googlesource.com/chromium/src/+/65a19f0cb5910a85ed5cb992fb7c...
==========

[commit-bot: I haz the power, 2017-01-26 16:11:51]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/65a19f0cb5910a85ed5cb992fb7c...