[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 60a87b6895d1e8f6ef773ca95d6fe22c8ad1912c..fca84f21522a43fe6377599654339d130822f245 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -187,8 +187,8 @@ NTPSnippetsService::NTPSnippetsService(
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<ImageFetcher> image_fetcher,
     std::unique_ptr<NTPSnippetsDatabase> database)
-    : state_(State::INITED),
-      enabled_(enabled),
+    : state_(State::NOT_INITED),
+      explicitly_disabled_(!enabled),
       pref_service_(pref_service),
       sync_service_(sync_service),
       sync_service_observer_(this),
@@ -199,21 +199,26 @@ NTPSnippetsService::NTPSnippetsService(
       image_fetcher_(std::move(image_fetcher)),
       database_(std::move(database)),
       fetch_after_load_(false) {
+  // TODO(dgn) should that be removed after branch point?
+  ClearDeprecatedPrefs();
+
+  if (explicitly_disabled_) {
+    EnterState(State::DISABLED);
+    return;
+  }
+
   snippets_fetcher_->SetCallback(base::Bind(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 
   // |sync_service_| can be null in tests or if sync is disabled.
+  // This is a service we want to keep listening to all the time, independently
+  // from the state, since it will allow us to enable the snippets service.
   if (sync_service_)
     sync_service_observer_.Add(sync_service_);
 
-  if (enabled_) {
-    database_->Load(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
-                               base::Unretained(this)));
-  }
-
-  RescheduleFetching();
-
-  ClearDeprecatedPrefs();
+  // Will trigger the transition to the READY state.
+  database_->Load(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
+                             base::Unretained(this)));
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
@@ -227,19 +232,18 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippetHosts);
 }
 
+// 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() {
-  if (loaded())
+  if (ready())
     FetchSnippetsFromHosts(GetSuggestionsHosts());
   else
     fetch_after_load_ = true;
@@ -247,7 +251,7 @@ void NTPSnippetsService::FetchSnippets() {
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
-  if (!loaded())
+  if (!ready())
     return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount);
@@ -258,7 +262,7 @@ void NTPSnippetsService::RescheduleFetching() {
   if (!scheduler_)
     return;
 
-  if (enabled_) {
+  if (ready()) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
         GetFetchingIntervalWifiCharging(), GetFetchingIntervalWifi(now),
@@ -289,7 +293,10 @@ void NTPSnippetsService::FetchSnippetImage(
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  if (!loaded())
+  if (!initialized())
+    return;
+
+  if (snippets_.empty())
     return;
 
   database_->Delete(snippets_);
@@ -310,7 +317,7 @@ std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
 }
 
 bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
-  if (!loaded())
+  if (!ready())
     return false;
 
   auto it =
@@ -334,7 +341,7 @@ bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
 }
 
 void NTPSnippetsService::ClearDiscardedSnippets() {
-  if (!loaded())
+  if (!initialized())
     return;
 
   database_->Delete(discarded_snippets_);
@@ -351,6 +358,34 @@ void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+DisabledReason NTPSnippetsService::GetDisabledReason() {
+  if (explicitly_disabled_)
+    return DisabledReason::EXPLICITLY_DISABLED;
+
+  if (!sync_service_ || !sync_service_->CanSyncStart()) {
+    DVLOG(1) << "[GetDisabledReason] Sync disabled";
+    return DisabledReason::HISTORY_SYNC_DISABLED;
+  }
+
+  // !IsSyncActive in cases where CanSyncStart is true hints at the backend not
+  // being initialized.
+  // ConfigurationDone() verifies that the sync service has properly loaded its
+  // configuration and is aware of the different components to sync.
+  if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone()) {
+    DVLOG(1) << "[GetDisabledReason] Sync initialization is not complete.";
+    return DisabledReason::HISTORY_SYNC_STATE_UNKNOWN;
+  }
+
+  if (!sync_service_->GetActiveDataTypes().Has(
+          syncer::HISTORY_DELETE_DIRECTIVES)) {
+    DVLOG(1) << "[GetDisabledReason] History sync disabled";
+    return DisabledReason::HISTORY_SYNC_DISABLED;
+  }
+
+  DVLOG(1) << "[GetDisabledReason] Enabled";
+  return DisabledReason::NONE;
+}
+
 // static
 int NTPSnippetsService::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
@@ -359,26 +394,18 @@ int NTPSnippetsService::GetMaxSnippetCountForTesting() {
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
-// sync_driver::SyncServiceObserver implementation.
 void NTPSnippetsService::OnStateChanged() {
-  if (IsSyncStateIncompatible()) {
-    ClearSnippets();
-    FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                      NTPSnippetsServiceDisabled());
+  if (!initialized())
     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();
+  DVLOG(1) << "[OnStateChanged]";
+  EnterState(GetStateForDependenciesStatus());
 }
 
 void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
   DCHECK(state_ == State::INITED || state_ == State::SHUT_DOWN);
   if (state_ == State::SHUT_DOWN)
     return;
-  state_ = State::LOADED;
 
   DCHECK(snippets_.empty());
   DCHECK(discarded_snippets_.empty());
@@ -393,27 +420,13 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
                const std::unique_ptr<NTPSnippet>& rhs) {
               return lhs->score() > rhs->score();
             });
-  LoadingSnippetsFinished();
 
-  // If host restrictions are enabled, register for host list updates.
-  // |suggestions_service_| can be null in tests.
-  if (snippets_fetcher_->UsesHostRestrictions() && suggestions_service_) {
-    suggestions_service_subscription_ =
-        suggestions_service_->AddCallback(base::Bind(
-            &NTPSnippetsService::OnSuggestionsChanged, base::Unretained(this)));
-  }
-
-  // Start a fetch if we don't have any snippets yet, or a fetch was requested
-  // earlier.
-  if (snippets_.empty() || fetch_after_load_) {
-    fetch_after_load_ = false;
-    FetchSnippets();
-  }
+  EnterState(GetStateForDependenciesStatus());
 }
 
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
-  DCHECK(loaded());
+  DCHECK(initialized());
 
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
@@ -442,7 +455,7 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
-  if (!loaded())
+  if (!ready())
     return;
 
   if (snippets) {
@@ -457,7 +470,7 @@ void NTPSnippetsService::OnFetchFinished(
 }
 
 void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
-  DCHECK(loaded());
+  DCHECK(ready());
 
   // Remove new snippets that we already have, or that have been discarded.
   std::set<std::string> old_snippet_ids;
@@ -540,7 +553,7 @@ void NTPSnippetsService::StoreSnippetHostsToPrefs(
 }
 
 void NTPSnippetsService::LoadingSnippetsFinished() {
-  DCHECK(loaded());
+  DCHECK(ready());
 
   // Remove expired snippets.
   base::Time expiry = base::Time::Now();
@@ -602,13 +615,102 @@ 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() {
+  // Start a fetch if we don't have any snippets yet, or a fetch was requested
+  // earlier.
+  if (snippets_.empty() || fetch_after_load_) {
+    fetch_after_load_ = false;
+    FetchSnippets();
+  }
+
+  // If host restrictions are enabled, register for host list updates.
+  // |suggestions_service_| can be null in tests.
+  if (snippets_fetcher_->UsesHostRestrictions() && suggestions_service_) {
+    suggestions_service_subscription_ =
+        suggestions_service_->AddCallback(base::Bind(
+            &NTPSnippetsService::OnSuggestionsChanged, base::Unretained(this)));
+  }
+
+  RescheduleFetching();
+}
+
+void NTPSnippetsService::Disable() {
+  // TODO(dgn) should we also clear the discarded snippets?
+  ClearSnippets();
+
+  suggestions_service_subscription_.reset();
+  expiry_timer_.Stop();
+
+  RescheduleFetching();
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceDisabled());
+}
+
+NTPSnippetsService::State NTPSnippetsService::GetStateForDependenciesStatus() {
+  DisabledReason dr = GetDisabledReason();
+
+  if (dr == DisabledReason::NONE)
+    return State::READY;
+
+  // HistorySync is not initialized yet, so we don't know what the actual state
+  // is. However, because we retreived the previous snippets from the database,
+  // if we got something, we know that the service was previously enabled, so
+  // we just restore that state. If things changed, |OnStateChanged| will call
+  // this function again to fix the state.
+  if (dr == DisabledReason::HISTORY_SYNC_STATE_UNKNOWN) {
+    DVLOG(1) << "Sync configuration not done, continuing based on the current "
+                "state.";
+    return snippets_.empty() ? State::DISABLED : State::READY;
+  }
+
+  return State::DISABLED;
+}
+
+void NTPSnippetsService::EnterState(State state) {
+  switch (state) {
+    case State::NOT_INITED:
+      // Initial state, it should not be possible to get back there.
+      NOTREACHED();
+      return;
+
+    case State::READY: {

[dgn, 2016/06/03 19:42:23]

maybe "ENABLED" is better?

[Marc Treib, 2016/06/06 08:38:32]

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.

+      DCHECK(state_ == State::NOT_INITED || state_ == State::READY ||
+             state_ == State::DISABLED);
+      if (state_ == State::READY)
+        return;
+
+      bool is_first_init = state_ == State::NOT_INITED;
+      DVLOG(1) << "Entering state: READY";
+      state_ = State::READY;
+
+      // At the first initialization, we loaded snippets from the database.
+      // We should notify listeners about them. We do so here because we have
+      // to be in the READY state.
+      if (is_first_init)
+        LoadingSnippetsFinished();
+
+      Enable();
+      return;
+    }
+
+    case State::DISABLED:
+      DCHECK(state_ == State::NOT_INITED || state_ == State::READY ||
+             state_ == State::DISABLED);
+      if (state_ == State::DISABLED)
+        return;
+
+      DVLOG(1) << "Entering state: DISABLED";
+      state_ = State::DISABLED;
+      Disable();
+      return;
+
+    case State::SHUT_DOWN:
+      DCHECK(state_ == State::NOT_INITED || state_ == State::DISABLED ||
+             state_ == State::READY);
+      DVLOG(1) << "Entering state: SHUT_DOWN";
+      state_ = State::SHUT_DOWN;
+      return;
+  }
 }
 
 void NTPSnippetsService::ClearDeprecatedPrefs() {