Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000233002:
[NTP Snippets] Unschedule fetches when the service should be disabled (Closed)
Description[NTP Snippets] Unschedule fetches when the service should be disabled
The snippet service is now able to switch between being enabled
or disabled, properly notifying its observers and releasing
unecessary resources
BUG=608913
Committed: https://crrev.com/5de0fd3bc4b53a39fc5557706d74a715839ee80c
Cr-Commit-Position: refs/heads/master@{#398290}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Please only look at ntp_snippets_service.*, the rest is because of the merge gone bad. #
Total comments: 15
Patch Set 3 : Rebase and address comments #
Total comments: 4
Patch Set 4 #
Total comments: 2
Patch Set 5 : #
Total comments: 38
Patch Set 6 : Address comments #Patch Set 7 : fix typos #
Total comments: 18
Patch Set 8 : Address comments #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 25 (5 generated)
dgn@chromium.org changed reviewers: + treib@chromium.org
PTAL. I don't even have the last version of your patch and tests are broken, but it works and conveys the idea. WDYT?
On 2016/05/23 20:28:21, dgn wrote:
> PTAL. I don't even have the last version of your patch and tests are broken,
but
> it works and conveys the idea. WDYT?
And about how often methods are called, then enabling/disabling sequence is
something like:
Enable -> Load from DB -> (A) NTPSnippetsServiceLoaded (0 snippets)
-> starts fetch -> (B) NTPSnippetsServiceLoaded (some)
Disable -> OnSuggestionsChanged -> (C) NTPSnippetsServiceLoaded (0 snippets)
-> ClearSnippets -> (D) NTPSnippetsServiceLoaded (0 snippets)
(A) and (C) could probably be skipped.
On 2016/05/23 20:55:38, dgn wrote: > On 2016/05/23 20:28:21, dgn wrote: > > PTAL. I don't even have the last version of your patch and tests are broken, > but > > it works and conveys the idea. WDYT? > > And about how often methods are called, then enabling/disabling sequence is > something like: > > Enable -> Load from DB -> (A) NTPSnippetsServiceLoaded (0 snippets) > -> starts fetch -> (B) NTPSnippetsServiceLoaded (some) > > > Disable -> OnSuggestionsChanged -> (C) NTPSnippetsServiceLoaded (0 snippets) > -> ClearSnippets -> (D) NTPSnippetsServiceLoaded (0 snippets) > > (A) and (C) could probably be skipped. Generally looks good, thanks! (A) shouldn't generally be skipped, because we expect to actually have > 0 snippets there :) (Maybe skip it only in the case where it's 0? Doesn't really matter though IMO, and would make tests more complicated, because then there's no simple way to wait for the DB load.) In the disable sequence, it probably depends on whether we or the SuggestionsService get notified first when Sync is disabled? I guess we can just early-out in ClearSnippets if it's already empty, that should take care of the double-notification.
https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:226: void NTPSnippetsService::Init(bool enabled) { Heads-up: Michael's https://codereview.chromium.org/1997473004/ moves the "enabled" bool to the ctor. (Shouldn't be a problem here, just to keep in mind) https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:387: // loaded to avoid unnecessary state changes. Hm, this kinda seems wrong. IMO we should have a consistent mapping from sync state to our state, and have EnterState handle any resulting no-op state changes, instead of ignoring things here. WDYT? Is that possible without introducing extra transitions (or even extra states)? https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:394: EnterState(enabled ? State::INITED : State::DISABLED); What if we were already in the READY state? https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:400: return; // TODO(dgn) how can we get here? Just race conditons? I think it can happen if we started a fetch, then got shutdown before it completed? https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:629: state_ = State::DISABLED; IMO only EnterState should change state_. I think you set it here because RescheduleFetching checks it, and EnterState sets it too late. Can that be changed? https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:168: READY = LOADED, Why READY and LOADED? Can we stick with just one? (READY is probably a better name than LOADED)
On 2016/05/24 09:04:40, Marc Treib wrote:
> On 2016/05/23 20:55:38, dgn wrote:
> > On 2016/05/23 20:28:21, dgn wrote:
> > > PTAL. I don't even have the last version of your patch and tests are
broken,
> > but
> > > it works and conveys the idea. WDYT?
> >
> > And about how often methods are called, then enabling/disabling sequence is
> > something like:
> >
> > Enable -> Load from DB -> (A) NTPSnippetsServiceLoaded (0 snippets)
> > -> starts fetch -> (B) NTPSnippetsServiceLoaded
(some)
> >
It's actually a triple notification!
Enable -> Load from DB -> (A) NTPSnippetsServiceLoaded (0 snippets)
-> starts fetch -> (B) NTPSnippetsServiceLoaded (0
snippets)
-> OnSuggestionsChanged -> (C) NTPSnippetsServiceLoaded
(some)
For tests, skipping A should be all right, as we would do a manual fetch when we
get nothing, and get the callback called through (B). As for the (C), well...
we'll just call it. But all those cases are specific only to the case where the
service was previously disabled, had no hosts nor snippets in the database. idk
how much impact it would have on the metrics. Probably small enough.
PTAL. Sorry about the dirty diff, that's a rebase gone wrong. relevant changes are still in ntp_snippets_service.* https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:226: void NTPSnippetsService::Init(bool enabled) { On 2016/05/24 09:04:46, Marc Treib wrote: > Heads-up: Michael's https://codereview.chromium.org/1997473004/ moves the > "enabled" bool to the ctor. (Shouldn't be a problem here, just to keep in mind) Acknowledged. https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:387: // loaded to avoid unnecessary state changes. On 2016/05/24 09:04:46, Marc Treib wrote: > Hm, this kinda seems wrong. IMO we should have a consistent mapping from sync > state to our state, and have EnterState handle any resulting no-op state > changes, instead of ignoring things here. WDYT? Is that possible without > introducing extra transitions (or even extra states)? I explicitly handle the UNKNOWN state now. WDYT? https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:394: EnterState(enabled ? State::INITED : State::DISABLED); On 2016/05/24 09:04:46, Marc Treib wrote: > What if we were already in the READY state? Handled same state transitions as noops in EnterState. https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:400: return; // TODO(dgn) how can we get here? Just race conditons? On 2016/05/24 09:04:46, Marc Treib wrote: > I think it can happen if we started a fetch, then got shutdown before it > completed? Acknowledged. https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:629: state_ = State::DISABLED; On 2016/05/24 09:04:46, Marc Treib wrote: > IMO only EnterState should change state_. > I think you set it here because RescheduleFetching checks it, and EnterState > sets it too late. Can that be changed? Done. https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:168: READY = LOADED, On 2016/05/24 09:04:46, Marc Treib wrote: > Why READY and LOADED? Can we stick with just one? (READY is probably a better > name than LOADED) That was in case we have more intermediate states, to have the reference to the READY state be able to easily move. Removed that. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:222: State GetStateForDependenciesStatus(); I'm thinking about moving all the service's state management methods and members to a different class, wdyt? That would be the new ones below, + add an interface with state change events. Not sure if it would be worth the trouble, but it would remove some stuff from this class that is starting to get quite big.
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:387: // loaded to avoid unnecessary state changes. On 2016/05/24 18:13:30, dgn wrote: > On 2016/05/24 09:04:46, Marc Treib wrote: > > Hm, this kinda seems wrong. IMO we should have a consistent mapping from sync > > state to our state, and have EnterState handle any resulting no-op state > > changes, instead of ignoring things here. WDYT? Is that possible without > > introducing extra transitions (or even extra states)? > > I explicitly handle the UNKNOWN state now. WDYT? SGTM! https://codereview.chromium.org/2000233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2000233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:139: Log.d("DGN", "histogram -> SNIPPETS_ACTION_SHOWN"); :) https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:292: // snippets-internals can call ClearSnippets while the service is ready. Er, isn't it acceptable for anyone to call ClearSnippets while the service is ready? It's part of the public interface after all. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:368: if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone()) { I think the ConfigurationDone check is redundant - if it's active, then configuration must be done too. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:394: EnterState(GetStateForDependenciesStatus()); Can we get here before the DB is loaded? If so, please handle, if not, please DCHECK. (Or maybe GetStateForDependenciesStatus should check the DB load status as well?) https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:52: enum class DisabledReason { I'd prefer to keep this and State in the NTPSnippetsService class, since they're not really useful outside of it (and aren't even part of the public interface, right?) https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:76: NOT_INITED, This state was just removed in https://codereview.chromium.org/1997473004 https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:222: State GetStateForDependenciesStatus(); On 2016/05/24 18:13:30, dgn wrote: > I'm thinking about moving all the service's state management methods and members > to a different class, wdyt? > > That would be the new ones below, + add an interface with state change events. > Not sure if it would be worth the trouble, but it would remove some stuff from > this class that is starting to get quite big. Hm, seems like it would be hard to cleanly separate that? Not sure if it's worth the trouble (and extra indirections), but I'm not opposed if you want to try it. I'd prefer a follow-up CL though.
https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:90: READY, FYI: Based on Bernhard's comments on my CL, it looks like the INITED vs READY distinction will go away again soon. That should simplify things here a bit :)
https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:292: // snippets-internals can call ClearSnippets while the service is ready. On 2016/05/25 14:47:32, Marc Treib wrote: > Er, isn't it acceptable for anyone to call ClearSnippets while the service is > ready? It's part of the public interface after all. True, removed that comment. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:368: if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone()) { On 2016/05/25 14:47:32, Marc Treib wrote: > I think the ConfigurationDone check is redundant - if it's active, then > configuration must be done too. When the sync options are modified, IsSyncActive() becomes true first, but the active data types are not populated yet. When ConfigurationDone() is true, the ActiveDataTypes are supposed to be in the right state. I'm not sure why IsSyncActive() is checked though. It verifies that the sync backend is connected, but I didn't search enough to clarify the possible combinations wrt ConfigurationDone(). That's what the suggestion service checks anyway. I added a comment to clarify this. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:394: EnterState(GetStateForDependenciesStatus()); On 2016/05/25 14:47:32, Marc Treib wrote: > Can we get here before the DB is loaded? If so, please handle, if not, please > DCHECK. (Or maybe GetStateForDependenciesStatus should check the DB load status > as well?) Done. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:52: enum class DisabledReason { On 2016/05/25 14:47:32, Marc Treib wrote: > I'd prefer to keep this and State in the NTPSnippetsService class, since they're > not really useful outside of it (and aren't even part of the public interface, > right?) This is going to be used to provide a way for Java to know what's wrong and display the right information to the user. I moved state back in the class. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:76: NOT_INITED, On 2016/05/25 14:47:32, Marc Treib wrote: > This state was just removed in https://codereview.chromium.org/1997473004 Left that one in, and removed INITED. the constructor starts the DB load, and when it finishes it now moves to READY. https://codereview.chromium.org/2000233002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:90: READY, On 2016/05/30 16:26:15, Marc Treib wrote: > FYI: Based on Bernhard's comments on my CL, it looks like the INITED vs READY > distinction will go away again soon. That should simplify things here a bit :) Hum... I ran in another weird case. Will comment that on the next patch set.
PTAL https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:214: RescheduleFetching(); Significant change: we now call Reschedule fetching during the transition to READY, rather than in the constructor. When the service is disabled, it happens during the transition to disable, synchronously from the constructor, so changes are minimal there. However here, it would happen only once the DB is done loading. Worth modifying? https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:223: // Significant change: we now call Reschedule fetching during the Was supposed to be a review comment. To be removed. https://codereview.chromium.org/2000233002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:676: case State::READY: { maybe "ENABLED" is better?
https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:214: RescheduleFetching(); On 2016/06/03 19:42:22, dgn wrote: > Significant change: we now call Reschedule fetching during the > transition to READY, rather than in the constructor. When the service > is disabled, it happens during the transition to disable, synchronously > from the constructor, so changes are minimal there. However here, it > would happen only once the DB is done loading. Worth modifying? I think this is fine. https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:223: // Significant change: we now call Reschedule fetching during the On 2016/06/03 19:42:22, dgn wrote: > Was supposed to be a review comment. To be removed. Acknowledged. https://codereview.chromium.org/2000233002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:676: case State::READY: { On 2016/06/03 19:42:23, dgn wrote: > maybe "ENABLED" is better? Hm, I don't know. You pass in "enabled = true" to the constructor, but then it won't be in the ENABLED state immediately... I think READY is fine. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:202: // TODO(dgn) should that be removed after branch point? Yup - seems I forgot to put in a TODO for that? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: // from the state, since it will allow us to enable the snippets service. enable or disable, even! https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:242: suggestions_service_subscription_.reset(); Maybe move this into a helper method (EnterStateShutDown or something like that), analogous to the Enable/Disable methods? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:373: // configuration and is aware of the different components to sync. s/components/data types ? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:558: DCHECK(ready() || state_ == State::NOT_INITED /* for OnDatabaseLoaded */); In OnDatabaseLoaded, could we call EnterState before LoadingSnippetsFinished? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:641: ClearSnippets(); Just to make sure: We do not get here during the regular shutdown sequence, right? In that case, yes, we should clear the discarded snippets too. Privacy, and all that. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:654: if (dr == DisabledReason::NONE) Make this a switch, so it's clear that all possible values are handled? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:150: // Returns a reason why the service could be disabled, or DisabledReason::NONE "could be disabled"? If this returns something !=NONE, then the service *is* disabled, right? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:152: DisabledReason GetDisabledReason(); const? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:183: // - DISABLED: if the state changed, for example after |OnStateChanged| is What's "the state" here? https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:185: // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown. s/is/if https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:193: // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown. here too https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:225: // Applies state transitions (see |State|'s documentation )and verifies them. nit: misplaced ")" "Verifies and applies"? The verification should come first :) https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:228: // Enable the service. Do not call directly, use |EnterState| instead. nit: Enable*s* the service https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:229: void Enable(); Maybe call this "EnterStateEnabled" or something like that? That would make it a bit clearer IMO, and also make it kinda obvious that it's not supposed to be called directly. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:231: // Disable the service. Do not call directly, use |EnterState| instead. Disables https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (left): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:256: EXPECT_TRUE(service_->loaded()); Any reason for removing this line?
PTAL https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:202: // TODO(dgn) should that be removed after branch point? On 2016/06/06 08:38:32, Marc Treib wrote: > Yup - seems I forgot to put in a TODO for that? Acknowledged. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: // from the state, since it will allow us to enable the snippets service. On 2016/06/06 08:38:32, Marc Treib wrote: > enable or disable, even! Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:242: suggestions_service_subscription_.reset(); On 2016/06/06 08:38:32, Marc Treib wrote: > Maybe move this into a helper method (EnterStateShutDown or something like > that), analogous to the Enable/Disable methods? Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:350: FetchSnippets(); Removed so that we don't fetch while disabling the service. added fetch in previous call sites. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:373: // configuration and is aware of the different components to sync. On 2016/06/06 08:38:32, Marc Treib wrote: > s/components/data types > ? Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:558: DCHECK(ready() || state_ == State::NOT_INITED /* for OnDatabaseLoaded */); On 2016/06/06 08:38:32, Marc Treib wrote: > In OnDatabaseLoaded, could we call EnterState before LoadingSnippetsFinished? Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:641: ClearSnippets(); On 2016/06/06 08:38:32, Marc Treib wrote: > Just to make sure: We do not get here during the regular shutdown sequence, > right? > In that case, yes, we should clear the discarded snippets too. Privacy, and all > that. No, we should not. It's guarded by a DCHECK. But we don't run the shutdown sequence on android, Chrome just gets killed. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:654: if (dr == DisabledReason::NONE) On 2016/06/06 08:38:32, Marc Treib wrote: > Make this a switch, so it's clear that all possible values are handled? Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:150: // Returns a reason why the service could be disabled, or DisabledReason::NONE On 2016/06/06 08:38:33, Marc Treib wrote: > "could be disabled"? If this returns something !=NONE, then the service *is* > disabled, right? It doesn't only depend on the state of the service itself, since we use it do check whether we should disable the service. Using sync as an example, it looks at the sync state, and then says that the service should be disabled because of the current sync state. In the sync observer implementation we call it and it returns "DISABLED" even though the service is currently enabled. And we use that result to transition the service to DISABLED. I'm not sure what the best way to clarify this in the documentation would be... https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:152: DisabledReason GetDisabledReason(); On 2016/06/06 08:38:33, Marc Treib wrote: > const? Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:183: // - DISABLED: if the state changed, for example after |OnStateChanged| is On 2016/06/06 08:38:33, Marc Treib wrote: > What's "the state" here? The state of other dependencies, like sync for example. Reworded. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:185: // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown. On 2016/06/06 08:38:33, Marc Treib wrote: > s/is/if Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:193: // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown. On 2016/06/06 08:38:33, Marc Treib wrote: > here too Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:225: // Applies state transitions (see |State|'s documentation )and verifies them. On 2016/06/06 08:38:33, Marc Treib wrote: > nit: misplaced ")" > "Verifies and applies"? The verification should come first :) Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:228: // Enable the service. Do not call directly, use |EnterState| instead. On 2016/06/06 08:38:33, Marc Treib wrote: > nit: Enable*s* the service Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:229: void Enable(); On 2016/06/06 08:38:33, Marc Treib wrote: > Maybe call this "EnterStateEnabled" or something like that? That would make it a > bit clearer IMO, and also make it kinda obvious that it's not supposed to be > called directly. Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:231: // Disable the service. Do not call directly, use |EnterState| instead. On 2016/06/06 08:38:33, Marc Treib wrote: > Disables Done. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (left): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:256: EXPECT_TRUE(service_->loaded()); On 2016/06/06 08:38:33, Marc Treib wrote: > Any reason for removing this line? That was related to the order between the db loading and the state change. Changed it and reverted here.
https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:350: FetchSnippets(); On 2016/06/06 15:44:39, dgn wrote: > Removed so that we don't fetch while disabling the service. added fetch in > previous call sites. Acknowledged. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:150: // Returns a reason why the service could be disabled, or DisabledReason::NONE On 2016/06/06 15:44:39, dgn wrote: > On 2016/06/06 08:38:33, Marc Treib wrote: > > "could be disabled"? If this returns something !=NONE, then the service *is* > > disabled, right? > > It doesn't only depend on the state of the service itself, since we use it do > check whether we should disable the service. > Using sync as an example, it looks at the sync state, and then says that the > service should be disabled because of the current sync state. In the sync > observer implementation we call it and it returns "DISABLED" even though the > service is currently enabled. And we use that result to transition the service > to DISABLED. > > I'm not sure what the best way to clarify this in the documentation would be... Hm. So I guess the issue is that this method serves two purposes: - Internal: "Should we be disabled?" - External: "what's the current disable reason, if any?" Maybe just something like "Returns the current disable reason, or DisabledReason::NONE if there isn't one."? That seems to cover both cases. https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (left): https://codereview.chromium.org/2000233002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:256: EXPECT_TRUE(service_->loaded()); On 2016/06/06 15:44:39, dgn wrote: > On 2016/06/06 08:38:33, Marc Treib wrote: > > Any reason for removing this line? > > That was related to the order between the db loading and the state change. > Changed it and reverted here. Acknowledged. https://codereview.chromium.org/2000233002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2000233002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:124: ntp_snippets_service_->FetchSnippets(); Hm. So this is the only call site for ClearDiscardedSnippets? Now that I see it, maybe it's actually better not to fetch automatically here? Less "magic" in the debug UI... WDYT? https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:215: // from the state, since it will allow us to enable or disabled the snippets nit: disable, no "d" https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:678: default: Can we list the remaining cases explicitly here? That way, we should get a compile error if we ever add another one in the future and forget to handle it here. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:698: bool fetch_snippets = state_ != State::NOT_INITED; ...so this will only be true when we're going from DISABLED to READY? Why do we want to fetch in that case? https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:182: // - READY: noop nit: I'd remove the "noop" comments, IMO they don't add anything, and they're not actually a *change*. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:395: // ResetSyncServiceMock(); ?
PTAL https://codereview.chromium.org/2000233002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2000233002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:124: ntp_snippets_service_->FetchSnippets(); On 2016/06/07 08:55:39, Marc Treib wrote: > Hm. So this is the only call site for ClearDiscardedSnippets? > Now that I see it, maybe it's actually better not to fetch automatically here? > Less "magic" in the debug UI... WDYT? I think so too. And when debugging the ntp, after clearing a refresh is required to pull the new snippets anyway, so it doesn't affect it. Removed this then. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:215: // from the state, since it will allow us to enable or disabled the snippets On 2016/06/07 08:55:39, Marc Treib wrote: > nit: disable, no "d" Done. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:678: default: On 2016/06/07 08:55:39, Marc Treib wrote: > Can we list the remaining cases explicitly here? That way, we should get a > compile error if we ever add another one in the future and forget to handle it > here. Done. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:698: bool fetch_snippets = state_ != State::NOT_INITED; On 2016/06/07 08:55:39, Marc Treib wrote: > ...so this will only be true when we're going from DISABLED to READY? > Why do we want to fetch in that case? The snippet DB would be cleared in that case (we do that in EnterStateDisabled), so we would not have anything to show. When we switch to READY, we would get snippets, notify the observer, that will notice that we finally got something and it will refresh the UI to display them. That's reproducing the previous behaviour. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:182: // - READY: noop On 2016/06/07 08:55:39, Marc Treib wrote: > nit: I'd remove the "noop" comments, IMO they don't add anything, and they're > not actually a *change*. I added them since transitions from states not written down here hit DCHECKs and crash, and in a case where it's supported, I suppose it's useful to know that we don't go through the observer setup, fetch, etc. again. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:395: // ResetSyncServiceMock(); On 2016/06/07 08:55:39, Marc Treib wrote: > ? Added that to the base test class, removed this subclass then, as it's not any special anymore.
lgtm https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:678: default: On 2016/06/07 11:22:46, dgn wrote: > On 2016/06/07 08:55:39, Marc Treib wrote: > > Can we list the remaining cases explicitly here? That way, we should get a > > compile error if we ever add another one in the future and forget to handle it > > here. > > Done. You'll have to add a "dummy return" after the switch to make it compile. Maybe also add a NOTREACHED(). https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:698: bool fetch_snippets = state_ != State::NOT_INITED; On 2016/06/07 11:22:46, dgn wrote: > On 2016/06/07 08:55:39, Marc Treib wrote: > > ...so this will only be true when we're going from DISABLED to READY? > > Why do we want to fetch in that case? > > The snippet DB would be cleared in that case (we do that in EnterStateDisabled), > so we would not have anything to show. When we switch to READY, we would get > snippets, notify the observer, that will notice that we finally got something > and it will refresh the UI to display them. That's reproducing the previous > behaviour. Ah, right - I missed that we're clearing the DB in the transition to DISABLED. Can we write this as "state_ == State::DISABLED"? IMO that'd make it a bit clearer. I'd also rewrite the comment to something like "If we're coming from DISABLED, fetch snippets now, because otherwise there won't be any" - WDYT? https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:182: // - READY: noop On 2016/06/07 11:22:47, dgn wrote: > On 2016/06/07 08:55:39, Marc Treib wrote: > > nit: I'd remove the "noop" comments, IMO they don't add anything, and they're > > not actually a *change*. > > I added them since transitions from states not written down here hit DCHECKs and > crash, and in a case where it's supported, I suppose it's useful to know that we > don't go through the observer setup, fetch, etc. again. Acknowledged. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:395: // ResetSyncServiceMock(); On 2016/06/07 11:22:47, dgn wrote: > On 2016/06/07 08:55:39, Marc Treib wrote: > > ? > > Added that to the base test class, removed this subclass then, as it's not any > special anymore. Yay!
Thanks! https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:678: default: On 2016/06/07 11:50:37, Marc Treib wrote: > On 2016/06/07 11:22:46, dgn wrote: > > On 2016/06/07 08:55:39, Marc Treib wrote: > > > Can we list the remaining cases explicitly here? That way, we should get a > > > compile error if we ever add another one in the future and forget to handle > it > > > here. > > > > Done. > > You'll have to add a "dummy return" after the switch to make it compile. Maybe > also add a NOTREACHED(). Doh. And I thought using clang locally would help me finding mistakes earlier. Why do they both have to have different constraints on compilation. Done. https://codereview.chromium.org/2000233002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:698: bool fetch_snippets = state_ != State::NOT_INITED; On 2016/06/07 11:50:37, Marc Treib wrote: > On 2016/06/07 11:22:46, dgn wrote: > > On 2016/06/07 08:55:39, Marc Treib wrote: > > > ...so this will only be true when we're going from DISABLED to READY? > > > Why do we want to fetch in that case? > > > > The snippet DB would be cleared in that case (we do that in > EnterStateDisabled), > > so we would not have anything to show. When we switch to READY, we would get > > snippets, notify the observer, that will notice that we finally got something > > and it will refresh the UI to display them. That's reproducing the previous > > behaviour. > > Ah, right - I missed that we're clearing the DB in the transition to DISABLED. > Can we write this as "state_ == State::DISABLED"? IMO that'd make it a bit > clearer. I'd also rewrite the comment to something like "If we're coming from > DISABLED, fetch snippets now, because otherwise there won't be any" - WDYT? Done.
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 Link to the patchset: https://codereview.chromium.org/2000233002/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000233002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Unschedule fetches when the service should be disabled The snippet service is now able to switch between being enabled or disabled, properly notifying its observers and releasing unecessary resources BUG=608913 ========== to ========== [NTP Snippets] Unschedule fetches when the service should be disabled The snippet service is now able to switch between being enabled or disabled, properly notifying its observers and releasing unecessary resources BUG=608913 Committed: https://crrev.com/5de0fd3bc4b53a39fc5557706d74a715839ee80c Cr-Commit-Position: refs/heads/master@{#398290} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5de0fd3bc4b53a39fc5557706d74a715839ee80c Cr-Commit-Position: refs/heads/master@{#398290} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
