Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232783002:
Support action button to fetch more content suggestions (Closed)
|
Created:
4 years, 4 months ago by Philipp Keck Modified:
4 years, 4 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@snippetsbridge Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport action button to fetch more content suggestions
Add has_more_button flag to CategoryInfo.
Add FetchMoreSuggestions method to the provider and service interface.
Update all existing provider implementations accordingly.
Provide the new flag and the new method over the SnippetsBridge.
Change ActionListItem to call the new method (or a hard-coded UI
action) depending on the category.
Enable the more-button for the bookmarks-section only.
BUG=632320, 635794
Committed: https://crrev.com/3b2e36375f8c0079121f4988c24ca1c4c6d303a7
Cr-Commit-Position: refs/heads/master@{#411624}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Marc's and Michael's comments #Patch Set 3 : Adjust NewTabPageAdapterTest #Patch Set 4 : Remove FetchMoreSuggestions #Patch Set 5 : Fix NewTabPageAdapterTest #
Total comments: 12
Patch Set 6 : Bernhard's comments #Patch Set 7 : Hard-code the sections that support the more-button in the UI code #Patch Set 8 : Bernhard's new comments #
Total comments: 2
Patch Set 9 : GetSuggestionsForCategory returns empty list if category is unAVAILABLE #Patch Set 10 : Rebase #
Total comments: 5
Patch Set 11 : Refactor NewTabPageAdapterTest #
Total comments: 16
Patch Set 12 : Add has_more_button comments for boolean literals #Patch Set 13 : Remove braces for single-line bodies #Patch Set 14 : Use JavaDoc comments #
Total comments: 4
Patch Set 15 : Indent comments #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 50 (8 generated)
pke@google.com changed reviewers: + bauerb@chromium.org, treib@chromium.org
PTAL OfflinePageButton will follow tomorrow if https://chromiumcodereview.appspot.com/2226733004/ goes through. Note that the NTPSnippetsService also disables the more-button. The reason is that we currently don't (properly) support the more-action. My implementation of FetchMoreSuggestions() simply calls FetchSnippets(true), but I think that should be improved on (not M54-relevant afaik).
Description was changed from ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. BUG=632320,635794 ========== to ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. Enable the more-button for the bookmarks-section only. BUG=632320,635794 ==========
https://codereview.chromium.org/2232783002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.h (right): https://codereview.chromium.org/2232783002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.h:56: // the a "More" button). nit: remove "a" https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:99: // Ignored. NOTREACHED() since we don't expect this to get called? https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:86: // the a "More" button). New suggestions will become available through nit: remove "a" https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:88: virtual void FetchMoreSuggestions(Category category) = 0; Hm. For now, we don't expect this method to ever actually get called, right? All the "more" buttons we'll have in M54 will have special UI actions. Might have been better to leave this out until we actually need it. But since you've already written it, and we're pretty sure we are actually gonna need it, I guess might as well keep it now. https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:93: // the a "More" button). nit: remove "a"
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:36: private ActionListItem mAction; nit: I think we might want to distinguish items, views, and view holders more clearly throughout our code. Maybe rename this to mActionItem. https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:43: return category == knownCategory; Is it useful to have a method to perform such a simple equality check? https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:55: * |category|. This must be initiated by the user (e.g. by clicking the a "More" button). s/the a/a/
pke@google.com changed reviewers: + dgn@chromium.org
dgn@chromium.org: Please review the changes in chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:36: private ActionListItem mAction; On 2016/08/10 10:09:29, Michael van Ouwerkerk wrote: > nit: I think we might want to distinguish items, views, and view holders more > clearly throughout our code. Maybe rename this to mActionItem. Done. mActionListItem for consistency with the full "TextView" suffix elsewhere. https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:43: return category == knownCategory; On 2016/08/10 10:09:29, Michael van Ouwerkerk wrote: > Is it useful to have a method to perform such a simple equality check? I knew some reviewer would ask this ;-) We have similar code here https://codesearch.chromium.org/chromium/src/components/ntp_snippets/category..., that's why created it here. But in the Java case, there is not even any casting involved, so maybe we should get rid of the helper method... WDYT? https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:55: * |category|. This must be initiated by the user (e.g. by clicking the a "More" button). On 2016/08/10 10:09:29, Michael van Ouwerkerk wrote: > s/the a/a/ Done. https://codereview.chromium.org/2232783002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.h (right): https://codereview.chromium.org/2232783002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.h:56: // the a "More" button). On 2016/08/10 09:40:30, Marc Treib wrote: > nit: remove "a" Done. https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:99: // Ignored. On 2016/08/10 09:40:31, Marc Treib wrote: > NOTREACHED() > since we don't expect this to get called? No. We will have global reload actions. Currently that's only *maybe* through the status card, depending on UX decisions. But after M54 we will introduce a global refresh actions that just needs to go out to all providers. So this must ignore, not fail. https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:86: // the a "More" button). New suggestions will become available through On 2016/08/10 09:40:31, Marc Treib wrote: > nit: remove "a" Done. https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:88: virtual void FetchMoreSuggestions(Category category) = 0; On 2016/08/10 09:40:31, Marc Treib wrote: > Hm. For now, we don't expect this method to ever actually get called, right? All > the "more" buttons we'll have in M54 will have special UI actions. Might have > been better to leave this out until we actually need it. > But since you've already written it, and we're pretty sure we are actually gonna > need it, I guess might as well keep it now. Acknowledged. We could, instead, put sth. like a notreached in the else-branch here: https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... But actually, I think we will need this: Our status cards will become per-section (or are already). We will have a "All Read --> Reload" card under the "Articles for you" header. In M55, this will be replaced with a "All Read, nothing here" card and the usual "More" button under the status-empty-card. But for M54, it's the Reload-button on the card. And instead of calling the static FetchSnippets across the bridge, we might as well trigger the section-specific FetchMoreSuggestion as it's supposed to be. (Although they end up in the same place, effectively). https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:93: // the a "More" button). On 2016/08/10 09:40:31, Marc Treib wrote: > nit: remove "a" Done.
https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:99: // Ignored. On 2016/08/10 13:14:46, Philipp Keck wrote: > On 2016/08/10 09:40:31, Marc Treib wrote: > > NOTREACHED() > > since we don't expect this to get called? > > No. We will have global reload actions. Currently that's only *maybe* through > the status card, depending on UX decisions. But after M54 we will introduce a > global refresh actions that just needs to go out to all providers. So this must > ignore, not fail. But "refresh" is not quite the same as "fetch more".
https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:99: // Ignored. On 2016/08/10 13:14:46, Philipp Keck wrote: > On 2016/08/10 09:40:31, Marc Treib wrote: > > NOTREACHED() > > since we don't expect this to get called? > > No. We will have global reload actions. Currently that's only *maybe* through > the status card, depending on UX decisions. But after M54 we will introduce a > global refresh actions that just needs to go out to all providers. So this must > ignore, not fail. But "refresh" is not quite the same as "fetch more".
https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:43: return category == knownCategory; On 2016/08/10 13:14:46, Philipp Keck wrote: > On 2016/08/10 10:09:29, Michael van Ouwerkerk wrote: > > Is it useful to have a method to perform such a simple equality check? > > I knew some reviewer would ask this ;-) > > We have similar code here > https://codesearch.chromium.org/chromium/src/components/ntp_snippets/category..., > that's why created it here. But in the Java case, there is not even any casting > involved, so maybe we should get rid of the helper method... WDYT? I see, thanks for the explanation. I think this particular method in Java is redundant and obscures what is happening.
https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:43: return category == knownCategory; On 2016/08/10 13:21:09, Michael van Ouwerkerk wrote: > On 2016/08/10 13:14:46, Philipp Keck wrote: > > On 2016/08/10 10:09:29, Michael van Ouwerkerk wrote: > > > Is it useful to have a method to perform such a simple equality check? > > > > I knew some reviewer would ask this ;-) > > > > We have similar code here > > > https://codesearch.chromium.org/chromium/src/components/ntp_snippets/category..., > > that's why created it here. But in the Java case, there is not even any > casting > > involved, so maybe we should get rid of the helper method... WDYT? > > I see, thanks for the explanation. I think this particular method in Java is > redundant and obscures what is happening. True. I removed the method. https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:99: // Ignored. On 2016/08/10 13:19:04, Marc Treib wrote: > On 2016/08/10 13:14:46, Philipp Keck wrote: > > On 2016/08/10 09:40:31, Marc Treib wrote: > > > NOTREACHED() > > > since we don't expect this to get called? > > > > No. We will have global reload actions. Currently that's only *maybe* through > > the status card, depending on UX decisions. But after M54 we will introduce a > > global refresh actions that just needs to go out to all providers. So this > must > > ignore, not fail. > > But "refresh" is not quite the same as "fetch more". True. I removed the whole FetchMoreSuggestions thing for now, as it won't be used. Lots of ignore methods and such will just be confusing for M54. Will add it back in after the feature freeze.
https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:32: public int getCategory() { This doesn't need to be public; it's only used by the ViewHolder which already has access. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:48: } else if (category == KnownCategories.OFFLINE_PAGES) { I'm not very happy about handling the known categories deep in the UI code here. For example, if someone were to add a new category, how would they know they'd have to update this part of the code? Would it work to plumb this back to the provider the category came from and have that one deal with the action? That would keep everything specific to a category in a single place. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:53: "FATAL: AMore action called on a dynamically added section: %d", Nit: "More". Also, "FATAL" is already included in the log level. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: // Special case: The UI hides the suggestions in order to show a status card. Wouldn't this case be contained in the `numSuggestions == 0` case below?
https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:32: public int getCategory() { On 2016/08/10 17:14:41, Bernhard Bauer wrote: > This doesn't need to be public; it's only used by the ViewHolder which already > has access. Done. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:48: } else if (category == KnownCategories.OFFLINE_PAGES) { On 2016/08/10 17:14:40, Bernhard Bauer wrote: > I'm not very happy about handling the known categories deep in the UI code here. > For example, if someone were to add a new category, how would they know they'd > have to update this part of the code? The way they currently find out is this: They set the "hasMoreButton" flag for their category to true for the first time, the button shows up on the UI, they click it, and the "wtf" below kicks in. Chrome will crash, they will look why, and then they know. In early M55, we will replace the "wtf" below with a method on the provider that's being called. That method was implemented earlier in this CL, but it's not there because the design isn't finalized and the method isn't needed at the moment. When that's implemented or when we're designing it, we will again consider your suggestion: > Would it work to plumb this back to the provider the category came from and have > that one deal with the action? That would keep everything specific to a category > in a single place. Yes, that would work, but would not necessarily be nicer. The bookmarks and Downloads Home need to be opened from Java code, so we would need to direct those things back across the bridge, so providers would have to access the SnippetsBridge (or some other bridge that has access to the currently open tab) directly (instead of through the service). This would add quite some indirection (from the button click in Java, across the bridge, through the service, into the provider, back through the bridge and to some Java call), whereas now the code sits where one might expect it: where the UI code for the button is. Marc and I thought about both solutions and decided for this one (at least for now) because it's simpler. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:53: "FATAL: AMore action called on a dynamically added section: %d", On 2016/08/10 17:14:41, Bernhard Bauer wrote: > Nit: "More". Also, "FATAL" is already included in the log level. Done. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: // Special case: The UI hides the suggestions in order to show a status card. On 2016/08/10 17:14:41, Bernhard Bauer wrote: > Wouldn't this case be contained in the `numSuggestions == 0` case below? Yes, but not here. When the status changes to something unavailable, the UI is cleared. The UI code does that. And the ContentSuggestionsService also clears the cache. However, the FakeSnippetsSource does not do that. I could, of course, implement it to do that. And additionally make it throw away updates if the status is not an available one. But I want to avoid replicating all the backend logic in this FakeSnippetsSource(), and instead I want to focus on the actual expectations -- what needs to be displayed according to UX in this case. When the status is unavailable, the getSuggestionsForCategory() may return whatever (empty list, non-empty list) and it should be ignored. And because the FakeSnippetsSource sometimes does return a non-empty list, the case is handled separately here.
After talking to Marc, I changed the code so that the categories which support a more-button are now hard-coded in the UI code, so the info.hasMoreButton() value is ignored for now. This is because the UI also handles all of the more-actions, and if it doesn't have a hard-coded action available, it crashes the application. So the backend has no influence anyway. I will change this back after M54, together with the backend handling some (or all) of the more actions.
lgtm
https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:48: } else if (category == KnownCategories.OFFLINE_PAGES) { On 2016/08/10 17:34:45, Philipp Keck wrote: > On 2016/08/10 17:14:40, Bernhard Bauer wrote: > > I'm not very happy about handling the known categories deep in the UI code > here. > > For example, if someone were to add a new category, how would they know they'd > > have to update this part of the code? > > The way they currently find out is this: They set the "hasMoreButton" flag for > their category to true for the first time, the button shows up on the UI, they > click it, and the "wtf" below kicks in. Chrome will crash, they will look why, > and then they know. Yes, but ideally we'd have something that already fails at compile time. > In early M55, we will replace the "wtf" below with a method on the provider > that's being called. That method was implemented earlier in this CL, but it's > not there because the design isn't finalized and the method isn't needed at the > moment. When that's implemented or when we're designing it, we will again > consider your suggestion: > > Would it work to plumb this back to the provider the category came from and > have > > that one deal with the action? That would keep everything specific to a > category > > in a single place. > Yes, that would work, but would not necessarily be nicer. The bookmarks and > Downloads Home need to be opened from Java code, so we would need to direct > those things back across the bridge, so providers would have to access the > SnippetsBridge (or some other bridge that has access to the currently open tab) > directly (instead of through the service). This would add quite some indirection > (from the button click in Java, across the bridge, through the service, into the > provider, back through the bridge and to some Java call), whereas now the code > sits where one might expect it: where the UI code for the button is. > > Marc and I thought about both solutions and decided for this one (at least for > now) because it's simpler. Hm, I'm still not super happy, but I guess if we are going to reconsider this post-M54 anyway, I'm fine with it for now. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: // Special case: The UI hides the suggestions in order to show a status card. On 2016/08/10 17:34:45, Philipp Keck wrote: > On 2016/08/10 17:14:41, Bernhard Bauer wrote: > > Wouldn't this case be contained in the `numSuggestions == 0` case below? > > Yes, but not here. > When the status changes to something unavailable, the UI is cleared. The UI code > does that. And the ContentSuggestionsService also clears the cache. However, the > FakeSnippetsSource does not do that. I could, of course, implement it to do > that. And additionally make it throw away updates if the status is not an > available one. > But I want to avoid replicating all the backend logic in this > FakeSnippetsSource(), and instead I want to focus on the actual expectations -- > what needs to be displayed according to UX in this case. > > When the status is unavailable, the getSuggestionsForCategory() may return > whatever (empty list, non-empty list) and it should be ignored. And because the > FakeSnippetsSource sometimes does return a non-empty list, the case is handled > separately here. Hm... first of all, we should probably document that contract in SuggestionsSource. Then, if that is the contract, we can directly reflect that in the code: if the status is unavailable, we expect the UI to ignore any suggestions it would get, so in that case we set numSuggestions to 0, otherwise to the actual number of suggestions.
https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionListItem.java:48: } else if (category == KnownCategories.OFFLINE_PAGES) { On 2016/08/11 10:40:17, Bernhard Bauer wrote: > On 2016/08/10 17:34:45, Philipp Keck wrote: > > On 2016/08/10 17:14:40, Bernhard Bauer wrote: > > > I'm not very happy about handling the known categories deep in the UI code > > here. > > > For example, if someone were to add a new category, how would they know > they'd > > > have to update this part of the code? > > > > The way they currently find out is this: They set the "hasMoreButton" flag for > > their category to true for the first time, the button shows up on the UI, they > > click it, and the "wtf" below kicks in. Chrome will crash, they will look why, > > and then they know. > > Yes, but ideally we'd have something that already fails at compile time. > > > In early M55, we will replace the "wtf" below with a method on the provider > > that's being called. That method was implemented earlier in this CL, but it's > > not there because the design isn't finalized and the method isn't needed at > the > > moment. When that's implemented or when we're designing it, we will again > > consider your suggestion: > > > Would it work to plumb this back to the provider the category came from and > > have > > > that one deal with the action? That would keep everything specific to a > > category > > > in a single place. > > Yes, that would work, but would not necessarily be nicer. The bookmarks and > > Downloads Home need to be opened from Java code, so we would need to direct > > those things back across the bridge, so providers would have to access the > > SnippetsBridge (or some other bridge that has access to the currently open > tab) > > directly (instead of through the service). This would add quite some > indirection > > (from the button click in Java, across the bridge, through the service, into > the > > provider, back through the bridge and to some Java call), whereas now the code > > sits where one might expect it: where the UI code for the button is. > > > > Marc and I thought about both solutions and decided for this one (at least for > > now) because it's simpler. > > Hm, I'm still not super happy, but I guess if we are going to reconsider this > post-M54 anyway, I'm fine with it for now. Acknowledged. https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: // Special case: The UI hides the suggestions in order to show a status card. On 2016/08/11 10:40:17, Bernhard Bauer wrote: > On 2016/08/10 17:34:45, Philipp Keck wrote: > > On 2016/08/10 17:14:41, Bernhard Bauer wrote: > > > Wouldn't this case be contained in the `numSuggestions == 0` case below? > > > > Yes, but not here. > > When the status changes to something unavailable, the UI is cleared. The UI > code > > does that. And the ContentSuggestionsService also clears the cache. However, > the > > FakeSnippetsSource does not do that. I could, of course, implement it to do > > that. And additionally make it throw away updates if the status is not an > > available one. > > But I want to avoid replicating all the backend logic in this > > FakeSnippetsSource(), and instead I want to focus on the actual expectations > -- > > what needs to be displayed according to UX in this case. > > > > When the status is unavailable, the getSuggestionsForCategory() may return > > whatever (empty list, non-empty list) and it should be ignored. And because > the > > FakeSnippetsSource sometimes does return a non-empty list, the case is handled > > separately here. > > Hm... first of all, we should probably document that contract in > SuggestionsSource. Then, if that is the contract, we can directly reflect that > in the code: if the status is unavailable, we expect the UI to ignore any > suggestions it would get, so in that case we set numSuggestions to 0, otherwise > to the actual number of suggestions. Ok. I'm not entirely sure I understood correctly, there are a number of ways to do this (including to modify the suggestionsSource). I now made the "special case" a regular case with numSuggestions := 0 and documented why this is the correct expectation. Is that what you meant?
https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:96: public List<SnippetArticleListItem> getSuggestionsForCategory(int category) { Actually, could we strengthen the contract and say that it's not allowed to ask for suggestions if the category is unavailable, rather than just declaring the result to be undefined?
https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:96: public List<SnippetArticleListItem> getSuggestionsForCategory(int category) { On 2016/08/11 13:08:52, Bernhard Bauer wrote: > Actually, could we strengthen the contract and say that it's not allowed to ask > for suggestions if the category is unavailable, rather than just declaring the > result to be undefined? We should first of all make the result something that's not undefined. Namely an empty list (and it should be guaranteed to be an empty list). I implemented that now in FakeSuggestionsSource and documented it in the SuggestionsSource interface. And I modified ContentSuggestionsService to fully ensure that, too (it doesn't accept new suggestions anymore if the status is not available). We can still think about strengthening the contract to not allowing calls at all. We could also not allow calls to GetCategoryInfo() if the status is NOT_PROVIDED (and get rid of the base::Optional<> there). So far, we didn't do that because updates to the status could potentially happen asynchronously. While that's very unlikely, it's nicer to have a silent fallback than to crash. @treib What do you think?
On 2016/08/11 14:01:07, Philipp Keck wrote: > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java > (right): > > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:96: > public List<SnippetArticleListItem> getSuggestionsForCategory(int category) { > On 2016/08/11 13:08:52, Bernhard Bauer wrote: > > Actually, could we strengthen the contract and say that it's not allowed to > ask > > for suggestions if the category is unavailable, rather than just declaring the > > result to be undefined? > > We should first of all make the result something that's not undefined. Namely an > empty list (and it should be guaranteed to be an empty list). I implemented that > now in FakeSuggestionsSource and documented it in the SuggestionsSource > interface. And I modified ContentSuggestionsService to fully ensure that, too > (it doesn't accept new suggestions anymore if the status is not available). > > We can still think about strengthening the contract to not allowing calls at > all. We could also not allow calls to GetCategoryInfo() if the status is > NOT_PROVIDED (and get rid of the base::Optional<> there). > So far, we didn't do that because updates to the status could potentially happen > asynchronously. While that's very unlikely, it's nicer to have a silent fallback > than to crash. > @treib What do you think? Discussed with Bernhard: As the UI should only receive its callbacks on the UI thread anyway, we also require all the OnNewSuggestions and OnCategoryStatusChanged to happen on the UI thread. So we can make GetCategoryInfo fail a DCHECK if the status is NOT_PROVIDED, and make GetSuggestionsForCategory fail if the status is not AVAILABLE*. I will implement this on another CL, unless Marc objects.
On 2016/08/11 14:20:08, Philipp Keck wrote: > On 2016/08/11 14:01:07, Philipp Keck wrote: > > > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java > > (right): > > > > > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:96: > > public List<SnippetArticleListItem> getSuggestionsForCategory(int category) { > > On 2016/08/11 13:08:52, Bernhard Bauer wrote: > > > Actually, could we strengthen the contract and say that it's not allowed to > > ask > > > for suggestions if the category is unavailable, rather than just declaring > the > > > result to be undefined? > > > > We should first of all make the result something that's not undefined. Namely > an > > empty list (and it should be guaranteed to be an empty list). I implemented > that > > now in FakeSuggestionsSource and documented it in the SuggestionsSource > > interface. And I modified ContentSuggestionsService to fully ensure that, too > > (it doesn't accept new suggestions anymore if the status is not available). > > > > We can still think about strengthening the contract to not allowing calls at > > all. We could also not allow calls to GetCategoryInfo() if the status is > > NOT_PROVIDED (and get rid of the base::Optional<> there). > > So far, we didn't do that because updates to the status could potentially > happen > > asynchronously. While that's very unlikely, it's nicer to have a silent > fallback > > than to crash. > > @treib What do you think? > > Discussed with Bernhard: > As the UI should only receive its callbacks on the UI thread anyway, we also > require all the OnNewSuggestions and OnCategoryStatusChanged to happen on the UI > thread. So we can make GetCategoryInfo fail a DCHECK if the status is > NOT_PROVIDED, and make GetSuggestionsForCategory fail if the status is not > AVAILABLE*. > I will implement this on another CL, unless Marc objects. So the proposal is to make all the getters fail if the corresponding category isn't in the expected state? Hm, I don't much like this kind of interface, where methods may only be called in certain (and frequently changing) situations...
On 2016/08/11 14:44:50, Marc Treib wrote: > On 2016/08/11 14:20:08, Philipp Keck wrote: > > On 2016/08/11 14:01:07, Philipp Keck wrote: > > > > > > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2232783002/diff/140001/chrome/android/java/sr... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:96: > > > public List<SnippetArticleListItem> getSuggestionsForCategory(int category) > { > > > On 2016/08/11 13:08:52, Bernhard Bauer wrote: > > > > Actually, could we strengthen the contract and say that it's not allowed > to > > > ask > > > > for suggestions if the category is unavailable, rather than just declaring > > the > > > > result to be undefined? > > > > > > We should first of all make the result something that's not undefined. > Namely > > an > > > empty list (and it should be guaranteed to be an empty list). I implemented > > that > > > now in FakeSuggestionsSource and documented it in the SuggestionsSource > > > interface. And I modified ContentSuggestionsService to fully ensure that, > too > > > (it doesn't accept new suggestions anymore if the status is not available). > > > > > > We can still think about strengthening the contract to not allowing calls at > > > all. We could also not allow calls to GetCategoryInfo() if the status is > > > NOT_PROVIDED (and get rid of the base::Optional<> there). > > > So far, we didn't do that because updates to the status could potentially > > happen > > > asynchronously. While that's very unlikely, it's nicer to have a silent > > fallback > > > than to crash. > > > @treib What do you think? > > > > Discussed with Bernhard: > > As the UI should only receive its callbacks on the UI thread anyway, we also > > require all the OnNewSuggestions and OnCategoryStatusChanged to happen on the > UI > > thread. So we can make GetCategoryInfo fail a DCHECK if the status is > > NOT_PROVIDED, and make GetSuggestionsForCategory fail if the status is not > > AVAILABLE*. > > I will implement this on another CL, unless Marc objects. > > So the proposal is to make all the getters fail if the corresponding category > isn't in the expected state? Hm, I don't much like this kind of interface, where > methods may only be called in certain (and frequently changing) situations... Why not? What you are describing is the general concept of a precondition :)
https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: private int expectedElementsCount() { Sorry for checking in so late. This is super convenient, but it doesn't make obvious from the test code what the expectation is, so fixing or updating tests might get tricky. For example depending on SnippetsBridge#isCategoryStatusInitOrAvailable sounds a bit meh. I suppose depending on the fake snippet source is ok since it's read only from the point of view of the adapter for now. Could we do something more explicit? Maybe something like expectedCountFor(sectionCount, elementCount, buttonsCount) Or maybe something that works a bit like a builder pattern, and could be used like // manipulate the objects to test // ... expectation = expectationFor().section(/*hasButton=*/false, /*suggestioncount=*/ 42) AssertEquals(expectation.count(), foo.getItemCount()); // some other stuff // ... expectation.section(true, 6); // building on the previous expectations AssertEquals(expectation.count(), foo.getItemCount()); We would be spelling what the recycler view should look like in a clearer way
some drive-by comments https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: private int expectedElementsCount() { On 2016/08/11 16:58:04, dgn wrote: > Sorry for checking in so late. This is super convenient, but it doesn't make > obvious from the test code what the expectation is, so fixing or updating tests > might get tricky. > > For example depending on SnippetsBridge#isCategoryStatusInitOrAvailable sounds a > bit meh. I suppose depending on the fake snippet source is ok since it's read > only from the point of view of the adapter for now. > > Could we do something more explicit? Maybe something like > expectedCountFor(sectionCount, elementCount, buttonsCount) > > Or maybe something that works a bit like a builder pattern, and could be used > like > > // manipulate the objects to test > // ... > expectation = expectationFor().section(/*hasButton=*/false, /*suggestioncount=*/ > 42) > AssertEquals(expectation.count(), foo.getItemCount()); > > // some other stuff > // ... > expectation.section(true, 6); // building on the previous expectations > AssertEquals(expectation.count(), foo.getItemCount()); > > We would be spelling what the recycler view should look like in a clearer way > > Drive-by comment. Such a complicated logic for determining the expected count (used in an assert statement) is usually a smell. The test setup should be pretty clear about the number of expected results. When a test fails, you don't want to have to debug through this function first to figure out how you got to the expected value. I'd vote for keeping this explicit in the test, e.g. write expressions like these in the test: EMPTY_STATE_ELEMENTS_COUNT + 5 /* results in section FOO */ + 1 /* More button */ https://codereview.chromium.org/2232783002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_info.h (right): https://codereview.chromium.org/2232783002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_info.h:28: bool has_more_button); can we use an enum instead? kNoMoreButton, kHasMoreButton is much nicer to read at the calling side.
https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: private int expectedElementsCount() { On 2016/08/11 16:58:04, dgn wrote: > Sorry for checking in so late. This is super convenient, but it doesn't make > obvious from the test code what the expectation is, so fixing or updating tests > might get tricky. > > For example depending on SnippetsBridge#isCategoryStatusInitOrAvailable sounds a > bit meh. I suppose depending on the fake snippet source is ok since it's read > only from the point of view of the adapter for now. > > Could we do something more explicit? Maybe something like > expectedCountFor(sectionCount, elementCount, buttonsCount) > > Or maybe something that works a bit like a builder pattern, and could be used > like > > // manipulate the objects to test > // ... > expectation = expectationFor().section(/*hasButton=*/false, /*suggestioncount=*/ > 42) > AssertEquals(expectation.count(), foo.getItemCount()); > > // some other stuff > // ... > expectation.section(true, 6); // building on the previous expectations > AssertEquals(expectation.count(), foo.getItemCount()); > > We would be spelling what the recycler view should look like in a clearer way > > Ok. That's the issue I contacted you in Hangouts about. I like the builder pattern idea and now implemented something similar (also fluent API): assertItemsFor(section(numElements), section(...), sectionWithMoreButton(...)) This solution doesn't need the inline comments to explain things and has the assertion built-in, which allows it to output the full list of actually present items in case it fails, which greatly helps debugging (so you can immediately see, for example, that there is one article more than you thought there should be, etc.). PTAL https://codereview.chromium.org/2232783002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:123: private int expectedElementsCount() { On 2016/08/12 08:02:39, tschumann wrote: > On 2016/08/11 16:58:04, dgn wrote: > > Sorry for checking in so late. This is super convenient, but it doesn't make > > obvious from the test code what the expectation is, so fixing or updating > tests > > might get tricky. > > > > For example depending on SnippetsBridge#isCategoryStatusInitOrAvailable sounds > a > > bit meh. I suppose depending on the fake snippet source is ok since it's read > > only from the point of view of the adapter for now. > > > > Could we do something more explicit? Maybe something like > > expectedCountFor(sectionCount, elementCount, buttonsCount) > > > > Or maybe something that works a bit like a builder pattern, and could be used > > like > > > > // manipulate the objects to test > > // ... > > expectation = expectationFor().section(/*hasButton=*/false, > /*suggestioncount=*/ > > 42) > > AssertEquals(expectation.count(), foo.getItemCount()); > > > > // some other stuff > > // ... > > expectation.section(true, 6); // building on the previous expectations > > AssertEquals(expectation.count(), foo.getItemCount()); > > > > We would be spelling what the recycler view should look like in a clearer way > > > > > Drive-by comment. Such a complicated logic for determining the expected count > (used in an assert statement) is usually a smell. The test setup should be > pretty clear about the number of expected results. When a test fails, you don't > want to have to debug through this function first to figure out how you got to > the expected value. > > I'd vote for keeping this explicit in the test, e.g. write expressions like > these in the test: EMPTY_STATE_ELEMENTS_COUNT + 5 /* results in section FOO */ + > 1 /* More button */ @Tim, please take a look at the new solution. First, we wanted something a bit less static because we sometimes change things like add a more-button, add a loading indicator, etc. and we always have to update all those numbers, even though the expectation is technically still the same ("it shows the sections I just added"). The solution implemented now, in my opinion, is even more expressive than "EMPTY_STATE_ELEMENTS_COUNT + 5" because it also explains how you get the 5 (namely a section with 1 header, 3 elements and 1 more button -- or a section with 1 header and 4 elements. Also, I think you don't have to debug through the helper methods anymore because they're all rather small and have a telling name, plus the failing assertion gives you usable output.
Also, since there's at least one other CL lining up behind this one, I'd like to do further unit test refactorings in a separate CL. In that CL, I will also add more tests for multi-section (e.g. what happens when you clear section X above section Y, etc). Btw another advantage of having assertItemsFor(...) is that we could extend it beyond stupid numbers, as with plain EMPTY_STATE_ELEMENTS_COUNT + 5 + 6, we can't differentiate that from EMPTY_STATE_ELEMENTS_COUNT + 6 + 5. The assertItemsFor() has the information it needs to check that the right elements are in the right positions, so we could extend it to do that without changing its contract.
On 2016/08/12 08:22:55, Philipp Keck wrote: > Also, since there's at least one other CL lining up behind this one, I'd like to > do further unit test refactorings in a separate CL. In that CL, I will also add > more tests for multi-section (e.g. what happens when you clear section X above > section Y, etc). > > Btw another advantage of having assertItemsFor(...) is that we could extend it > beyond stupid numbers, as with plain EMPTY_STATE_ELEMENTS_COUNT + 5 + 6, we > can't differentiate that from EMPTY_STATE_ELEMENTS_COUNT + 6 + 5. The > assertItemsFor() has the information it needs to check that the right elements > are in the right positions, so we could extend it to do that without changing > its contract. Let's discuss this offline (coming over). I also realize that this is something the London folks should decide. My take is the following: assertions in tests should be explicit. If a test wants to verify a certain number of elements, it should do that explicitly. And if I later change the code so that a different number of elements appears, I want all tests to fail which asserted that before. Maybe the tests simply assert more than they should? Maybe they should just assert the number of elements within a specific section and we should have a single test covering the whole page?
C++ LGTM, didn't look at Java https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95: ContentSuggestionsCardLayout::MINIMAL_CARD, true); Optional: You could add a /* has_more_button */ comment https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:287: ContentSuggestionsCardLayout::FULL_CARD, false); Also here: comment to explain what the "false" means? https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:60: ContentSuggestionsCardLayout::MINIMAL_CARD, false); and here
On 2016/08/12 08:33:55, tschumann wrote: > On 2016/08/12 08:22:55, Philipp Keck wrote: > > Also, since there's at least one other CL lining up behind this one, I'd like > to > > do further unit test refactorings in a separate CL. In that CL, I will also > add > > more tests for multi-section (e.g. what happens when you clear section X above > > section Y, etc). > > > > Btw another advantage of having assertItemsFor(...) is that we could extend it > > beyond stupid numbers, as with plain EMPTY_STATE_ELEMENTS_COUNT + 5 + 6, we > > can't differentiate that from EMPTY_STATE_ELEMENTS_COUNT + 6 + 5. The > > assertItemsFor() has the information it needs to check that the right elements > > are in the right positions, so we could extend it to do that without changing > > its contract. > > Let's discuss this offline (coming over). I also realize that this is something > the London folks should decide. > My take is the following: assertions in tests should be explicit. If a test > wants to verify a certain number of elements, it should do that explicitly. And > if I later change the code so that a different number of elements appears, I > want all tests to fail which asserted that before. > Maybe the tests simply assert more than they should? Maybe they should just > assert the number of elements within a specific section and we should have a > single test covering the whole page? FWIW, I think that maintainability of tests is a Good Thing. If all the tests only have magic numbers in them and they all fail if the code changes, it makes it more likely that the author will just blindly update all the expectations to whatever the code does right now, without actually checking that it makes sense. The downside is that you are now writing code in assertItemsFor() that is non-trivial in of itself, so it a test fails, you can't be sure that the production code is actually at fault. On balance, that is still worth the gain in maintainability to me though. https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. Nit: two spaces before comments. https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:190: if (!IsCategoryStatusAvailable(provider->GetCategoryStatus(category))) { Nit: The if-statement above uses braces for a single-line body as well, but other places in this file don't. It would be good to be consistent (in either direction).
I added the comments that Marc suggested. Along with the refactoring of the way
we handle more-actions, I will implement Tim's suggestion and use an enum
instead. Depending on where we choose to handle the actions (does the UI
directly open native views, or do we pass it back to the provider and back into
the UI), we could have a tri-state enum ("no more button", "more button with
refresh functionality", "more button that opens a native page, which the UI
should know about").
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95:
ContentSuggestionsCardLayout::MINIMAL_CARD, true);
On 2016/08/12 08:44:39, Marc Treib wrote:
> Optional: You could add a /* has_more_button */ comment
Done.
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:287:
ContentSuggestionsCardLayout::FULL_CARD, false);
On 2016/08/12 08:44:39, Marc Treib wrote:
> Also here: comment to explain what the "false" means?
Done.
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):
https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:60:
ContentSuggestionsCardLayout::MINIMAL_CARD, false);
On 2016/08/12 08:44:39, Marc Treib wrote:
> and here
Done.
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. On 2016/08/12 09:03:09, Bernhard Bauer wrote: > Nit: two spaces before comments. "git cl format" removes those and I couldn't find this rule in the style guide. https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2232783002/diff/190001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:190: if (!IsCategoryStatusAvailable(provider->GetCategoryStatus(category))) { On 2016/08/12 09:03:09, Bernhard Bauer wrote: > Nit: The if-statement above uses braces for a single-line body as well, but > other places in this file don't. It would be good to be consistent (in either > direction). Removed them here and just above and just below.
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. On 2016/08/12 09:16:42, Philipp Keck wrote: > On 2016/08/12 09:03:09, Bernhard Bauer wrote: > > Nit: two spaces before comments. > > "git cl format" removes those and I couldn't find this rule in the style guide. See https://google.github.io/styleguide/cppguide.html#Implementation_Comments, "line comments". If git cl format really removes them, this just might be one of those cases where you'll have to revert those changes. (And maybe file a bug against clang-format?)
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. On 2016/08/12 09:16:42, Philipp Keck wrote: > On 2016/08/12 09:03:09, Bernhard Bauer wrote: > > Nit: two spaces before comments. > > "git cl format" removes those and I couldn't find this rule in the style guide. I believe the "two spaces before comments" rule exists only for C++, not for Java. Yay for consistency (though this is certainly one of the smaller problems in that regard...)
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. On 2016/08/12 09:23:38, Marc Treib wrote: > On 2016/08/12 09:16:42, Philipp Keck wrote: > > On 2016/08/12 09:03:09, Bernhard Bauer wrote: > > > Nit: two spaces before comments. > > > > "git cl format" removes those and I couldn't find this rule in the style > guide. > > I believe the "two spaces before comments" rule exists only for C++, not for > Java. Yay for consistency (though this is certainly one of the smaller problems > in that regard...) Oh, that's true, I just defaulted to C++ :)
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:126: int expectedCount = 1; // above-the-fold. On 2016/08/12 09:29:53, Bernhard Bauer wrote: > On 2016/08/12 09:23:38, Marc Treib wrote: > > On 2016/08/12 09:16:42, Philipp Keck wrote: > > > On 2016/08/12 09:03:09, Bernhard Bauer wrote: > > > > Nit: two spaces before comments. > > > > > > "git cl format" removes those and I couldn't find this rule in the style > > guide. > > > > I believe the "two spaces before comments" rule exists only for C++, not for > > Java. Yay for consistency (though this is certainly one of the smaller > problems > > in that regard...) > > Oh, that's true, I just defaulted to C++ :) So I leave as is. I'm okay if they change the formatter, but if it's just for a space, I don't want to keep re-inserting that only because I happened to change a line next to that and the formatter removed the space.
Thanks! looks good! Test lgtm with one tiny nit https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:125: private void assertItemsFor(int... sections) { nit: javadoc comments please. Otherwise IDEs don't see them.
https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/190001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:125: private void assertItemsFor(int... sections) { On 2016/08/12 09:55:34, dgn wrote: > nit: javadoc comments please. Otherwise IDEs don't see them. Done.
https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:127: * to represent it on the UI. For readability, these numbers should be generated with the The continuation of the comment should be indented. Admittedly we're not very consistent about how much exactly, but the two common styles are aligning it with the beginning of the comment on the previous line (i.e. after `@param sections`) or indenting 8 spaces. https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:146: * , use either no section at all (if it is not displayed) or {@link #sectionWithStatusCard()}. Nit: The comma should not the first thing on the line :) But that's probably moot anyway if you change the indentation.
https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:127: * to represent it on the UI. For readability, these numbers should be generated with the On 2016/08/12 10:31:35, Bernhard Bauer wrote: > The continuation of the comment should be indented. Admittedly we're not very > consistent about how much exactly, but the two common styles are aligning it > with the beginning of the comment on the previous line (i.e. after `@param > sections`) or indenting 8 spaces. Done. https://codereview.chromium.org/2232783002/diff/250001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:146: * , use either no section at all (if it is not displayed) or {@link #sectionWithStatusCard()}. On 2016/08/12 10:31:35, Bernhard Bauer wrote: > Nit: The comma should not the first thing on the line :) But that's probably > moot anyway if you change the indentation. Done.
https://codereview.chromium.org/2232783002/diff/270001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/270001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:135: if (expectedCount != actualCount) { assertEquals will do this check for you already :)
lgtm
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org, treib@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2232783002/#ps270001 (title: "Indent comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2232783002/diff/270001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2232783002/diff/270001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:135: if (expectedCount != actualCount) { On 2016/08/12 11:10:43, dgn wrote: > assertEquals will do this check for you already :) This is addressed here: https://codereview.chromium.org/2230303003/diff/60001/chrome/android/junit/sr...
Message was sent while issue was closed.
Description was changed from ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. Enable the more-button for the bookmarks-section only. BUG=632320,635794 ========== to ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. Enable the more-button for the bookmarks-section only. BUG=632320,635794 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. Enable the more-button for the bookmarks-section only. BUG=632320,635794 ========== to ========== Support action button to fetch more content suggestions Add has_more_button flag to CategoryInfo. Add FetchMoreSuggestions method to the provider and service interface. Update all existing provider implementations accordingly. Provide the new flag and the new method over the SnippetsBridge. Change ActionListItem to call the new method (or a hard-coded UI action) depending on the category. Enable the more-button for the bookmarks-section only. BUG=632320,635794 Committed: https://crrev.com/3b2e36375f8c0079121f4988c24ca1c4c6d303a7 Cr-Commit-Position: refs/heads/master@{#411624} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3b2e36375f8c0079121f4988c24ca1c4c6d303a7 Cr-Commit-Position: refs/heads/master@{#411624} |
