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

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 169 matching lines @@
 NTPSnippetsService::NTPSnippetsService(
     bool enabled,
     PrefService* pref_service,
     sync_driver::SyncService* sync_service,
     SuggestionsService* suggestions_service,
     const std::string& application_language_code,
     NTPSnippetsScheduler* scheduler,
     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),
       suggestions_service_(suggestions_service),
       application_language_code_(application_language_code),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
       database_(std::move(database)),
       fetch_after_load_(false) {
+  // TODO(dgn) should that be removed after branch point?

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

Yup - seems I forgot to put in a TODO for that?

[dgn, 2016/06/06 15:44:38]

Acknowledged.

+  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.

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

enable or disable, even!

[dgn, 2016/06/06 15:44:38]

Done.

   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() {
   DCHECK(state_ == State::SHUT_DOWN);
 }
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kDeprecatedSnippets);
   registry->RegisterListPref(prefs::kDeprecatedDiscardedSnippets);
   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();

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

Maybe move this into a helper method (EnterStateShutDown or something like
that), analogous to the Enable/Disable methods?

[dgn, 2016/06/06 15:44:39]

Done.

-  enabled_ = false;
 }
 
 void NTPSnippetsService::FetchSnippets() {
-  if (loaded())
+  if (ready())
     FetchSnippetsFromHosts(GetSuggestionsHosts());
   else
     fetch_after_load_ = true;
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
-  if (!loaded())
+  if (!ready())
     return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount);
 }
 
 void NTPSnippetsService::RescheduleFetching() {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
-  if (enabled_) {
+  if (ready()) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
         GetFetchingIntervalWifiCharging(), GetFetchingIntervalWifi(now),
         GetFetchingIntervalFallback(), GetRescheduleTime(now));
   } else {
     scheduler_->Unschedule();
   }
 }
 
 void NTPSnippetsService::FetchSnippetImage(
@@ skipping 10 matching lines @@
     return;
   }
 
   const NTPSnippet& snippet = *it->get();
   image_fetcher_->StartOrQueueNetworkRequest(
       snippet.id(), snippet.salient_image_url(), callback);
   // TODO(treib): Cache/persist the snippet image.
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  if (!loaded())
+  if (!initialized())
+    return;
+
+  if (snippets_.empty())
     return;
 
   database_->Delete(snippets_);
   snippets_.clear();
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 }
 
 std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
   // |suggestions_service_| can be null in tests.
   if (!suggestions_service_)
     return std::set<std::string>();
 
   // TODO(treib): This should just call GetSnippetHostsFromPrefs.
   return GetSuggestionsHostsImpl(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
-  if (!loaded())
+  if (!ready())
     return false;
 
   auto it =
       std::find_if(snippets_.begin(), snippets_.end(),
                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
                      return snippet->id() == snippet_id;
                    });
   if (it == snippets_.end())
     return false;
 
   (*it)->set_discarded(true);
 
   database_->Save(**it);
 
   discarded_snippets_.push_back(std::move(*it));
   snippets_.erase(it);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
   return true;
 }
 
 void NTPSnippetsService::ClearDiscardedSnippets() {
-  if (!loaded())
+  if (!initialized())
     return;
 
   database_->Delete(discarded_snippets_);
   discarded_snippets_.clear();
 
   FetchSnippets();

[dgn, 2016/06/06 15:44:39]

Removed so that we don't fetch while disabling the service. added fetch in
previous call sites.

[Marc Treib, 2016/06/07 08:55:38]

Acknowledged.

 }
 
 void NTPSnippetsService::AddObserver(NTPSnippetsServiceObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 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.

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

s/components/data types
?

[dgn, 2016/06/06 15:44:38]

Done.

+  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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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);
+  DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN);
   if (state_ == State::SHUT_DOWN)
     return;
-  state_ = State::LOADED;
 
   DCHECK(snippets_.empty());
   DCHECK(discarded_snippets_.empty());
   for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
     if (snippet->is_discarded())
       discarded_snippets_.emplace_back(std::move(snippet));
     else
       snippets_.emplace_back(std::move(snippet));
   }
   std::sort(snippets_.begin(), snippets_.end(),
             [](const std::unique_ptr<NTPSnippet>& lhs,
                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())
     return;
 
   // Remove existing snippets that aren't in the suggestions anymore.
   // TODO(treib,maybelle): If there is another source with an allowed host,
   // then we should fall back to that.
   // First, move them over into |to_delete|.
   NTPSnippet::PtrVector to_delete;
   for (std::unique_ptr<NTPSnippet>& snippet : snippets_) {
     if (!hosts.count(snippet->best_source().url.host()))
       to_delete.emplace_back(std::move(snippet));
   }
   Compact(&snippets_);
   // Then delete the removed snippets from the database.
   database_->Delete(to_delete);
 
   StoreSnippetHostsToPrefs(hosts);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
   FetchSnippetsFromHosts(hosts);
 }
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
-  if (!loaded())
+  if (!ready())
     return;
 
   if (snippets) {
     // Sparse histogram used because the number of snippets is small (bound by
     // kMaxSnippetCount).
     DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
   LoadingSnippetsFinished();
 }
 
 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;
   InsertAllIDs(discarded_snippets_, &old_snippet_ids);
   InsertAllIDs(snippets_, &old_snippet_ids);
   new_snippets.erase(
       std::remove_if(
           new_snippets.begin(), new_snippets.end(),
           [&old_snippet_ids](const std::unique_ptr<NTPSnippet>& snippet) {
             if (old_snippet_ids.count(snippet->id()))
@@ skipping 62 matching lines @@
 
 void NTPSnippetsService::StoreSnippetHostsToPrefs(
     const std::set<std::string>& hosts) {
   base::ListValue list;
   for (const std::string& host : hosts)
     list.AppendString(host);
   pref_service_->Set(prefs::kSnippetHosts, list);
 }
 
 void NTPSnippetsService::LoadingSnippetsFinished() {
-  DCHECK(loaded());
+  DCHECK(ready() || state_ == State::NOT_INITED /* for OnDatabaseLoaded */);

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

In OnDatabaseLoaded, could we call EnterState before LoadingSnippetsFinished?

[dgn, 2016/06/06 15:44:38]

Done.

 
   // Remove expired snippets.
   base::Time expiry = base::Time::Now();
 
   // Move expired snippets over into |to_delete|.
   NTPSnippet::PtrVector to_delete;
   for (std::unique_ptr<NTPSnippet>& snippet : snippets_) {
     if (snippet->expiry_date() <= expiry)
       to_delete.emplace_back(std::move(snippet));
   }
@@ skipping 41 matching lines @@
   for (const auto& snippet : discarded_snippets_) {
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
                       base::Bind(&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();

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

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.

[dgn, 2016/06/06 15:44:38]

No, we should not. It's guarded by a DCHECK. But we don't run the shutdown
sequence on android, Chrome just gets killed.

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

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

Make this a switch, so it's clear that all possible values are handled?

[dgn, 2016/06/06 15:44:38]

Done.

+    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:
+      DCHECK(state_ == State::NOT_INITED || state_ == State::READY ||
+             state_ == State::DISABLED);
+      if (state_ == State::READY)
+        return;
+
+      DVLOG(1) << "Entering state: READY";
+      state_ = State::READY;
+      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() {
   pref_service_->ClearPref(prefs::kDeprecatedSnippets);
   pref_service_->ClearPref(prefs::kDeprecatedDiscardedSnippets);
 }
 
 }  // namespace ntp_snippets