[Suggestions] Remove TreeNode.getSuggestionAt() in favor of a visitor.

[Suggestions] Remove TreeNode.getSuggestionAt() in favor of a visitor.

The method is only used in tests, where we can get better coverage by
using the visitor pattern when verifying section contents (as it allows
e.g. also verifying the specific action of an action item).

This also pushes dependencies on domain-specific code out of the
TreeNode interface.

Also implement stringification for (sub-)trees in terms of a visitor.

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

[Bernhard Bauer, 2017-05-02 17:25:43]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-05-02 17:26:08]

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

[commit-bot: I haz the power, 2017-05-02 17:26:09]

This CL has an open dependency (Issue 2855873002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Bernhard Bauer, 2017-05-03 09:43:06]

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

[commit-bot: I haz the power, 2017-05-03 09:43:22]

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-05-03 11:18:41]

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

[commit-bot: I haz the power, 2017-05-03 11:18:42]

Dry run: 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-05-03 13:47:46]

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

[commit-bot: I haz the power, 2017-05-03 13:47:56]

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-05-08 15:07:28]

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

[commit-bot: I haz the power, 2017-05-08 15:07:53]

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-05-08 16:18:20]

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

[commit-bot: I haz the power, 2017-05-08 16:18:20]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-05-19 12:21:49]

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

[Bernhard Bauer, 2017-05-19 12:21:50]

Please review.

I've been sitting on this CL long enough :)

[dgn, 2017-05-22 13:47:03]

I don't see getSuggestionAt(int) being removed here...

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

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:113:
private static class SectionDescriptor {
I think it would be nice to extract all of that from this test class. That way
we can use it in other tests to check the state of a section (e.g.
SuggestionsSectionTest). Or do you think it gets overly restrictive and enforces
too much to be used outside of here?

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:172:
mInOrder.verify(mVisitor, mVerification).visitHeader();
we can have sections without headers, if the section is the only one. wouldn't
that fail this case?

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:588:
final int category = 42;
nit: use TEST_CATEGORY

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java:182:
stringBuilder.append(String.format("%s - %s%n", mPosition++, description));
Didn't know about %n, really cool :)

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java:186:
describeItem(String.format(template, args));
Nit: I think using String.format raises a lint warning. Use Locale.ROOT to avoid
using the deprecated method?

[Bernhard Bauer, 2017-05-23 16:37:40]

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

[commit-bot: I haz the power, 2017-05-23 16:38:12]

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-05-23 16:40:27]

On 2017/05/22 13:47:03, dgn wrote:
> I don't see getSuggestionAt(int) being removed here...

Argh, I must have undone that change (probably for rebasing) and then forgotten
about it. Sorry about that!

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

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:113:
private static class SectionDescriptor {
On 2017/05/22 13:47:03, dgn wrote:
> I think it would be nice to extract all of that from this test class. That way
> we can use it in other tests to check the state of a section (e.g.
> SuggestionsSectionTest). Or do you think it gets overly restrictive and
enforces
> too much to be used outside of here?

I think it's a good idea, but can we do that in a followup? That's going to
touch quite a lot of code.

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:172:
mInOrder.verify(mVisitor, mVerification).visitHeader();
On 2017/05/22 13:47:03, dgn wrote:
> we can have sections without headers, if the section is the only one. wouldn't
> that fail this case?

That is true, good catch! It only applies to Articles for You though, and the
existing tests would fail too. I've added a test specifically for that case.

https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:588:
final int category = 42;
On 2017/05/22 13:47:03, dgn wrote:
> nit: use TEST_CATEGORY

Done.

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java:186:
describeItem(String.format(template, args));
On 2017/05/22 13:47:03, dgn wrote:
> Nit: I think using String.format raises a lint warning. Use Locale.ROOT to
avoid
> using the deprecated method?

I'm using Locale.US now, because the format strings really are English -- does
that work?

[commit-bot: I haz the power, 2017-05-23 20:12:41]

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

[commit-bot: I haz the power, 2017-05-23 20:12:42]

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, 2017-05-24 10:56:57]

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

[commit-bot: I haz the power, 2017-05-24 10:57: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-05-24 11:30:31]

lgtm

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java:186:
describeItem(String.format(template, args));
On 2017/05/23 16:40:27, Bernhard Bauer wrote:
> On 2017/05/22 13:47:03, dgn wrote:
> > Nit: I think using String.format raises a lint warning. Use Locale.ROOT to
> avoid
> > using the deprecated method?
> 
> I'm using Locale.US now, because the format strings really are English -- does
> that work?

Acknowledged.

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:130:
private int getLastCardPosition() {
nit: move at the end with the other utility functions?

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java
(right):

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:523:
SuggestionsList suggestionList = section.getSuggestionListForTesting();
isn't it possible to use visitor to check that?

[commit-bot: I haz the power, 2017-05-24 12:11:23]

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

[commit-bot: I haz the power, 2017-05-24 12:11:24]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-05-25 12:27:54]

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:130:
private int getLastCardPosition() {
On 2017/05/24 11:30:31, dgn wrote:
> nit: move at the end with the other utility functions?

Done.

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java
(right):

https://codereview.chromium.org/2860463002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:523:
SuggestionsList suggestionList = section.getSuggestionListForTesting();
On 2017/05/24 11:30:31, dgn wrote:
> isn't it possible to use visitor to check that?

Sure, done.

[Bernhard Bauer, 2017-05-25 12:28:54]

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

[commit-bot: I haz the power, 2017-05-25 12:29:02]

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-05-25 12:53:33]

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

[commit-bot: I haz the power, 2017-05-25 12:53:34]

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

[dgn, 2017-05-25 14:09:55]

thanks! lgtm :)

[Bernhard Bauer, 2017-05-25 14:16:38]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-05-25 14:16:38]

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/2860463002/#ps120001
(title: "review")

[commit-bot: I haz the power, 2017-05-25 14:16:57]

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-05-25 15:37:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495721798301330,
"parent_rev": "d19bf77be42160b9b983a185e013714ae8207055", "commit_rev":
"afdce664e395ad89ff43e38ffc38eecb6130d374"}

[commit-bot: I haz the power, 2017-05-25 15:37:38]

Description was changed from

==========
[Suggestions] Remove TreeNode.getSuggestionAt() in favor of a visitor.

The method is only used in tests, where we can get better coverage by
using the visitor pattern when verifying section contents (as it allows
e.g. also verifying the specific action of an action item).

This also pushes dependencies on domain-specific code out of the
TreeNode interface.

Also implement stringification for (sub-)trees in terms of a visitor.

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

to

==========
[Suggestions] Remove TreeNode.getSuggestionAt() in favor of a visitor.

The method is only used in tests, where we can get better coverage by
using the visitor pattern when verifying section contents (as it allows
e.g. also verifying the specific action of an action item).

This also pushes dependencies on domain-specific code out of the
TreeNode interface.

Also implement stringification for (sub-)trees in terms of a visitor.

BUG=616090

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

[commit-bot: I haz the power, 2017-05-25 15:37:39]

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