Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190353002:
📰Fix SnippetsService's status reporting (Closed)
Description[NTP Client] Fix SnippetsService's status reporting
During the initial phases of the snippets service lifetime, it currently
reports frequent state changes and does not use AVAILABLE_LOADING at all
to notify its observers that it is currently fetching. This CL aims at
fixing that to allow showing more useful information to the user.
BUG=627488
Committed: https://crrev.com/0bfd38b0fbfd42e7224b4b07430b781119036387
Cr-Commit-Position: refs/heads/master@{#409002}
Patch Set 1 #
Total comments: 15
Patch Set 2 : cleanup #
Total comments: 4
Patch Set 3 : rebase #
Messages
Total messages: 25 (12 generated)
dgn@chromium.org changed reviewers: + pke@google.com, treib@chromium.org
PTAL. There are some aspects of the SnippetsService I don't completely understand, mostly related to when we notify the clients of new suggestions. Could you please comment on the highlighted lines? Thanks! https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:447: NotifyNewSuggestions(); Why do we do that BEFORE fetching? https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:722: NotifyNewSuggestions(); If we started a fetch as a result of the state being updated and not having snippets from the DB, because of the call below we would still notify the client with no snippets and force the "AVAILABLE" status. WAI? IMO that seems wrong. Skipping this call will put us either: - in AVAILABLE_LOADING: because of the pending fetch started above. - in INITIALIZING status: If it's right on startup and because we don't have sync data, the fetch can't start. We appropriately report that we are still initializing.
Description was changed from ========== [WIP][NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ========== to ========== [WIP][NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ==========
Some comments, Philipp might have more... :) https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:263: EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); I don't think this is right. AVAILABLE_LOADING is meant to be the initial state, when the service is active but the very first fetch hasn't completed yet. We shouldn't go back into this state while a background fetch is running. https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:447: NotifyNewSuggestions(); On 2016/07/29 09:58:36, dgn wrote: > Why do we do that BEFORE fetching? We potentially deleted some snippets above, so we should arguably tell observers that something changed. We should at least check if we actually deleted anything though. Probably we should also only notify if we're already AVAILABLE. https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:722: NotifyNewSuggestions(); On 2016/07/29 09:58:36, dgn wrote: > If we started a fetch as a result of the state being updated > and not having snippets from the DB, because of the call below we would > still notify the client with no snippets and force the "AVAILABLE" status. > WAI? > IMO that seems wrong. Skipping this call will put us either: > - in AVAILABLE_LOADING: because of the pending fetch started above. > - in INITIALIZING status: If it's right on startup and because we don't > have sync data, the fetch can't start. We appropriately report that we > are still initializing. I guess this is here because we "probably" loaded some snippets from the DB. It seems we should check that here, and then either go into AVAILABLE and notify, or go into AVAILABLE_LOADING and start a fetch. https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:822: EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE); Huh, seems like we should already be in this state once we call this? Or possibly enter it immediately afterwards I guess. In any case, changing state here seems wrong...
https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:263: EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); On 2016/07/29 10:19:47, Marc Treib wrote: > I don't think this is right. AVAILABLE_LOADING is meant to be the initial state, > when the service is active but the very first fetch hasn't completed yet. We > shouldn't go back into this state while a background fetch is running. Ah ok I didn't understand AVAILABLE_LOADING properly. So for the snippets service, it would apply only when we just initialized, db has no snippets and we started a fetch? https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:447: NotifyNewSuggestions(); On 2016/07/29 10:19:47, Marc Treib wrote: > On 2016/07/29 09:58:36, dgn wrote: > > Why do we do that BEFORE fetching? > > We potentially deleted some snippets above, so we should arguably tell observers > that something changed. > We should at least check if we actually deleted anything though. > Probably we should also only notify if we're already AVAILABLE. Wouldn't they be notified of the new list when the fetch finishes anyway? https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:722: NotifyNewSuggestions(); On 2016/07/29 10:19:47, Marc Treib wrote: > On 2016/07/29 09:58:36, dgn wrote: > > If we started a fetch as a result of the state being updated > > and not having snippets from the DB, because of the call below we would > > still notify the client with no snippets and force the "AVAILABLE" status. > > WAI? > > IMO that seems wrong. Skipping this call will put us either: > > - in AVAILABLE_LOADING: because of the pending fetch started above. > > - in INITIALIZING status: If it's right on startup and because we don't > > have sync data, the fetch can't start. We appropriately report that we > > are still initializing. > > I guess this is here because we "probably" loaded some snippets from the DB. It > seems we should check that here, and then either go into AVAILABLE and notify, > or go into AVAILABLE_LOADING and start a fetch. So the intent is that while a fetch is pending and we have 0 suggestions locally, we are in AVAILABLE_LOADING, but if we have suggestions or no fetch is pending, we are in AVAILABLE?
https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:263: EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); On 2016/07/29 10:42:48, dgn wrote: > On 2016/07/29 10:19:47, Marc Treib wrote: > > I don't think this is right. AVAILABLE_LOADING is meant to be the initial > state, > > when the service is active but the very first fetch hasn't completed yet. We > > shouldn't go back into this state while a background fetch is running. > > Ah ok I didn't understand AVAILABLE_LOADING properly. So for the snippets > service, it would apply only when we just initialized, db has no snippets and we > started a fetch? Also while we're waiting for results from the DB (which is also async). https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:447: NotifyNewSuggestions(); On 2016/07/29 10:42:48, dgn wrote: > On 2016/07/29 10:19:47, Marc Treib wrote: > > On 2016/07/29 09:58:36, dgn wrote: > > > Why do we do that BEFORE fetching? > > > > We potentially deleted some snippets above, so we should arguably tell > observers > > that something changed. > > We should at least check if we actually deleted anything though. > > Probably we should also only notify if we're already AVAILABLE. > > Wouldn't they be notified of the new list when the fetch finishes anyway? True, but that might take any amount of time, and might not ever complete successfully. https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:722: NotifyNewSuggestions(); On 2016/07/29 10:42:48, dgn wrote: > On 2016/07/29 10:19:47, Marc Treib wrote: > > On 2016/07/29 09:58:36, dgn wrote: > > > If we started a fetch as a result of the state being updated > > > and not having snippets from the DB, because of the call below we would > > > still notify the client with no snippets and force the "AVAILABLE" status. > > > WAI? > > > IMO that seems wrong. Skipping this call will put us either: > > > - in AVAILABLE_LOADING: because of the pending fetch started above. > > > - in INITIALIZING status: If it's right on startup and because we don't > > > have sync data, the fetch can't start. We appropriately report that we > > > are still initializing. > > > > I guess this is here because we "probably" loaded some snippets from the DB. > It > > seems we should check that here, and then either go into AVAILABLE and notify, > > or go into AVAILABLE_LOADING and start a fetch. > > So the intent is that while a fetch is pending and we have 0 suggestions > locally, we are in AVAILABLE_LOADING, but if we have suggestions or no fetch is > pending, we are in AVAILABLE? I think so, yes. We should also be in _LOADING if we're waiting for history sync state and the like (if that still exists...) Philipp, please chime in :)
PTAL.
The new implementation does the following:
Start => INITIALIZING
DBLoaded => If has snippets => AVAILABLE
If has no snippets => AVAILABLE_LOADING
Fetch requested => if has no snippets => AVAILABLE_LOADING
else => no change
Fetch finished => AVAILABLE
EnterStateEnabled => if has no snippets => requests fetch => AVAILABLE_LOADING
else => AVAILABLE
https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):
https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:263: EnterState(state_,
ContentSuggestionsCategoryStatus::AVAILABLE_LOADING);
On 2016/07/29 10:45:46, Marc Treib wrote:
> On 2016/07/29 10:42:48, dgn wrote:
> > On 2016/07/29 10:19:47, Marc Treib wrote:
> > > I don't think this is right. AVAILABLE_LOADING is meant to be the initial
> > state,
> > > when the service is active but the very first fetch hasn't completed yet.
We
> > > shouldn't go back into this state while a background fetch is running.
> >
> > Ah ok I didn't understand AVAILABLE_LOADING properly. So for the snippets
> > service, it would apply only when we just initialized, db has no snippets
and
> we
> > started a fetch?
>
> Also while we're waiting for results from the DB (which is also async).
What about when the service was disabled, so we cleared the snippets. Shouldn't
we go DISABLED -> AVAILABLE_LOADING -> AVAILABLE ?
Also, INITIALIZING is the initial state, from what I can see. WDYT about the new
proposal?
https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:447: NotifyNewSuggestions();
On 2016/07/29 10:45:46, Marc Treib wrote:
> On 2016/07/29 10:42:48, dgn wrote:
> > On 2016/07/29 10:19:47, Marc Treib wrote:
> > > On 2016/07/29 09:58:36, dgn wrote:
> > > > Why do we do that BEFORE fetching?
> > >
> > > We potentially deleted some snippets above, so we should arguably tell
> > observers
> > > that something changed.
> > > We should at least check if we actually deleted anything though.
> > > Probably we should also only notify if we're already AVAILABLE.
> >
> > Wouldn't they be notified of the new list when the fetch finishes anyway?
>
> True, but that might take any amount of time, and might not ever complete
> successfully.
Added a comment about this.
LGTM, but please wait for Philipp to take a look before landing. Thanks! https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:263: EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); On 2016/07/29 16:37:46, dgn wrote: > On 2016/07/29 10:45:46, Marc Treib wrote: > > On 2016/07/29 10:42:48, dgn wrote: > > > On 2016/07/29 10:19:47, Marc Treib wrote: > > > > I don't think this is right. AVAILABLE_LOADING is meant to be the initial > > > state, > > > > when the service is active but the very first fetch hasn't completed yet. > We > > > > shouldn't go back into this state while a background fetch is running. > > > > > > Ah ok I didn't understand AVAILABLE_LOADING properly. So for the snippets > > > service, it would apply only when we just initialized, db has no snippets > and > > we > > > started a fetch? > > > > Also while we're waiting for results from the DB (which is also async). > > What about when the service was disabled, so we cleared the snippets. Shouldn't > we go DISABLED -> AVAILABLE_LOADING -> AVAILABLE ? Yes, that sounds right. > Also, INITIALIZING is the initial state, from what I can see. WDYT about the new > proposal? SGTM https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:264: UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); Hm, so this status effectively means "no suggestions, but a fetch is in progress"? And the UI will use it to show a loading indicator? Alright, makes sense I suppose :)
lgtm -- maybe found a corner case, see below? https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:461: DCHECK(category_status_ == ContentSuggestionsCategoryStatus::AVAILABLE || What if we shut down during a fetch? Or if the user signs out of Chrome during a fetch? Will the fetch be canceled, or can this callback still be fired? Otherwise, this DCHECK would fail. But if that's a possible case, we should instead just return (i.e. ignore the fetch results because during the fetch we decided that we don't want it anymore).
https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:461: DCHECK(category_status_ == ContentSuggestionsCategoryStatus::AVAILABLE || On 2016/08/01 09:14:10, Philipp Keck wrote: > What if we shut down during a fetch? Or if the user signs out of Chrome during a > fetch? Will the fetch be canceled, or can this callback still be fired? > Otherwise, this DCHECK would fail. But if that's a possible case, we should > instead just return (i.e. ignore the fetch results because during the fetch we > decided that we don't want it anymore). I thought about that too :) But there's a ready() check above, so if any of those things happened, we won't get here.
Thanks. I'll update the CL about the UI changes (https://codereview.chromium.org/2191593002/) based on this to make sure it works properly before landing. https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2190353002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:264: UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE_LOADING); On 2016/08/01 09:03:31, Marc Treib wrote: > Hm, so this status effectively means "no suggestions, but a fetch is in > progress"? And the UI will use it to show a loading indicator? Alright, makes > sense I suppose :) Yes, that's how I understand it and the doc of the enum.
The CQ bit was checked by dgn@chromium.org
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 dgn@chromium.org
Description was changed from ========== [WIP][NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ========== to ========== [NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ==========
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...
The CQ bit was unchecked by dgn@chromium.org
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, pke@google.com Link to the patchset: https://codereview.chromium.org/2190353002/#ps40001 (title: "rebase")
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] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ========== to ========== [NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ==========
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] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 ========== to ========== [NTP Client] Fix SnippetsService's status reporting During the initial phases of the snippets service lifetime, it currently reports frequent state changes and does not use AVAILABLE_LOADING at all to notify its observers that it is currently fetching. This CL aims at fixing that to allow showing more useful information to the user. BUG=627488 Committed: https://crrev.com/0bfd38b0fbfd42e7224b4b07430b781119036387 Cr-Commit-Position: refs/heads/master@{#409002} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0bfd38b0fbfd42e7224b4b07430b781119036387 Cr-Commit-Position: refs/heads/master@{#409002} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
