[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 176 matching lines @@
 NTPSnippetsService::NTPSnippetsService(
     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::NOT_INITED),
-      enabled_(false),
+      explicitly_disabled_(false),
       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)) {
   snippets_fetcher_->SetCallback(base::Bind(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 
   // |sync_service_| can be null in tests or if sync is disabled.
   if (sync_service_)
     sync_service_observer_.Add(sync_service_);
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
   DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN);
 }
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippets);
   registry->RegisterListPref(prefs::kDiscardedSnippets);
   registry->RegisterListPref(prefs::kSnippetHosts);
 }
 
 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() {
   FetchSnippetsFromHosts(GetSuggestionsHosts());
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
   if (!loaded())
     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 (state_ == State::INITED || state_ == State::READY) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
         GetFetchingIntervalWifiCharging(), GetFetchingIntervalWifi(now),
         GetFetchingIntervalFallback(), GetRescheduleTime(now));
   } else {
     scheduler_->Unschedule();
   }
 }
 
 void NTPSnippetsService::FetchSnippetImage(
@@ skipping 13 matching lines @@
   const NTPSnippet& snippet = *it->get();
   // TODO(treib): Make ImageFetcher take a string instead of a GURL as an
   // identifier.
   image_fetcher_->StartOrQueueNetworkRequest(
       GURL(snippet.id()), snippet.salient_image_url(),
       base::Bind(WrapImageFetchedCallback, callback));
   // TODO(treib): Cache/persist the snippet image.
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  if (!loaded())
+  if (!loaded() && state_ != State::DISABLED)
     return;
 
   database_->Delete(snippets_);
   snippets_.clear();
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 }
 
 std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
@@ skipping 41 matching lines @@
 }
 
 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() ||
+      !sync_service_->GetActiveDataTypes().Has(
+          syncer::HISTORY_DELETE_DIRECTIVES))
+    return DisabledReason::HISTORY_SYNC_DISABLED;
+
+  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());
+  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());
   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(),
@@ skipping 11 matching lines @@
             &NTPSnippetsService::OnSuggestionsChanged, base::Unretained(this)));
   }
 
   // If we don't have any snippets yet, start a fetch.
   if (snippets_.empty())
     FetchSnippets();
 }
 
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
-  DCHECK(loaded());
+  DCHECK(loaded() || state_ == State::DISABLED);
 
   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;
@@ skipping 168 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() {
+  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