Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644143003:
[Content suggestions] Never update snippets after FetchMore (Closed)
Description[Content suggestions] Never update snippets after FetchMore
The new logic for replacing snippets does not work well for the case
after more suggestions are fetched by the provider on demand and
appended to the list.
This CL switches off replacing the snippets after any snippets have been
appended.
BUG=683050
Review-Url: https://codereview.chromium.org/2644143003
Cr-Commit-Position: refs/heads/master@{#446005}
Committed: https://chromium.googlesource.com/chromium/src/+/389bba160bee7a83f0fd3b0227b6757dc8ae3e77
Patch Set 1 #
Total comments: 4
Patch Set 2 : Nicolas' comments #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Nicolas' comments #2 #
Total comments: 4
Patch Set 5 : Further comments #
Total comments: 4
Patch Set 6 : Further comments #2 #
Messages
Total messages: 37 (21 generated)
jkrcal@chromium.org changed reviewers: + dgn@chromium.org
Nicolas, I am really sorry for missing this (quite a prominent) corner case! I guess this should get merged to M57. PTAL.
https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:56: private boolean mHasAppended; nit: javadoc comments (/** ... */) for member declaration https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:395: if (!replaceExisting) mHasAppended = true; also, why not set mNumberOfSuggestionsSeen to max_int instead of adding a new member? Another approach could be to set it to max_int as soon as we see the last suggestion, in childSeen(). That would address in a nice way the "you have seen all of it" semantics.
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...
Thanks, Nicolas! PTAL, again. https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:56: private boolean mHasAppended; On 2017/01/20 11:08:54, dgn wrote: > nit: javadoc comments (/** ... */) for member declaration Done. https://codereview.chromium.org/2644143003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:395: if (!replaceExisting) mHasAppended = true; On 2017/01/20 11:08:54, dgn wrote: > also, why not set mNumberOfSuggestionsSeen to max_int instead of adding a new > member? Another approach could be to set it to max_int as soon as we see the > last suggestion, in childSeen(). That would address in a nice way the "you have > seen all of it" semantics. Cool, thanks, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2644143003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:282: if (nextPosition == getItemCount() nit: you also have getSuggestionsCount(). wouldn't that be more straightforward?
Patchset #3 (id:40001) has been deleted
Thanks! https://codereview.chromium.org/2644143003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:282: if (nextPosition == getItemCount() On 2017/01/20 19:48:06, dgn wrote: > nit: you also have getSuggestionsCount(). wouldn't that be more straightforward? 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...
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:281: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { drive-by comment: Reading this diff, I have no clue how this is related to the actual problem. Are we sure this is the right place to fix it? Are we sure the method is named properly?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: - tschumann@chromium.org
https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:281: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { On 2017/01/23 08:49:45, tschumann wrote: > drive-by comment: > Reading this diff, I have no clue how this is related to the actual problem. > Are we sure this is the right place to fix it? > Are we sure the method is named properly? I had a closer look now, and I think the original version was much cleaner in this regard. My preference is to keep the 'number of seen elements' (which i guess is used for the new, simplified metrics) and the fact whether a user extended the suggestion list decoupled. The reason is that we don't want to 'hot-replace' suggestions if the user interactively extended the list (because they now have certain expectations about the contents) and not because all suggestions in that section have been seen. Coupling these two things too closely seems like bugs are waiting to happen. Maybe loop in others from the LON team for their input?
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:281: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { On 2017/01/23 10:34:06, tschumann wrote: > On 2017/01/23 08:49:45, tschumann wrote: > > drive-by comment: > > Reading this diff, I have no clue how this is related to the actual problem. > > Are we sure this is the right place to fix it? > > Are we sure the method is named properly? > > I had a closer look now, and I think the original version was much cleaner in > this regard. > My preference is to keep the 'number of seen elements' (which i guess is used > for the new, simplified metrics) and the fact whether a user extended the > suggestion list decoupled. The reason is that we don't want to 'hot-replace' > suggestions if the user interactively extended the list (because they now have > certain expectations about the contents) and not because all suggestions in that > section have been seen. > Coupling these two things too closely seems like bugs are waiting to happen. > > Maybe loop in others from the LON team for their input? Yeah, that seems reasonable. We can just add a flag to SuggestionsSection that we started (or finished?) fetching more suggestions, and check that in addition to our other checks in setSuggestions().
Thanks for input! PTAL again. https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:281: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { On 2017/01/24 13:53:39, Bernhard Bauer wrote: > On 2017/01/23 10:34:06, tschumann wrote: > > On 2017/01/23 08:49:45, tschumann wrote: > > > drive-by comment: > > > Reading this diff, I have no clue how this is related to the actual problem. > > > Are we sure this is the right place to fix it? > > > Are we sure the method is named properly? > > > > I had a closer look now, and I think the original version was much cleaner in > > this regard. > > My preference is to keep the 'number of seen elements' (which i guess is used > > for the new, simplified metrics) and the fact whether a user extended the > > suggestion list decoupled. The reason is that we don't want to 'hot-replace' > > suggestions if the user interactively extended the list (because they now have > > certain expectations about the contents) and not because all suggestions in > that > > section have been seen. > > Coupling these two things too closely seems like bugs are waiting to happen. > > > > Maybe loop in others from the LON team for their input? > > Yeah, that seems reasonable. We can just add a flag to SuggestionsSection that > we started (or finished?) fetching more suggestions, and check that in addition > to our other checks in setSuggestions(). Done. I think flipping the flag when finished makes more sense (because only then people can see and interact with new suggestions).
lgtm https://codereview.chromium.org/2644143003/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2644143003/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:562: List<SnippetArticle> snippets = createDummySuggestions(4, KnownCategories.ARTICLES, "old"); nit: in this case, the categoryId is 42, not KnownCategories.ARTICLES.
https://codereview.chromium.org/2644143003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: if (mNumberOfSuggestionsSeen >= getSuggestionsCount() || mHasAppended) { Wouldn't we want to distinguish this case for UMA and logging from the one where all suggestions have been shown?
Thanks, PTAL, again! https://codereview.chromium.org/2644143003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2644143003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: if (mNumberOfSuggestionsSeen >= getSuggestionsCount() || mHasAppended) { On 2017/01/24 16:37:15, Bernhard Bauer wrote: > Wouldn't we want to distinguish this case for UMA and logging from the one where > all suggestions have been shown? Not needed, from my point of view. The root cause is very similar - the user has at least once scrolled down to the end of this section. The fact that the user also pressed "More" is IMO not relevant to effectiveness of replacing snippets that we want to track with this histogram. https://codereview.chromium.org/2644143003/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2644143003/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:562: List<SnippetArticle> snippets = createDummySuggestions(4, KnownCategories.ARTICLES, "old"); On 2017/01/24 15:48:25, dgn wrote: > nit: in this case, the categoryId is 42, not KnownCategories.ARTICLES. Done. Replaced 42 by a constant all over this file and fixed all my tests to have consistent category ids.
lgtm
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.
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 Link to the patchset: https://codereview.chromium.org/2644143003/#ps120001 (title: "Further comments #2")
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": 120001, "attempt_start_ts": 1485348707814810,
"parent_rev": "516550732e4f5e423d00bed3620dd9386f746084", "commit_rev":
"389bba160bee7a83f0fd3b0227b6757dc8ae3e77"}
Message was sent while issue was closed.
Description was changed from ========== [Content suggestions] Never update snippets after FetchMore The new logic for replacing snippets does not work well for the case after more suggestions are fetched by the provider on demand and appended to the list. This CL switches off replacing the snippets after any snippets have been appended. BUG=683050 ========== to ========== [Content suggestions] Never update snippets after FetchMore The new logic for replacing snippets does not work well for the case after more suggestions are fetched by the provider on demand and appended to the list. This CL switches off replacing the snippets after any snippets have been appended. BUG=683050 Review-Url: https://codereview.chromium.org/2644143003 Cr-Commit-Position: refs/heads/master@{#446005} Committed: https://chromium.googlesource.com/chromium/src/+/389bba160bee7a83f0fd3b0227b6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/389bba160bee7a83f0fd3b0227b6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
