Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166753003:
📰 Stop clearing suggestions to show the RELOAD card. (Closed)
Description[NTP Client] Stop clearing suggestions to show the RELOAD card.
We used to clear already loaded suggestions unconditionally when
receiving notifications of the service status having changed. During
startup we used to pull suggestions right away and get the status OK
notification afterwards, which cleared them. We now properly detect
and do nothing in that case.
BUG=626831
Committed: https://crrev.com/fc4b68d76ae1bc8db479c14be1b306e9185a1626
Cr-Commit-Position: refs/heads/master@{#406837}
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove dodgy boolean, add test #
Total comments: 4
Patch Set 3 : address comments #
Total comments: 5
Messages
Total messages: 38 (20 generated)
Description was changed from ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 ========== to ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
The CQ bit was checked by dgn@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/2166753003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:158: mStatusListItem = StatusListItem.create(disabledReason, this, mNewTabPageManager); Move this to where the list item is set? (Really, I guess we should create this on demand and update the disabled reason when it changes.) https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:172: // transitioned. to a disabled state. We now clear it. Nit: no period before "to". https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:176: notifyItemRangeChanged(FIRST_CARD_POSITION, 2); // Update both the first card and the I would move this comment to before the line it's associated with.
https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:158: mStatusListItem = StatusListItem.create(disabledReason, this, mNewTabPageManager); On 2016/07/20 14:38:43, Bernhard Bauer wrote: > Move this to where the list item is set? > > (Really, I guess we should create this on demand and update the disabled reason > when it changes.) when we run loadSnippets() we pull the current ListItem if needed, so we want to have the most recent version up there. I need to set it here because of the early return below. In most cases, doing it here is on-demand already. The case where we update the status card in advance is for DisabledReason.NONE, it will be shown when the last suggestion is dismissed by the user. We could make the case that the StatusList item could have a reference to SnippetsBridge and request it status across JNI, but we already should have the info here. If you prefer that I could do the change.
OK, LGTM with the nits from the previous review fixed.
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...)
The CQ bit was checked by dgn@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...
I changed the patch to use a combination of whether we have snippets and what the current status is to determine when we should load new snippets. This makes tests easier and clearer PTAL https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:172: // transitioned. to a disabled state. We now clear it. On 2016/07/20 14:38:43, Bernhard Bauer wrote: > Nit: no period before "to". Done. https://codereview.chromium.org/2166753003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:176: notifyItemRangeChanged(FIRST_CARD_POSITION, 2); // Update both the first card and the On 2016/07/20 14:38:43, Bernhard Bauer wrote: > I would move this comment to before the line it's associated with. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
pke@google.com changed reviewers: + pke@google.com
https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:304: return getItemViewType(FIRST_CARD_POSITION) == NewTabPageListItem.VIEW_TYPE_SNIPPET; This is in conflict with: https://codereview.chromium.org/2158413004/diff/1/chrome/android/java/src/org...
https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:137: if (hasSuggestions() I would split this up into two separate returns, and then maybe negate the second condition?
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:137: if (hasSuggestions() On 2016/07/21 09:21:07, Bernhard Bauer wrote: > I would split this up into two separate returns, and then maybe negate the > second condition? Done. https://codereview.chromium.org/2166753003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:304: return getItemViewType(FIRST_CARD_POSITION) == NewTabPageListItem.VIEW_TYPE_SNIPPET; On 2016/07/21 07:59:10, Philipp Keck wrote: > This is in conflict with: > https://codereview.chromium.org/2158413004/diff/1/chrome/android/java/src/org... Thanks for the heads up. I synced with mvanouwerkerk@.
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.
pke@google.com changed reviewers: - pke@google.com
Still LGTM
The CQ bit was checked by dgn@chromium.org
On 2016/07/21 13:59:33, Bernhard Bauer wrote: > Still LGTM Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 ========== to ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 ========== to ========== [NTP Client] Stop clearing suggestions to show the RELOAD card. We used to clear already loaded suggestions unconditionally when receiving notifications of the service status having changed. During startup we used to pull suggestions right away and get the status OK notification afterwards, which cleared them. We now properly detect and do nothing in that case. BUG=626831 Committed: https://crrev.com/fc4b68d76ae1bc8db479c14be1b306e9185a1626 Cr-Commit-Position: refs/heads/master@{#406837} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fc4b68d76ae1bc8db479c14be1b306e9185a1626 Cr-Commit-Position: refs/heads/master@{#406837}
Message was sent while issue was closed.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
Message was sent while issue was closed.
Some follow-up comments. Primarily to help me better understand the current logic. That aside, thanks for the quick fix!! https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:169: // We had many items, implies that the service was previously enabled and just I wonder if we should verify the bit "was previously enabled.". This assumption was incorrect before and let to issues. Should we check for disabledReason != DisabledReason.None? Also, this would potentially lead to the UI dropping suggestion cards the user is looking at right now. What would be a valid, concrete scenario for that? Just wondering if we ever want this to happen...
Message was sent while issue was closed.
https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:169: // We had many items, implies that the service was previously enabled and just On 2016/07/22 08:58:57, tschumann wrote: > I wonder if we should verify the bit "was previously enabled.". This assumption > was incorrect before and let to issues. Should we check for disabledReason != > DisabledReason.None? > > Also, this would potentially lead to the UI dropping suggestion cards the user > is looking at right now. What would be a valid, concrete scenario for that? Just > wondering if we ever want this to happen... FYI: My upcoming CL replaces DisabledReason with ContentSuggestionsCategoryStatus: https://codereview.chromium.org/2158883002/diff/100001/chrome/android/java/sr... It's pretty much equivalent, but there is a fine difference with "INITIALIZING": The status that is set at the very beginning is INITIALIZING, just as DisabledReason.NONE was set initially. But for DisabledReason.HISTORY_SYNC_STATE_UNKNOWN you also get INITIALIZING. That's because it should be equivalent to the user of that state (to the UI) because they both mean: Not fully started yet, cannot tell yet whether content will be available, but stay tuned. However, in the code here, they're sometimes treated differently. So when we reason about different scenarios, we should take the new enum into account.
Message was sent while issue was closed.
https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:169: // We had many items, implies that the service was previously enabled and just On 2016/07/22 08:58:57, tschumann wrote: > I wonder if we should verify the bit "was previously enabled.". This assumption > was incorrect before and let to issues. Should we check for disabledReason != > DisabledReason.None? > > Also, this would potentially lead to the UI dropping suggestion cards the user > is looking at right now. What would be a valid, concrete scenario for that? Just > wondering if we ever want this to happen... We already check above (l166) that we are in the enabled state and that we have suggestions. In that case we don't do anything. We want to clear suggestions when the user signs out, or the snippets get disabled for some reason. If we don't clear them we don't really have a way to let the user know if something is wrong, since we only show status when there are no suggestions.
Message was sent while issue was closed.
https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:169: // We had many items, implies that the service was previously enabled and just On 2016/07/22 09:47:14, dgn wrote: > On 2016/07/22 08:58:57, tschumann wrote: > > I wonder if we should verify the bit "was previously enabled.". This > assumption > > was incorrect before and let to issues. Should we check for disabledReason != > > DisabledReason.None? > > > > Also, this would potentially lead to the UI dropping suggestion cards the user > > is looking at right now. What would be a valid, concrete scenario for that? > Just > > wondering if we ever want this to happen... > > We already check above (l166) that we are in the enabled state and that we have > suggestions. In that case we don't do anything. > > We want to clear suggestions when the user signs out, or the snippets get > disabled for some reason. If we don't clear them we don't really have a way to > let the user know if something is wrong, since we only show status when there > are no suggestions. ok, given that Philipp is changing this anyways, let's table this. (given that we know only few cases in which we actually want to clear the snippets, we might enumerate those explicitly). You're right with that condition being enforced by line 166 -- i was just a little afraid of hidden assumptions.
Message was sent while issue was closed.
https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2166753003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:169: // We had many items, implies that the service was previously enabled and just On 2016/07/22 10:00:51, tschumann wrote: > On 2016/07/22 09:47:14, dgn wrote: > > On 2016/07/22 08:58:57, tschumann wrote: > > > I wonder if we should verify the bit "was previously enabled.". This > > assumption > > > was incorrect before and let to issues. Should we check for disabledReason > != > > > DisabledReason.None? > > > > > > Also, this would potentially lead to the UI dropping suggestion cards the > user > > > is looking at right now. What would be a valid, concrete scenario for that? > > Just > > > wondering if we ever want this to happen... > > > > We already check above (l166) that we are in the enabled state and that we > have > > suggestions. In that case we don't do anything. > > > > We want to clear suggestions when the user signs out, or the snippets get > > disabled for some reason. If we don't clear them we don't really have a way to > > let the user know if something is wrong, since we only show status when there > > are no suggestions. > > ok, given that Philipp is changing this anyways, let's table this. (given that > we know only few cases in which we actually want to clear the snippets, we might > enumerate those explicitly). > You're right with that condition being enforced by line 166 -- i was just a > little afraid of hidden assumptions. Ok, I'll look at his CL after the merge to see how this can be clarified. Thanks for the feedback! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
