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

Issue 2860463002: [Suggestions] Remove TreeNode.getSuggestionAt() in favor of a visitor. (Closed)

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

Description

[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

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Total comments: 10

Patch Set 4 : review #

Patch Set 5 : fix test #

Total comments: 4

Patch Set 6 : review #

Patch Set 7 : review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+485 lines, -254 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/AboveTheFoldItem.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/AllDismissedItem.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Footer.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java View 1 2 3 3 chunks +7 lines, -8 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java View 1 2 3 2 chunks +0 lines, -8 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java View 1 2 3 2 chunks +0 lines, -6 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NodeVisitor.java View 1 2 3 4 5 6 1 chunk +70 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java View 1 2 3 4 chunks +11 lines, -7 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressItem.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java View 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingItem.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java View 1 2 3 4 5 3 chunks +8 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/TreeNode.java View 1 2 3 3 chunks +8 lines, -9 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SectionHeader.java View 1 2 3 4 2 chunks +8 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java View 1 2 3 4 5 3 chunks +6 lines, -7 lines 0 comments Download
M chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java View 1 2 3 2 chunks +0 lines, -19 lines 0 comments Download
M chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java View 1 2 3 4 27 chunks +217 lines, -100 lines 0 comments Download
M chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java View 1 2 3 4 5 10 chunks +32 lines, -26 lines 0 comments Download
M chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java View 1 2 3 3 chunks +68 lines, -57 lines 0 comments Download

Messages

Total messages: 38 (29 generated)
commit-bot: I haz the power
This CL has an open dependency (Issue 2855873002 Patch 1). Please resolve the dependency and ...
3 years, 7 months ago (2017-05-02 17:26:09 UTC) #3
Bernhard Bauer
Please review. I've been sitting on this CL long enough :)
3 years, 7 months ago (2017-05-19 12:21:50 UTC) #15
dgn
I don't see getSuggestionAt(int) being removed here... https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2860463002/diff/40001/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java#newcode113 chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:113: private static ...
3 years, 7 months ago (2017-05-22 13:47:03 UTC) #16
Bernhard Bauer
On 2017/05/22 13:47:03, dgn wrote: > I don't see getSuggestionAt(int) being removed here... Argh, I ...
3 years, 7 months ago (2017-05-23 16:40:27 UTC) #19
dgn
lgtm https://codereview.chromium.org/2860463002/diff/40001/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java 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/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java#newcode186 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: ...
3 years, 7 months ago (2017-05-24 11:30:31 UTC) #24
Bernhard Bauer
https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java (right): https://codereview.chromium.org/2860463002/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java#newcode130 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: ...
3 years, 7 months ago (2017-05-25 12:27:54 UTC) #27
dgn
thanks! lgtm :)
3 years, 7 months ago (2017-05-25 14:09:55 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/2860463002/120001
3 years, 7 months ago (2017-05-25 14:16:57 UTC) #35
commit-bot: I haz the power
3 years, 7 months ago (2017-05-25 15:37:39 UTC) #38
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/afdce664e395ad89ff43e38ffc38...

Powered by Google App Engine
This is Rietveld 408576698