Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607333002:
[NTP Suggestions] Updating the suggestions before going below the fold. (Closed)
Description[NTP Suggestions] Updating the suggestions before going below the fold.
Currently the NTP UI updates suggestions only when the current list of
suggestions for the given category is empty. After this CL, suggestions
are also updated for non-empty category. The pre-conditions are:
- the user has not yet scrolled below the fold on the NTP,
- there is no peeking-card animation on the NTP.
BUG=674163
Review-Url: https://codereview.chromium.org/2607333002
Cr-Commit-Position: refs/heads/master@{#443908}
Committed: https://chromium.googlesource.com/chromium/src/+/d14a88c13b200244200ad1fe5298f01a9cc3e52f
Patch Set 1 #Patch Set 2 : Finish the CL #Patch Set 3 : Hide behind a variation parameter #
Total comments: 25
Patch Set 4 : Comments #Patch Set 5 : Minor fixes #
Total comments: 24
Patch Set 6 : Unit-tests #Patch Set 7 : Bernhard's comments #Patch Set 8 : Unit-tests fixed #
Total comments: 21
Patch Set 9 : Comments #
Total comments: 24
Patch Set 10 : Comments #2 #
Total comments: 5
Patch Set 11 : Comments #3 #
Total comments: 22
Patch Set 12 : Comments #4 #
Total comments: 8
Patch Set 13 : Comments #5 #
Total comments: 6
Patch Set 14 : Comments #5 #Patch Set 15 : Fix a unit-test #Messages
Total messages: 75 (35 generated)
jkrcal@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Michael, could you PTAL? It does not work, yet. I updated it because I would like to get further feedback. When I update existing suggestions, I get a debug output like the following: 01-02 16:11:06.788 30575 30575 D cr_Ntp : [SectionList.java:129] Received 10 new suggestions for category 10001. 01-02 16:11:06.788 30575 30575 D cr_NtpCards: [SuggestionsSection.java:256] addSuggestions: current number of suggestions: 10 01-02 16:11:06.789 30575 30575 D cr_NtpCards: [SuggestionsSection.java:263] addSuggestions: Removed duplicates from incoming suggestions. Count changed from 10 to 2 and nothing changes in the UI. It seems like SuggestionsSection.addSuggestions() removes duplicates from the new set and appends the new suggestions to the existing ones. What is the reason? I would like all previous suggestions to be replaced by the new one. What of the existing flows would get broken by this change?
On 2017/01/02 15:32:01, jkrcal wrote: > Michael, could you PTAL? > > It does not work, yet. I updated it because I would like to get further > feedback. When I update existing suggestions, I get a debug output like the > following: > > 01-02 16:11:06.788 30575 30575 D cr_Ntp : [SectionList.java:129] Received 10 > new suggestions for category 10001. > 01-02 16:11:06.788 30575 30575 D cr_NtpCards: [SuggestionsSection.java:256] > addSuggestions: current number of suggestions: 10 > 01-02 16:11:06.789 30575 30575 D cr_NtpCards: [SuggestionsSection.java:263] > addSuggestions: Removed duplicates from incoming suggestions. Count changed from > 10 to 2 > > and nothing changes in the UI. > > It seems like SuggestionsSection.addSuggestions() removes duplicates from the > new set and appends the new suggestions to the existing ones. What is the > reason? I would like all previous suggestions to be replaced by the new one. > What of the existing flows would get broken by this change? This is a left-over of the first version of fetch-more (see https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chro...). I believe that block can be removed now.
On 2017/01/03 09:02:57, tschumann wrote: > On 2017/01/02 15:32:01, jkrcal wrote: > > Michael, could you PTAL? > > > > It does not work, yet. I updated it because I would like to get further > > feedback. When I update existing suggestions, I get a debug output like the > > following: > > > > 01-02 16:11:06.788 30575 30575 D cr_Ntp : [SectionList.java:129] Received 10 > > new suggestions for category 10001. > > 01-02 16:11:06.788 30575 30575 D cr_NtpCards: [SuggestionsSection.java:256] > > addSuggestions: current number of suggestions: 10 > > 01-02 16:11:06.789 30575 30575 D cr_NtpCards: [SuggestionsSection.java:263] > > addSuggestions: Removed duplicates from incoming suggestions. Count changed > from > > 10 to 2 > > > > and nothing changes in the UI. > > > > It seems like SuggestionsSection.addSuggestions() removes duplicates from the > > new set and appends the new suggestions to the existing ones. What is the > > reason? I would like all previous suggestions to be replaced by the new one. > > What of the existing flows would get broken by this change? > This is a left-over of the first version of fetch-more (see > https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chro...). > I believe that block can be removed now. Just double-checked: We pass in the displayed suggestions as already seen IDs here: https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chro... so that should be fine. The only remaining issue is that the direct chrome-reader integration does not properly support sorting out duplicates. However, this is also true for re-fetches and should not be worked-around in the java code. IMO, it's also not worth fixing in another place as we shouldn't use the direct chrome-reader integration anyways. So, feel free to remove.
Thanks, Tim! Michael, I've finished the CL. PTAL!
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2017/01/04 12:01:54, jkrcal wrote: > Thanks, Tim! > > Michael, I've finished the CL. PTAL! crbug.com/674163 now has a recording of how the current CL works.
On 2017/01/04 13:25:08, jkrcal wrote: > On 2017/01/04 12:01:54, jkrcal wrote: > > Thanks, Tim! > > > > Michael, I've finished the CL. PTAL! > > crbug.com/674163 now has a recording of how the current CL works. Additional question to Michael: org.chromium.chrome.browser.ntp.cards.NewTabPageAdapterTest.* chrome_junit_tests fail because I ask about variation params in the constructor of SectionList. What is a safe moment in time to figure out the variation param?
On 2017/01/04 14:31:24, jkrcal wrote: > On 2017/01/04 13:25:08, jkrcal wrote: > > On 2017/01/04 12:01:54, jkrcal wrote: > > > Thanks, Tim! > > > > > > Michael, I've finished the CL. PTAL! > > > > crbug.com/674163 now has a recording of how the current CL works. > > Additional question to Michael: > > org.chromium.chrome.browser.ntp.cards.NewTabPageAdapterTest.* chrome_junit_tests > fail because I ask about variation params in the constructor of SectionList. > What is a safe moment in time to figure out the variation param? Not sure, isn't @EnableFeatures supposed to help with this?
https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:59: * @return Whether the NTP should initially be scrolled below the fold. Please update this comment which was copied from the method above :-) Probably something like your CL description. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:63: FIELD_TRIAL_NAME, PARAM_UPDATE_EXISTING_SUGGESTIONS)); This might be for another CL, but I thought reusing FIELD_TRIAL_NAME "NTPSnippets" makes it hard to do concurrent experiments. Should we continue reusing it like this? https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:49: /** nit: indentation below should be +1 https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:75: /** Maybe add a new line before this, it looks crowded. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:662: // Any impression means that the user has scrolled below the fold. Does this code run when the ntp is initially scrolled below the fold? I _think_ it does. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:44: !CardsVariationParameters.isUpdatingExistingSuggestionsEnabled(); There's an unfortunate mixing of enabled and disabled terminology here. Generally, positive phrasing is easier to read. Would it make sense to change the "disabled" bits to "enabled"? At least the mUpdatingExistingSuggestionsDisabled field might read better as mUpdatingExistingSuggestionsEnabled https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:186: private void assignGlobalIndices(@CategoryInt int category, List<SnippetArticle> suggestions) { assignGlobalPositions?
https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:122: if (mSections.get(category).hasSuggestions() && mUpdatingExistingSuggestionsDisabled) { Do we have test coverage for this new feature?
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:80: /** Updating existing suggestions in this recycler view has been disabled. */ Also an empty line before this one please. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:161: private void disableUpdating() { This seems a bit... involved. I think it would be easier to just directly call a method on the adapter here, and have the adapter forward to the SectionList. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:648: disableUpdating(); And here. A good rule of thumb is to always add an empty line before a comment. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:43: mUpdatingExistingSuggestionsDisabled = Can you add some comments to explain the logic? With the negations it's a bit tricky to follow. Something like, by default we always ignore new suggestions, but if the corresponding experiment flag is set, we accept new suggestions until an impression is triggered or the animation is shown. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:44: !CardsVariationParameters.isUpdatingExistingSuggestionsEnabled(); On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > There's an unfortunate mixing of enabled and disabled terminology here. > Generally, positive phrasing is easier to read. Would it make sense to change > the "disabled" bits to "enabled"? At least the > mUpdatingExistingSuggestionsDisabled field might read better as > mUpdatingExistingSuggestionsEnabled Or maybe get rid of the "enabled"/"disabled" terminology and make it something like "ignore updates"?
Thanks for the comments. I changed the code so that we never update anything that the user has seen previously In particular, seeing the peeking card prohibits to update the first item from the first section, seeing any other card prohibits to update its section completely. No "smooth-animation" discussion needed any more. Do you see any other problem with this approach? As the variation parameter has been negated (as defaults to "false"), this updating is enabled by default. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:59: * @return Whether the NTP should initially be scrolled below the fold. On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > Please update this comment which was copied from the method above :-) Probably > something like your CL description. Done. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:63: FIELD_TRIAL_NAME, PARAM_UPDATE_EXISTING_SUGGESTIONS)); On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > This might be for another CL, but I thought reusing FIELD_TRIAL_NAME > "NTPSnippets" makes it hard to do concurrent experiments. Should we continue > reusing it like this? I think the solution is to allow loading parameters via feature name and not via trial name so that we can decide about splitting experiments into multiple files later (when we create the server-side config). Loading via feature name is possible in c++, needs to be added in VariationsAssociatedData. I have it in my TODO list :) https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:49: /** On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > nit: indentation below should be +1 Removed. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:75: /** On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > Maybe add a new line before this, it looks crowded. Done. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:80: /** Updating existing suggestions in this recycler view has been disabled. */ On 2017/01/04 16:02:31, Bernhard Bauer wrote: > Also an empty line before this one please. Done. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:161: private void disableUpdating() { On 2017/01/04 16:02:31, Bernhard Bauer wrote: > This seems a bit... involved. I think it would be easier to just directly call a > method on the adapter here, and have the adapter forward to the SectionList. Fixed according to offline discussion. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:648: disableUpdating(); On 2017/01/04 16:02:31, Bernhard Bauer wrote: > And here. A good rule of thumb is to always add an empty line before a comment. Done. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:662: // Any impression means that the user has scrolled below the fold. On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > Does this code run when the ntp is initially scrolled below the fold? I _think_ > it does. Do all the signalling from onSnippetBound which seems bullet-proof. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:44: !CardsVariationParameters.isUpdatingExistingSuggestionsEnabled(); On 2017/01/04 16:02:31, Bernhard Bauer wrote: > On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > > There's an unfortunate mixing of enabled and disabled terminology here. > > Generally, positive phrasing is easier to read. Would it make sense to change > > the "disabled" bits to "enabled"? At least the > > mUpdatingExistingSuggestionsDisabled field might read better as > > mUpdatingExistingSuggestionsEnabled > > Or maybe get rid of the "enabled"/"disabled" terminology and make it something > like "ignore updates"? Done. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:186: private void assignGlobalIndices(@CategoryInt int category, List<SnippetArticle> suggestions) { On 2017/01/04 15:03:20, Michael van Ouwerkerk wrote: > assignGlobalPositions? Done.
One missed comment. https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:122: if (mSections.get(category).hasSuggestions() && mUpdatingExistingSuggestionsDisabled) { On 2017/01/04 15:04:45, Michael van Ouwerkerk wrote: > Do we have test coverage for this new feature? I am sorry, I've missed this comment. To be honest, I am a bit scared of writing tests in Java. Can you give me some pointers, please?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:122: if (mSections.get(category).hasSuggestions() && mUpdatingExistingSuggestionsDisabled) { On 2017/01/05 15:06:35, jkrcal wrote: > On 2017/01/04 15:04:45, Michael van Ouwerkerk wrote: > > Do we have test coverage for this new feature? > > I am sorry, I've missed this comment. > To be honest, I am a bit scared of writing tests in Java. Can you give me some > pointers, please? From a quick look, I think you may not need an instrumentation test. I think you could cover the important parts with a modest addition to NewTabPageAdapterTest.java
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:64: FIELD_TRIAL_NAME, PARAM_IGNORE_UPDATES_FOR_EXISTING_SUGGESTIONS)); This defaults to false (i.e. not ignoring updates), which is a change in behavior. Is that what we want? https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:70: private boolean mUpdatingDisabled; This is now unused. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:603: getNewTabPageAdapter().onFirstSectionPeeked(); FTR, this is a different definition of "peeking" than what we use elsewhere (which is: it's the first card, and less than 32dp of it are visible[*]). So, even if that was the desired behavior, there would be the issue that we're using the same term to refer to two different things, but I think this is in fact not the desired behavior, namely if the increased card visibility experiment is _not_ enabled. In that case, no card content will be visible if the NTP is not scrolled, so we would keep the first suggestion around unnecessarily. [*] And yes, according to that definition, the first card never peeks if the increased visibility experiment is enabled, which you can see by the fact that the card never has margins. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:612: private void animatePeeking(View cardView) { Is there a particular reason why you're splitting this out? A comment "// Animate the peeking card" might do the same thing. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:39: private boolean mFirstSectionPeeked; Shouldn't we also reset these when resetting the sections? https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:143: boolean isTheFirstSection = category == mSections.keySet().iterator().next(); Mini-nit: Articles in variable names are not a very common style. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:124: if (itemCount == 0) return null; Would this actually happen? https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:130: mSuggestions.add(first); mSuggestions.subList(1, itemCount).clear()? And then you could probably put this into an `if (itemCount > 1)` and just return first at the end. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:131: notifyItemRangeRemoved(1, itemCount); That's the wrong item count (the second parameter is a count of removed items, not an end index). https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:268: int items = mSuggestionsList.getItemCount(); Nit: numItems? https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:284: + "(to preserve the provided length)."); I feel this message adds more explanation than is necessary to debug the code. If you want to explain what you're doing, a comment is the right place; if you want signal to a developer which branch the code took, you can be more succinct.
PTAL, again! https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:64: FIELD_TRIAL_NAME, PARAM_IGNORE_UPDATES_FOR_EXISTING_SUGGESTIONS)); On 2017/01/05 18:50:38, Bernhard Bauer wrote: > This defaults to false (i.e. not ignoring updates), which is a change in > behavior. Is that what we want? This is intended. This feature is now much less controversial then replacing visible snippets with some non-professional animation :) It is also good to get as much testing in canary / dev as possible. Do you think this is a bad idea? https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:70: private boolean mUpdatingDisabled; On 2017/01/05 18:50:38, Bernhard Bauer wrote: > This is now unused. Done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:603: getNewTabPageAdapter().onFirstSectionPeeked(); On 2017/01/05 18:50:38, Bernhard Bauer wrote: > FTR, this is a different definition of "peeking" than what we use elsewhere > (which is: it's the first card, and less than 32dp of it are visible[*]). > > So, even if that was the desired behavior, there would be the issue that we're > using the same term to refer to two different things, but I think this is in > fact not the desired behavior, namely if the increased card visibility > experiment is _not_ enabled. In that case, no card content will be visible if > the NTP is not scrolled, so we would keep the first suggestion around > unnecessarily. > > [*] And yes, according to that definition, the first card never peeks if the > increased visibility experiment is enabled, which you can see by the fact that > the card never has margins. Good point about mixing definitions! [even though I do not like the current isPeeking() definition] Anyway, this was not super robust (I used the assumption that only the first item can be seen if scroll position == 0). I stick to onSnippetBound(). I do not rely on scrolling but on article position now. Namely, I treat the first position in a special way to maximize chances to replace stuff. I am not sure about the case with non increased card visibility. You have the peeking card animation around that can still show the content. I'd rather err on the side of keeping the first item even though the user did not see any content. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:612: private void animatePeeking(View cardView) { On 2017/01/05 18:50:38, Bernhard Bauer wrote: > Is there a particular reason why you're splitting this out? A comment "// > Animate the peeking card" might do the same thing. Not really, reverted. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:39: private boolean mFirstSectionPeeked; On 2017/01/05 18:50:38, Bernhard Bauer wrote: > Shouldn't we also reset these when resetting the sections? Done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:143: boolean isTheFirstSection = category == mSections.keySet().iterator().next(); On 2017/01/05 18:50:38, Bernhard Bauer wrote: > Mini-nit: Articles in variable names are not a very common style. Done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:124: if (itemCount == 0) return null; On 2017/01/05 18:50:39, Bernhard Bauer wrote: > Would this actually happen? Not needed anymore. Removed. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:130: mSuggestions.add(first); On 2017/01/05 18:50:39, Bernhard Bauer wrote: > mSuggestions.subList(1, itemCount).clear()? And then you could probably put this > into an `if (itemCount > 1)` and just return first at the end. Thanks, done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:131: notifyItemRangeRemoved(1, itemCount); On 2017/01/05 18:50:39, Bernhard Bauer wrote: > That's the wrong item count (the second parameter is a count of removed items, > not an end index). Oh :) Done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:268: int items = mSuggestionsList.getItemCount(); On 2017/01/05 18:50:39, Bernhard Bauer wrote: > Nit: numItems? Done. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:284: + "(to preserve the provided length)."); On 2017/01/05 18:50:39, Bernhard Bauer wrote: > I feel this message adds more explanation than is necessary to debug the code. > If you want to explain what you're doing, a comment is the right place; if you > want signal to a developer which branch the code took, you can be more succinct. Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Finished unit-tests so that they finally pass. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: List<SnippetArticle> copiedSuggestions = new ArrayList<SnippetArticle>(suggestions); I realized one flaw in current code: I need to update mPosition / mGlobalPosition for all the snippets. For this reason, I need to make a deep copy of the list. Will fix it on Monday.
dgn@chromium.org changed reviewers: + dgn@chromium.org
Sorry for chiming in so late. That looks a bit more complicated than I expected, but maybe I am missing something that makes the 2 new sets in SectionList necessary? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:600: getNewTabPageAdapter().onFirstOfSectionSeen(category); At some point the level of indirection gets a bit over the top. It might be better to just expose the section to the suggestion. ActionItem already has a reference to the parent section for example. Then we can just notify the section of its content being bound from SnippetArticleViewHolder#onBindViewHolder. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:602: // This means that >= 1px of the category has been shown. I don't get that comment. More than 1 item instead? Just having the header shown would not trigger it, right? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:39: private final Set<Integer> mSectionsWithFirstItemSeen = new TreeSet<Integer>(); how about setting this info on the section itself, then it can handle updating its list of suggestions without exposing a FooKeepFirst method to SectionList? And when resetting the sections we won't have to care about these value since they will be cleared with the section itself. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: List<SnippetArticle> copiedSuggestions = new ArrayList<SnippetArticle>(suggestions); On 2017/01/06 09:24:12, jkrcal wrote: > I realized one flaw in current code: I need to update mPosition / > mGlobalPosition for all the snippets. For this reason, I need to make a deep > copy of the list. > > Will fix it on Monday. mPosition/mGlobalPosition is already wrong. I guess that can be done more generally in a separate patch. I will have a look at it. I don't get why we need a copy of the list here though. Why can't we just alter that one? once we append the items we throw away the container anyway.
Oh hey, looks like Nicolas had similar comments :) Sorry for the duplication! https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsVariationParameters.java:64: FIELD_TRIAL_NAME, PARAM_IGNORE_UPDATES_FOR_EXISTING_SUGGESTIONS)); On 2017/01/05 19:57:15, jkrcal wrote: > On 2017/01/05 18:50:38, Bernhard Bauer wrote: > > This defaults to false (i.e. not ignoring updates), which is a change in > > behavior. Is that what we want? > > This is intended. This feature is now much less controversial then replacing > visible snippets with some non-professional animation :) It is also good to get > as much testing in canary / dev as possible. > > Do you think this is a bad idea? It's fine, it's just that adding the experiment looked to me a bit like the change in behavior would be behind that experiment, when the experiment actually reverts back to the old behavior. https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:603: getNewTabPageAdapter().onFirstSectionPeeked(); On 2017/01/05 19:57:15, jkrcal wrote: > On 2017/01/05 18:50:38, Bernhard Bauer wrote: > > FTR, this is a different definition of "peeking" than what we use elsewhere > > (which is: it's the first card, and less than 32dp of it are visible[*]). > > > > So, even if that was the desired behavior, there would be the issue that we're > > using the same term to refer to two different things, but I think this is in > > fact not the desired behavior, namely if the increased card visibility > > experiment is _not_ enabled. In that case, no card content will be visible if > > the NTP is not scrolled, so we would keep the first suggestion around > > unnecessarily. > > > > [*] And yes, according to that definition, the first card never peeks if the > > increased visibility experiment is enabled, which you can see by the fact that > > the card never has margins. > > Good point about mixing definitions! [even though I do not like the current > isPeeking() definition] Fair point :) It did make more sense back before we had the increased visibility. > Anyway, this was not super robust (I used the assumption that only the first > item can be seen if scroll position == 0). I stick to onSnippetBound(). I do not > rely on scrolling but on article position now. Namely, I treat the first > position in a special way to maximize chances to replace stuff. > > I am not sure about the case with non increased card visibility. You have the > peeking card animation around that can still show the content. I'd rather err on > the side of keeping the first item even though the user did not see any content. I think that's fine; mostly I'd like to be explicit about our assumptions and intended behavior. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:197: public void onSectionSeen(@CategoryInt int category) { Would it be easier to just make this a single method that takes the position, and have the section list decide what to do? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:93: mSectionsSeen.remove(category); Why do you remove the category when resetting the section, but not when removing it? That would seem a bit more intuitive to me, as we would have the invariant that a category we don't know about wouldn't show up in |mSectionsSeen| or |mSectionsWithFirstItemSeen|. And actually, if we have that invariant, we could even push this into the actual SuggestionSection and just make it two flags on there. Then it could be entirely up to the SuggestionSection to decide how to handle new suggestions. Oh! And in addition, the code in the RecyclerView now doesn't actually look at the state of the UI anymore, so there isn't really a reason why we have to go through it at all! We could just set flags on the SuggestionsList immediately when we bind the ViewHolder and get rid of all the plumbing. Sorry for the back-and-forth here, but that seems like the least complicated solution overall. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:114: Remove the empty line? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:136: && (CardsVariationParameters.ignoreUpdatesForExistingSuggestions() Right now, |mSectionsSeen| contains a category only if a suggestion _other_ than the first one has been shown. That means that if this return true (i.e. we want the more conservative behavior of ignoring _any_ updates for existing suggestions) but only the first card of a section has been shown, we won't actually return, and instead replace all suggestions but the first one. I think we want to add to |mSectionsSeen| when the first suggestion is shown as well? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:124: SnippetArticle first = mSuggestions.get(0); It looks like you don't actually use |first| in the if-statement, so just return it inline below? https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: List<SnippetArticle> copiedSuggestions = new ArrayList<SnippetArticle>(suggestions); On 2017/01/06 09:24:12, jkrcal wrote: > I realized one flaw in current code: I need to update mPosition / > mGlobalPosition for all the snippets. For this reason, I need to make a deep > copy of the list. > > Will fix it on Monday. All of that is kind of broken right now anyway :-/ What happens to the subsequent sections, for example? Their global positions might now also be wrong. I would be very happy if we could get rid of storing (global) positions in the suggestions themselves, and instead always calculate the current value or something. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:156: fail("Not exactly the right snippet at position " + mCurrentIndex + "\n" "Not exactly right" sounds a bit understated. The snippet is wrong, period :)
Thanks for the comments! The CL got simpler. PTAL, again. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:197: public void onSectionSeen(@CategoryInt int category) { On 2017/01/06 14:58:22, Bernhard Bauer wrote: > Would it be easier to just make this a single method that takes the position, > and have the section list decide what to do? Removed both :) https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:600: getNewTabPageAdapter().onFirstOfSectionSeen(category); On 2017/01/06 14:53:53, dgn wrote: > At some point the level of indirection gets a bit over the top. It might be > better to just expose the section to the suggestion. ActionItem already has a > reference to the parent section for example. Then we can just notify the section > of its content being bound from SnippetArticleViewHolder#onBindViewHolder. Indirection removed :) Thanks! https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:602: // This means that >= 1px of the category has been shown. On 2017/01/06 14:53:53, dgn wrote: > I don't get that comment. More than 1 item instead? Just having the header shown > would not trigger it, right? Comment removed, not needed any more (and was unclear, agreed). https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:39: private final Set<Integer> mSectionsWithFirstItemSeen = new TreeSet<Integer>(); On 2017/01/06 14:53:53, dgn wrote: > how about setting this info on the section itself, then it can handle updating > its list of suggestions without exposing a FooKeepFirst method to SectionList? > And when resetting the sections we won't have to care about these value since > they will be cleared with the section itself. Done. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:93: mSectionsSeen.remove(category); On 2017/01/06 14:58:22, Bernhard Bauer wrote: > Why do you remove the category when resetting the section, but not when removing > it? That would seem a bit more intuitive to me, as we would have the invariant > that a category we don't know about wouldn't show up in |mSectionsSeen| or > |mSectionsWithFirstItemSeen|. > > And actually, if we have that invariant, we could even push this into the actual > SuggestionSection and just make it two flags on there. Then it could be entirely > up to the SuggestionSection to decide how to handle new suggestions. > > Oh! And in addition, the code in the RecyclerView now doesn't actually look at > the state of the UI anymore, so there isn't really a reason why we have to go > through it at all! We could just set flags on the SuggestionsList immediately > when we bind the ViewHolder and get rid of all the plumbing. Sorry for the > back-and-forth here, but that seems like the least complicated solution overall. Brilliant, done in SuggestionsList. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:114: On 2017/01/06 14:58:22, Bernhard Bauer wrote: > Remove the empty line? Done. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:136: && (CardsVariationParameters.ignoreUpdatesForExistingSuggestions() On 2017/01/06 14:58:22, Bernhard Bauer wrote: > Right now, |mSectionsSeen| contains a category only if a suggestion _other_ than > the first one has been shown. That means that if this return true (i.e. we want > the more conservative behavior of ignoring _any_ updates for existing > suggestions) but only the first card of a section has been shown, we won't > actually return, and instead replace all suggestions but the first one. I think > we want to add to |mSectionsSeen| when the first suggestion is shown as well? Renamed the variables. Hope this is clearer now. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:124: SnippetArticle first = mSuggestions.get(0); On 2017/01/06 14:58:22, Bernhard Bauer wrote: > It looks like you don't actually use |first| in the if-statement, so just return > it inline below? The method is gone now. https://codereview.chromium.org/2607333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: List<SnippetArticle> copiedSuggestions = new ArrayList<SnippetArticle>(suggestions); On 2017/01/06 14:53:53, dgn wrote: > On 2017/01/06 09:24:12, jkrcal wrote: > > I realized one flaw in current code: I need to update mPosition / > > mGlobalPosition for all the snippets. For this reason, I need to make a deep > > copy of the list. > > > > Will fix it on Monday. > > mPosition/mGlobalPosition is already wrong. I guess that can be done more > generally in a separate patch. I will have a look at it. > > I don't get why we need a copy of the list here though. Why can't we just alter > that one? once we append the items we throw away the container anyway. We could not alter the old one because it is the one owned (and repeatedly served) by the suggestions source. At least the FakeSuggestionsSource. Maybe that's a bug in the fake implementation that should be fixed there and not here :) Anyway, the copying is gone by now as I do not have to touch the old list when doing the job directly in SuggestionsList. And I will not worry about positions in this CL. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:398: /** Should I keep the new tests here (as there are similar tests here) or move them to SuggestionsSectionTest (as the logic lives in SuggestionsSection)?
https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:106: suggestionSeen(position); One more question: is it normal that onBindViewHolder is called way before the item is displayed? (whenever I scroll into item n, this is called for item n+1) This is still erring on the conservative side, so no big deal. And it meets the goal as initially, it is called only for item 0.
Thanks! https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:174: @CategoryStatusEnum int status, boolean replaceExisting) { Add a Javadoc comment, in particular explaining what |replaceExisting| means? https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:79: private boolean mLatterSuggestionsSeen; Grammar nit: I think "latter" means closer to the end than the beginning. Maybe "subsequent"? And also clarify the exact meaning with a comment. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:106: suggestionSeen(position); On 2017/01/09 20:30:29, jkrcal wrote: > One more question: is it normal that onBindViewHolder is called way before the > item is displayed? (whenever I scroll into item n, this is called for item n+1) > > This is still erring on the conservative side, so no big deal. And it meets the > goal as initially, it is called only for item 0. I think that could be to make scrolling a bit smoother, as there will already be a child to scroll into view when it happens. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:109: public void suggestionSeen(int position) { Javadoc for public methods please. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:178: notifyItemRangeInserted(insertionPointIndex, suggestions.size()); You could pull this out of the if-statement, as in the |firstSuggestionKept| case mSuggestions.size() will be 1, right? https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:398: /** On 2017/01/09 20:27:37, jkrcal wrote: > Should I keep the new tests here (as there are similar tests here) or move them > to SuggestionsSectionTest (as the logic lives in SuggestionsSection)? SuggestionSectionTest might be a good places for them, yeah.
https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: mSuggestionsList.setSuggestions(suggestions, replaceExisting); I liked better the approach from patchset 6 with clearAllButFirst() :/ I prefer SuggestionList as "dumb" list that doesn't care much about application logic. seems clearer to me that way. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:312: for (SnippetArticle article : mSuggestionsList.getSuggestions()) { mSuggestionsList is iterable so you can loop over it directly, no need for getSuggestions(). Also I think you meant to iterate over |suggestions| as now you would be iterating over the old suggestions too and updating their offline availability. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:321: public void suggestionSeenForTesting(int position) { nit: @VisibleForTesting
Thanks! Addressed most of the comments. Two questions left. PTAL, again. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:174: @CategoryStatusEnum int status, boolean replaceExisting) { On 2017/01/10 10:31:30, Bernhard Bauer wrote: > Add a Javadoc comment, in particular explaining what |replaceExisting| means? Done. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:79: private boolean mLatterSuggestionsSeen; On 2017/01/10 10:31:30, Bernhard Bauer wrote: > Grammar nit: I think "latter" means closer to the end than the beginning. Maybe > "subsequent"? And also clarify the exact meaning with a comment. Done. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:106: suggestionSeen(position); On 2017/01/10 10:31:30, Bernhard Bauer wrote: > On 2017/01/09 20:30:29, jkrcal wrote: > > One more question: is it normal that onBindViewHolder is called way before the > > item is displayed? (whenever I scroll into item n, this is called for item > n+1) > > > > This is still erring on the conservative side, so no big deal. And it meets > the > > goal as initially, it is called only for item 0. > > I think that could be to make scrolling a bit smoother, as there will already be > a child to scroll into view when it happens. Acknowledged. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:109: public void suggestionSeen(int position) { On 2017/01/10 10:31:30, Bernhard Bauer wrote: > Javadoc for public methods please. Done. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:178: notifyItemRangeInserted(insertionPointIndex, suggestions.size()); On 2017/01/10 10:31:30, Bernhard Bauer wrote: > You could pull this out of the if-statement, as in the |firstSuggestionKept| > case mSuggestions.size() will be 1, right? I do not understand. Do you want me to pull out the line "int insertionPointIndex = mSuggestions.size()" and replace "1" in the if-branch by "insertionPointIndex"? If so, I believe it is clearer as it is but feel free to disagree :) https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: mSuggestionsList.setSuggestions(suggestions, replaceExisting); On 2017/01/10 14:43:06, dgn wrote: > I liked better the approach from patchset 6 with clearAllButFirst() :/ I prefer > SuggestionList as "dumb" list that doesn't care much about application logic. > seems clearer to me that way. Well, after removing the indirection, it is the SuggestionList that knows what has been seen by the user. For this reason, I think it is necessary to have the logic in the list. Another approach is to let some of the upper layers know about what has been seen by the user and implement the logic in there. Do you have some concrete idea which layer it should be and how to let it know without the ugly indirection? https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:312: for (SnippetArticle article : mSuggestionsList.getSuggestions()) { On 2017/01/10 14:43:06, dgn wrote: > mSuggestionsList is iterable so you can loop over it directly, no need for > getSuggestions(). > > Also I think you meant to iterate over |suggestions| as now you would be > iterating over the old suggestions too and updating their offline availability. Done. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:321: public void suggestionSeenForTesting(int position) { On 2017/01/10 14:43:06, dgn wrote: > nit: @VisibleForTesting Done. https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:398: /** On 2017/01/10 10:31:30, Bernhard Bauer wrote: > On 2017/01/09 20:27:37, jkrcal wrote: > > Should I keep the new tests here (as there are similar tests here) or move > them > > to SuggestionsSectionTest (as the logic lives in SuggestionsSection)? > > SuggestionSectionTest might be a good places for them, yeah. Done. I left two of them here because they need interaction of multiple sections which does not make sense in SuggestionsSectionTest.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:178: notifyItemRangeInserted(insertionPointIndex, suggestions.size()); On 2017/01/10 17:47:32, jkrcal wrote: > On 2017/01/10 10:31:30, Bernhard Bauer wrote: > > You could pull this out of the if-statement, as in the |firstSuggestionKept| > > case mSuggestions.size() will be 1, right? > > I do not understand. Do you want me to pull out the line "int > insertionPointIndex = mSuggestions.size()" and replace "1" in the if-branch by > "insertionPointIndex"? If so, I believe it is clearer as it is but feel free to > disagree :) Something like this: int insertionPointIndex = mSuggestions.size(); mSuggestions.addAll(suggestions); if (firstSuggestionKept) { // ... } notifyItemRangeInserted(insertionPointIndex, mSuggestions.size()); The idea being that in both branches we add the suggestions and notify about the number of added suggestions, which we can get by looking at the old size and the new size. Basically, we're keeping the amount of code where the two branches diverge to a minimum. Does that make sense? https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: mSuggestionsList.setSuggestions(suggestions, replaceExisting); On 2017/01/10 17:47:32, jkrcal wrote: > On 2017/01/10 14:43:06, dgn wrote: > > I liked better the approach from patchset 6 with clearAllButFirst() :/ I > prefer > > SuggestionList as "dumb" list that doesn't care much about application logic. > > seems clearer to me that way. > > Well, after removing the indirection, it is the SuggestionList that knows what > has been seen by the user. > For this reason, I think it is necessary to have the logic in the list. > > Another approach is to let some of the upper layers know about what has been > seen by the user and implement the logic in there. Do you have some concrete > idea which layer it should be and how to let it know without the ugly > indirection? I think Nicolas' suggestion was to have the logic in SuggestionsSection, where the rest of the logic about updating suggestions lives too. https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:438: public SnippetArticle getSuggestionAtForTesting(int position) { I'm a bit worried that this method appears very similar to the existing getSuggestionAt() method in _this_ class (which delegates to one of its children), but has different semantics, as it doesn't take the offset of non-suggestion items into account. Could you just use getSuggestionAt() with an offset of 1 in the test? https://codereview.chromium.org/2607333002/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:120: public SectionDescriptor withFirstItem(SnippetArticle firstItem) { This is unused now.
Thanks! Quick response to get better understanding before I get back to coding (I fully agree with the rest of the comments). https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: mSuggestionsList.setSuggestions(suggestions, replaceExisting); On 2017/01/11 10:41:47, Bernhard Bauer wrote: > On 2017/01/10 17:47:32, jkrcal wrote: > > On 2017/01/10 14:43:06, dgn wrote: > > > I liked better the approach from patchset 6 with clearAllButFirst() :/ I > > prefer > > > SuggestionList as "dumb" list that doesn't care much about application > logic. > > > seems clearer to me that way. > > > > Well, after removing the indirection, it is the SuggestionList that knows what > > has been seen by the user. > > For this reason, I think it is necessary to have the logic in the list. > > > > Another approach is to let some of the upper layers know about what has been > > seen by the user and implement the logic in there. Do you have some concrete > > idea which layer it should be and how to let it know without the ugly > > indirection? > > I think Nicolas' suggestion was to have the logic in SuggestionsSection, where > the rest of the logic about updating suggestions lives too. That's agreeable for me. I just do not know how to let it know nicely about impressions as SuggestionsList does not know about SuggestionsSection and I do not want to go via recyclerView -> adapter -> sectionList -> suggestionSection. Is it okay to have a cyclic dependency between suggestionsList and suggestionsSection? Or do you prefer the kind of original approach of section being the observer of the list? https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:438: public SnippetArticle getSuggestionAtForTesting(int position) { On 2017/01/11 10:41:47, Bernhard Bauer wrote: > I'm a bit worried that this method appears very similar to the existing > getSuggestionAt() method in _this_ class (which delegates to one of its > children), but has different semantics, as it doesn't take the offset of > non-suggestion items into account. Could you just use getSuggestionAt() with an > offset of 1 in the test? I am puzzled. I see only SuggestionsList.getSuggestionAt() and nothin with the name getSuggestionAt() in this class. Can you point me to a line number, please? https://codereview.chromium.org/2607333002/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:120: public SectionDescriptor withFirstItem(SnippetArticle firstItem) { On 2017/01/11 10:41:47, Bernhard Bauer wrote: > This is unused now. I use it on lines 458 / 461.
https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: mSuggestionsList.setSuggestions(suggestions, replaceExisting); onBindViewHolder is implemented at each level of the tree, so you could override SuggestionSection#onBindViewHolder with something like: onBindViewHolder(..., int position) { mBoundPosition = Math.max(mSeenPosition, position); super.onBindViewHolder(..., position) } Then later, in setSuggestions if (mBoundPosition == 0) { // clear } else if (mBoundPosition == 1 /* Pos of first suggestion */) { // Clear all but first ... } I'm thinking something like List<SnippetArticle> suggestionList#clearFrom(int position); would be neat, allowing to get read of that if check, and support updating the bottom size()-x if they are not seen yet. But that's also maybe overkill for what we want to do here. https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:438: public SnippetArticle getSuggestionAtForTesting(int position) { On 2017/01/11 10:54:48, jkrcal wrote: > On 2017/01/11 10:41:47, Bernhard Bauer wrote: > > I'm a bit worried that this method appears very similar to the existing > > getSuggestionAt() method in _this_ class (which delegates to one of its > > children), but has different semantics, as it doesn't take the offset of > > non-suggestion items into account. Could you just use getSuggestionAt() with > an > > offset of 1 in the test? > > I am puzzled. I see only SuggestionsList.getSuggestionAt() and nothin with the > name getSuggestionAt() in this class. Can you point me to a line number, please? https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
On 2017/01/11 11:33:25, dgn wrote: > https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java > (right): > > https://codereview.chromium.org/2607333002/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:309: > mSuggestionsList.setSuggestions(suggestions, replaceExisting); > onBindViewHolder is implemented at each level of the tree, so you could override > SuggestionSection#onBindViewHolder with something like: > > onBindViewHolder(..., int position) { > mBoundPosition = Math.max(mSeenPosition, position); > super.onBindViewHolder(..., position) > } > > Then later, in setSuggestions > > if (mBoundPosition == 0) { > // clear > } else if (mBoundPosition == 1 /* Pos of first suggestion */) { > // Clear all but first > ... > } > > I'm thinking something like > > List<SnippetArticle> suggestionList#clearFrom(int position); > > would be neat, allowing to get read of that if check, and support updating the > bottom size()-x if they are not seen yet. But that's also maybe overkill for > what we want to do here. > > https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java > (right): > > https://codereview.chromium.org/2607333002/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:438: > public SnippetArticle getSuggestionAtForTesting(int position) { > On 2017/01/11 10:54:48, jkrcal wrote: > > On 2017/01/11 10:41:47, Bernhard Bauer wrote: > > > I'm a bit worried that this method appears very similar to the existing > > > getSuggestionAt() method in _this_ class (which delegates to one of its > > > children), but has different semantics, as it doesn't take the offset of > > > non-suggestion items into account. Could you just use getSuggestionAt() with > > an > > > offset of 1 in the test? > > > > I am puzzled. I see only SuggestionsList.getSuggestionAt() and nothin with the > > name getSuggestionAt() in this class. Can you point me to a line number, > please? > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Cool, that helps! Thanks for both explanations and for your patience! ;)
PTAL, again. (I am sorry to mix in a minor rebase in the diff :|)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks, I like it! Few questions and nits https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:612: // TODO(mvanouverk): Shouldn't this line go down before ObjectAnimator? This way we track nit: username is wrong. Also if there is an issue for real, can you add a bug link? https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:157: if (getItemCount() != 1) return; can that happen? if not, assert rather than failing silently. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: * Notify the Suggestion that a child on the given |position| has been impressed to the user. nit "impressed": not true. We count impression when 2/3 of a card is visible, while here we would call this when a card is bound, which happens usually before it can be seen at all. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; This will set the flag whatever the actual type of the item here is. It can be a suggestion, a status card, an action item, a loading indicator, or whatever we thing about adding later. If we bind something that is not a suggestion, is that still all right to set the flags? (If you want to check item types: #getItemViewType() and CardViewHolder#isCard(viewType) could be useful) https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:324: * Puts |suggestions| into this section. It can either replace all existing suggestions with nitty mc nitface: in javadoc, we use {@code foo} to put names/variables/etc in monospace and indicate it's not a piece of the prose, rather than |foo|. Your IDE should be able to display the processed javadoc, to check the effect. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:367: for (SnippetArticle article : suggestions) { since you modify mSuggestionsList and not the list passed as input in addAllFilterFirst, you will process one extra suggestion than desired here. Removing the element from |suggestions| before mSuggestions.addAll() would fix it.
https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:272: childSeen(position); Don't you need to call super to do the actual binding? You tested this, right? https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; On 2017/01/13 11:02:32, dgn wrote: > This will set the flag whatever the actual type of the item here is. It can be a > suggestion, a status card, an action item, a loading indicator, or whatever we > thing about adding later. Oh, good catch! In fact, the first item will in fact be the header, no? > If we bind something that is not a suggestion, is that still all right to set > the flags? > > (If you want to check item types: #getItemViewType() and > CardViewHolder#isCard(viewType) could be useful) https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:348: int itemCount = getSuggestionsCount(); This isn't used anymore.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Thanks, PTAL, again! https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:612: // TODO(mvanouverk): Shouldn't this line go down before ObjectAnimator? This way we track On 2017/01/13 11:02:32, dgn wrote: > nit: username is wrong. Also if there is an issue for real, can you add a bug > link? This anyway does not belong to this CL. I put a note elsewhere and file a bug (or fix it directly) if it is a problem. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:157: if (getItemCount() != 1) return; On 2017/01/13 11:02:32, dgn wrote: > can that happen? if not, assert rather than failing silently. Done. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:272: childSeen(position); On 2017/01/13 12:13:00, Bernhard Bauer wrote: > Don't you need to call super to do the actual binding? You tested this, right? Yeah, thanks! Unit-tests also revealed this, in the mean-time. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:276: * Notify the Suggestion that a child on the given |position| has been impressed to the user. On 2017/01/13 11:02:32, dgn wrote: > nit "impressed": not true. We count impression when 2/3 of a card is visible, > while here we would call this when a card is bound, which happens usually before > it can be seen at all. Done. I'd rather stick to the name of the function but can change it as well if you insist. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; On 2017/01/13 11:02:32, dgn wrote: > This will set the flag whatever the actual type of the item here is. It can be a > suggestion, a status card, an action item, a loading indicator, or whatever we > thing about adding later. > > If we bind something that is not a suggestion, is that still all right to set > the flags? > > (If you want to check item types: #getItemViewType() and > CardViewHolder#isCard(viewType) could be useful) Good point! Done. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; On 2017/01/13 12:13:00, Bernhard Bauer wrote: > On 2017/01/13 11:02:32, dgn wrote: > > This will set the flag whatever the actual type of the item here is. It can be > a > > suggestion, a status card, an action item, a loading indicator, or whatever we > > thing about adding later. > > Oh, good catch! In fact, the first item will in fact be the header, no? > Is it worth asserting? https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:324: * Puts |suggestions| into this section. It can either replace all existing suggestions with On 2017/01/13 11:02:32, dgn wrote: > nitty mc nitface: in javadoc, we use {@code foo} to put names/variables/etc in > monospace and indicate it's not a piece of the prose, rather than |foo|. Your > IDE should be able to display the processed javadoc, to check the effect. Done. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:348: int itemCount = getSuggestionsCount(); On 2017/01/13 12:13:00, Bernhard Bauer wrote: > This isn't used anymore. Done. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:367: for (SnippetArticle article : suggestions) { On 2017/01/13 11:02:32, dgn wrote: > since you modify mSuggestionsList and not the list passed as input in > addAllFilterFirst, you will process one extra suggestion than desired here. > Removing the element from |suggestions| before mSuggestions.addAll() would fix > it. Is it a problem to process one extra suggestion? The FakeSuggestionsSource does not provide a copy of the list but its list directly. Therefore editing the list breaks unit-tests. The real SuggestionsSource creates the list when converting from c++ to java. I am not sure what is the contract w.r.t. ownership of the list.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; On 2017/01/13 12:33:57, jkrcal wrote: > On 2017/01/13 12:13:00, Bernhard Bauer wrote: > > On 2017/01/13 11:02:32, dgn wrote: > > > This will set the flag whatever the actual type of the item here is. It can > be > > a > > > suggestion, a status card, an action item, a loading indicator, or whatever > we > > > thing about adding later. > > > > Oh, good catch! In fact, the first item will in fact be the header, no? > > > > Is it worth asserting? Yeah, I would assert that position 0 is not a snippet and position 1 is one. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:367: for (SnippetArticle article : suggestions) { On 2017/01/13 12:33:56, jkrcal wrote: > On 2017/01/13 11:02:32, dgn wrote: > > since you modify mSuggestionsList and not the list passed as input in > > addAllFilterFirst, you will process one extra suggestion than desired here. > > Removing the element from |suggestions| before mSuggestions.addAll() would fix > > it. > > Is it a problem to process one extra suggestion? > > The FakeSuggestionsSource does not provide a copy of the list but its list > directly. Therefore editing the list breaks unit-tests. The real > SuggestionsSource creates the list when converting from c++ to java. I am not > sure what is the contract w.r.t. ownership of the list. What I would suggest is defining the contract explicitly. I would be okay with requiring the source to return a modifiable list and doing a small amount more work in the fake to achieve that.
PTAL, again! https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:286: mFirstSuggestionSeen = true; On 2017/01/13 14:17:05, Bernhard Bauer wrote: > On 2017/01/13 12:33:57, jkrcal wrote: > > On 2017/01/13 12:13:00, Bernhard Bauer wrote: > > > On 2017/01/13 11:02:32, dgn wrote: > > > > This will set the flag whatever the actual type of the item here is. It > can > > be > > > a > > > > suggestion, a status card, an action item, a loading indicator, or > whatever > > we > > > > thing about adding later. > > > > > > Oh, good catch! In fact, the first item will in fact be the header, no? > > > > > > > Is it worth asserting? > > Yeah, I would assert that position 0 is not a snippet and position 1 is one. Done. https://codereview.chromium.org/2607333002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:367: for (SnippetArticle article : suggestions) { On 2017/01/13 14:17:05, Bernhard Bauer wrote: > On 2017/01/13 12:33:56, jkrcal wrote: > > On 2017/01/13 11:02:32, dgn wrote: > > > since you modify mSuggestionsList and not the list passed as input in > > > addAllFilterFirst, you will process one extra suggestion than desired here. > > > Removing the element from |suggestions| before mSuggestions.addAll() would > fix > > > it. > > > > Is it a problem to process one extra suggestion? > > > > The FakeSuggestionsSource does not provide a copy of the list but its list > > directly. Therefore editing the list breaks unit-tests. The real > > SuggestionsSource creates the list when converting from c++ to java. I am not > > sure what is the contract w.r.t. ownership of the list. > > What I would suggest is defining the contract explicitly. I would be okay with > requiring the source to return a modifiable list and doing a small amount more > work in the fake to achieve that. Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with nits Thanks! https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:133: public void clearAllButFirst() { Can this be private? https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:156: public void addAllFilterFirst(List<SnippetArticle> suggestions) { Can this be private? https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:277: * Notify the Suggestion that a child on the given {@code position} has been shown to the user. I'm not sure I understand this documentation. This method sets a boolean, it doesn't notify anyone, does it? Also, the capitalized Suggestion confused me as we don't have such a class. https://codereview.chromium.org/2607333002/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2607333002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:193: CardsVariationParameters.setTestVariationParams(new TreeMap<String, String>()); Does this need to be a SortedMap? If not, it's more common to use a HashMap. https://codereview.chromium.org/2607333002/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2607333002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:67: CardsVariationParameters.setTestVariationParams(new TreeMap<String, String>()); This can probably also use a HashMap. https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:174: * Puts |suggestions| into given |category|. It can either replace all existing suggestions with nit: we generally use {@code paramName} in our Java code now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % michael's comments
LGTM with nits as well, thanks! https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:273: } else if (position > 1) { Nit: only one space between `else` and `if`. https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:274: mSubsequentSuggestionsSeen = true; Can you assert that the type is SNIPPET in this case?
Thanks for all the comments! All should be resolved by now. I hope to have addressed the last round correctly :) https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:133: public void clearAllButFirst() { On 2017/01/16 14:41:57, Michael van Ouwerkerk wrote: > Can this be private? Done. https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:156: public void addAllFilterFirst(List<SnippetArticle> suggestions) { On 2017/01/16 14:41:57, Michael van Ouwerkerk wrote: > Can this be private? Removed in the last version. https://codereview.chromium.org/2607333002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:277: * Notify the Suggestion that a child on the given {@code position} has been shown to the user. On 2017/01/16 14:41:57, Michael van Ouwerkerk wrote: > I'm not sure I understand this documentation. This method sets a boolean, it > doesn't notify anyone, does it? Also, the capitalized Suggestion confused me as > we don't have such a class. Rephrased. https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:174: * Puts |suggestions| into given |category|. It can either replace all existing suggestions with On 2017/01/16 14:41:58, Michael van Ouwerkerk wrote: > nit: we generally use {@code paramName} in our Java code now. Done. https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:273: } else if (position > 1) { On 2017/01/16 14:57:05, Bernhard Bauer wrote: > Nit: only one space between `else` and `if`. Done. https://codereview.chromium.org/2607333002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:274: mSubsequentSuggestionsSeen = true; On 2017/01/16 14:57:05, Bernhard Bauer wrote: > Can you assert that the type is SNIPPET in this case? Reworked after an offline discussion with Bernhard.
lgtm
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2607333002/#ps280001 (title: "Fix a unit-test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484583994212900,
"parent_rev": "0ed6d578394cf6648a30dbb8714bc22b99ab91e4", "commit_rev":
"d14a88c13b200244200ad1fe5298f01a9cc3e52f"}
Message was sent while issue was closed.
Description was changed from ========== [NTP Suggestions] Updating the suggestions before going below the fold. Currently the NTP UI updates suggestions only when the current list of suggestions for the given category is empty. After this CL, suggestions are also updated for non-empty category. The pre-conditions are: - the user has not yet scrolled below the fold on the NTP, - there is no peeking-card animation on the NTP. BUG=674163 ========== to ========== [NTP Suggestions] Updating the suggestions before going below the fold. Currently the NTP UI updates suggestions only when the current list of suggestions for the given category is empty. After this CL, suggestions are also updated for non-empty category. The pre-conditions are: - the user has not yet scrolled below the fold on the NTP, - there is no peeking-card animation on the NTP. BUG=674163 Review-Url: https://codereview.chromium.org/2607333002 Cr-Commit-Position: refs/heads/master@{#443908} Committed: https://chromium.googlesource.com/chromium/src/+/d14a88c13b200244200ad1fe5298... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/d14a88c13b200244200ad1fe5298... |
