[NTP Snippets] Unschedule fetches when the service should be disabled

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 5d601cde69c5ce0ae2c542cd60b746e116900934..92d852b41f322864c3c00705221347e5594264d3 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -194,7 +194,7 @@ NTPSnippetsService::NTPSnippetsService(
     std::unique_ptr<ImageFetcher> image_fetcher,
     std::unique_ptr<NTPSnippetsDatabase> database)
     : state_(State::NOT_INITED),
-      enabled_(false),
+      explicitly_disabled_(false),
       pref_service_(pref_service),
       sync_service_(sync_service),
       sync_service_observer_(this),
@@ -225,26 +225,19 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 
 void NTPSnippetsService::Init(bool enabled) {

[Marc Treib, 2016/05/24 09:04:46]

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)

[dgn, 2016/05/24 18:13:30]

Acknowledged.

   DCHECK(state_ == State::NOT_INITED);
-  state_ = State::INITED;
-
-  enabled_ = enabled;
-  if (enabled_) {
-    database_->Load(base::Bind(&NTPSnippetsService::OnSnippetsLoaded,
-                               base::Unretained(this)));
-  }
-
-  RescheduleFetching();
+  explicitly_disabled_ = !enabled;
+  EnterState(GetDisabledReason() == DisabledReason::NONE ? State::INITED
+                                                         : State::DISABLED);
 }
 
+// Inherited from KeyedService.
 void NTPSnippetsService::Shutdown() {
-  DCHECK(state_ == State::INITED || state_ == State::LOADED);
-  state_ = State::SHUT_DOWN;
+  EnterState(State::SHUT_DOWN);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceShutdown());
   expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
-  enabled_ = false;
 }
 
 void NTPSnippetsService::FetchSnippets() {
@@ -264,7 +257,7 @@ void NTPSnippetsService::RescheduleFetching() {
   if (!scheduler_)
     return;
 
-  if (enabled_) {
+  if (state_ == State::INITED || state_ == State::READY) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
         GetFetchingIntervalWifiCharging(), GetFetchingIntervalWifi(now),
@@ -298,7 +291,7 @@ void NTPSnippetsService::FetchSnippetImage(
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  if (!loaded())
+  if (!loaded() && state_ != State::DISABLED)
     return;
 
   database_->Delete(snippets_);
@@ -360,6 +353,18 @@ void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+DisabledReason NTPSnippetsService::GetDisabledReason() {
+  if (explicitly_disabled_)
+    return DisabledReason::EXPLICITLY_DISABLED;
+
+  if (!sync_service_ || !sync_service_->CanSyncStart() ||
+      !sync_service_->GetActiveDataTypes().Has(
+          syncer::HISTORY_DELETE_DIRECTIVES))
+    return DisabledReason::HISTORY_SYNC_DISABLED;
+
+  return DisabledReason::NONE;
+}
+
 // static
 int NTPSnippetsService::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
@@ -370,24 +375,30 @@ int NTPSnippetsService::GetMaxSnippetCountForTesting() {
 
 // sync_driver::SyncServiceObserver implementation.
 void NTPSnippetsService::OnStateChanged() {
-  if (IsSyncStateIncompatible()) {
-    ClearSnippets();
-    FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                      NTPSnippetsServiceDisabled());
+  bool enabled = GetDisabledReason() == DisabledReason::NONE;
+
+  if (enabled == (state_ != State::DISABLED))
+    return;
+
+  // |GetDisabledReason| is strict and marks the state as disabled even in cases
+  // where we miss some data. During initialization or when the user is changing
+  // the sync settings, we can run in false negatives where we mark the state
+  // as disabled. We wait do nothing here until the configuration is completely
+  // loaded to avoid unnecessary state changes.

[Marc Treib, 2016/05/24 09:04:46]

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)?

[dgn, 2016/05/24 18:13:30]

I explicitly handle the UNKNOWN state now. WDYT?

[Marc Treib, 2016/05/25 14:47:32]

SGTM!

+  if (sync_service_ && sync_service_->IsSyncActive() &&
+      !sync_service_->ConfigurationDone()) {
+    DVLOG(1) << "Sync configuration not done, skipping a state change.";
     return;
   }
 
-  // TODO(dgn): When the data sources change, we may want to not fetch here,
-  // as we will get notified of changes from the snippet sources as well, and
-  // start multiple fetches.
-  FetchSnippets();
+  EnterState(enabled ? State::INITED : State::DISABLED);

[Marc Treib, 2016/05/24 09:04:46]

What if we were already in the READY state?

[dgn, 2016/05/24 18:13:30]

Handled same state transitions as noops in EnterState.

 }
 
 void NTPSnippetsService::OnSnippetsLoaded(NTPSnippet::PtrVector snippets) {
   DCHECK(state_ == State::INITED || state_ == State::SHUT_DOWN);
   if (state_ == State::SHUT_DOWN)
-    return;
-  state_ = State::LOADED;
+    return;  // TODO(dgn) how can we get here? Just race conditons?

[Marc Treib, 2016/05/24 09:04:46]

I think it can happen if we started a fetch, then got shutdown before it
completed?

[dgn, 2016/05/24 18:13:30]

Acknowledged.

+  EnterState(State::LOADED);
 
   DCHECK(snippets_.empty());
   DCHECK(discarded_snippets_.empty());
@@ -419,7 +430,7 @@ void NTPSnippetsService::OnSnippetsLoaded(NTPSnippet::PtrVector snippets) {
 
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
-  DCHECK(loaded());
+  DCHECK(loaded() || state_ == State::DISABLED);
 
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
@@ -608,13 +619,44 @@ void NTPSnippetsService::LoadingSnippetsFinished() {
                                  base::Unretained(this)));
 }
 
-bool NTPSnippetsService::IsSyncStateIncompatible() {
-  if (!sync_service_ || !sync_service_->CanSyncStart())
-    return true;
-  if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone())
-    return false;  // Sync service is not initialized, yet not disabled.
-  return !sync_service_->GetActiveDataTypes().Has(
-      syncer::HISTORY_DELETE_DIRECTIVES);
+void NTPSnippetsService::Enable() {
+  database_->Load(base::Bind(&NTPSnippetsService::OnSnippetsLoaded,
+                             base::Unretained(this)));
+  RescheduleFetching();
+}
+
+void NTPSnippetsService::Disable() {
+  state_ = State::DISABLED;

[Marc Treib, 2016/05/24 09:04:46]

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?

[dgn, 2016/05/24 18:13:30]

Done.

+  ClearSnippets();
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceDisabled());
+  RescheduleFetching();
+}
+
+void NTPSnippetsService::EnterState(State state) {
+  switch (state) {
+    case State::NOT_INITED:
+      // Initial state, should not be possible to get back there.
+      NOTREACHED();
+      break;
+    case State::INITED:
+      DCHECK(state_ == State::NOT_INITED || state_ == State::DISABLED);
+      Enable();
+      break;
+    case State::READY:
+      DCHECK(state_ == State::INITED);
+      // TODO
+      break;
+    case State::DISABLED:
+      DCHECK(state_ == State::NOT_INITED || state_ == State::READY);
+      Disable();
+      break;
+    case State::SHUT_DOWN:
+      DCHECK(state_ == State::INITED || state_ == State::DISABLED ||
+             state_ == State::READY);
+      break;
+  }
+  state_ = state;
 }
 
 }  // namespace ntp_snippets