[Android NTP] Move dismissing items into the TreeNode interface.

[Android NTP] Move dismissing items into the TreeNode interface.

BUG=616090
Committed: https://crrev.com/a2b8adc6785ae7989af4fbbf7155547c28e90f32
Cr-Commit-Position: refs/heads/master@{#440091}

[Bernhard Bauer, 2016-12-19 10:54:02]

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

[commit-bot: I haz the power, 2016-12-19 10:54: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, 2016-12-19 11:07:34]

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

[Bernhard Bauer, 2016-12-19 11:07:35]

Please review!

[dgn, 2016-12-19 14:43:09]

Really nice! That stuff is much better in the RV and it merges the 2 paths of
the animated vs the user made dismissal.

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

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:134:
private class ItemTouchCallbacks extends ItemTouchHelper.Callback {
nit: move the inner class declaration at the bottom of the class?

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

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:53:
public interface Parent extends NodeParent {
should some TreeNode methods be moved to such an interface too? the ones like
getSuggestionAt(). Otherwise, if we are fine mixing specific method like this,
the method could as well just go in NodeParent.

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:167:
itemRemovedCallback.onResult(suggestion.mTitle);
nit: it can NPE here, if the index doesn't correspond to a suggestion and
remove() returns null, right? checkIndex() earlier maybe?

[Bernhard Bauer, 2016-12-19 17:08:58]

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

[commit-bot: I haz the power, 2016-12-19 17:09:09]

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

[Bernhard Bauer, 2016-12-19 17:09:32]

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

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:134:
private class ItemTouchCallbacks extends ItemTouchHelper.Callback {
On 2016/12/19 14:43:08, dgn wrote:
> nit: move the inner class declaration at the bottom of the class?

Done.

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

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:53:
public interface Parent extends NodeParent {
On 2016/12/19 14:43:09, dgn wrote:
> should some TreeNode methods be moved to such an interface too? the ones like
> getSuggestionAt(). Otherwise, if we are fine mixing specific method like this,
> the method could as well just go in NodeParent.

Well, this interface is to prevent SuggestionsSection from depending on
SectionList directly (as there is already a dependency in the other direction).
I do agree that it would be nice to get rid of the rather domain-specific
getSuggestionAt() in TreeNode and have clients use the method on SuggestionsList
if necessary. In fact, after this CL the only use of it is in tests, but
rewriting the test code to use something else is a bit bigger than what I'd like
to do in this CL.

https://codereview.chromium.org/2570913003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:167:
itemRemovedCallback.onResult(suggestion.mTitle);
On 2016/12/19 14:43:09, dgn wrote:
> nit: it can NPE here, if the index doesn't correspond to a suggestion and
> remove() returns null, right? checkIndex() earlier maybe?

Done.

[commit-bot: I haz the power, 2016-12-19 17:11:00]

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

[commit-bot: I haz the power, 2016-12-19 17:11:01]

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, 2016-12-19 18:17:38]

Rebased after the setParent change. I now renamed the SuggestionsSection.Parent
to Delegate, as it doesn't make sense to have it extend NodeParent. PTAL,
thanks!

[dgn, 2016-12-20 11:53:25]

Thanks! lgtm

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

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:49:
protected void checkIndex(int position) {
nit: use checkIndex in InnerNode then? getChildIndexForPosition,
getStartingOffsetForChildIndex, etc. It's fine in another CL too

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

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:50:
public interface Delegate extends NodeParent {
according to your comment it should not extend NodeParent

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:226:
for (SnippetArticle suggestion : mSuggestionsList) {
nit: SnippetArticle#equals checks the category and the id within category
already. An alternative implementation could just be to call #remove(article) on
the underlying list

[Bernhard Bauer, 2016-12-20 13:32:11]

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

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:49:
protected void checkIndex(int position) {
On 2016/12/20 11:53:25, dgn wrote:
> nit: use checkIndex in InnerNode then? getChildIndexForPosition,
> getStartingOffsetForChildIndex, etc. It's fine in another CL too

Done (in getChildIndexForPosition, not in getStartOffsetForChildIndex, because
that is based on the child index).

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

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:50:
public interface Delegate extends NodeParent {
On 2016/12/20 11:53:25, dgn wrote:
> according to your comment it should not extend NodeParent

Huh, I thought I had removed that. Done now.

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:226:
for (SnippetArticle suggestion : mSuggestionsList) {
On 2016/12/20 11:53:25, dgn wrote:
> nit: SnippetArticle#equals checks the category and the id within category
> already. An alternative implementation could just be to call #remove(article)
on
> the underlying list

Yes, but I don't have a SnippetArticle to compare, as this method is called when
the underlying suggestion has been invalidated, so it doesn't exist anymore.

[Bernhard Bauer, 2016-12-20 13:32:24]

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

[commit-bot: I haz the power, 2016-12-20 13:32:44]

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

[dgn, 2016-12-20 13:42:28]

still lgtm

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

https://codereview.chromium.org/2570913003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:226:
for (SnippetArticle suggestion : mSuggestionsList) {
On 2016/12/20 13:32:10, Bernhard Bauer wrote:
> On 2016/12/20 11:53:25, dgn wrote:
> > nit: SnippetArticle#equals checks the category and the id within category
> > already. An alternative implementation could just be to call
#remove(article)
> on
> > the underlying list
> 
> Yes, but I don't have a SnippetArticle to compare, as this method is called
when
> the underlying suggestion has been invalidated, so it doesn't exist anymore.

ah true! I was looking at the test code :/

[commit-bot: I haz the power, 2016-12-20 13:54:36]

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

[commit-bot: I haz the power, 2016-12-20 13:54:37]

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

[Bernhard Bauer, 2016-12-20 16:59:02]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2016-12-20 16:59: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/2570913003/#ps140001
(title: "test")

[commit-bot: I haz the power, 2016-12-20 16:59:25]

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, 2016-12-20 18:27:32]

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

[commit-bot: I haz the power, 2016-12-20 18:27:33]

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, 2016-12-21 13:22:06]

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

[Bernhard Bauer, 2016-12-21 13:22:07]

Michael, could you take a look at the NewTabPageRecyclerViewTest changes?

[Michael van Ouwerkerk, 2016-12-21 13:29:27]

lgtm

[Bernhard Bauer, 2016-12-21 13:30:08]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2016-12-21 13:30:08]

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

[commit-bot: I haz the power, 2016-12-21 13:30:24]

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, 2016-12-21 14:21:55]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482327008377240,
"parent_rev": "632e7f653b6ada11b32941329fcf97ab9c33021c", "commit_rev":
"f2c2ed0303644ba2c9a77c76717889c05820b8f0"}

[commit-bot: I haz the power, 2016-12-21 14:22:21]

Description was changed from

==========
[Android NTP] Move dismissing items into the TreeNode interface.

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

to

==========
[Android NTP] Move dismissing items into the TreeNode interface.

BUG=616090

Review-Url: https://codereview.chromium.org/2570913003
==========

[commit-bot: I haz the power, 2016-12-21 14:22:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-21 14:23:52]

Description was changed from

==========
[Android NTP] Move dismissing items into the TreeNode interface.

BUG=616090

Review-Url: https://codereview.chromium.org/2570913003
==========

to

==========
[Android NTP] Move dismissing items into the TreeNode interface.

BUG=616090

Committed: https://crrev.com/a2b8adc6785ae7989af4fbbf7155547c28e90f32
Cr-Commit-Position: refs/heads/master@{#440091}
==========

[commit-bot: I haz the power, 2016-12-21 14:23:53]

Patchset 10 (id:??) landed as
https://crrev.com/a2b8adc6785ae7989af4fbbf7155547c28e90f32
Cr-Commit-Position: refs/heads/master@{#440091}