[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 4da4e50a3ddeb8950cafb65e3d8ba89a28e7d58b..0bf12b545ae3361094d5a3ecf99922df80d8569e 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -21,6 +21,7 @@
 #include "base/values.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
+#include "components/ntp_snippets/ntp_snippets_database.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -157,16 +158,6 @@ std::set<std::string> GetSuggestionsHostsImpl(
   return hosts;
 }
 
-std::unique_ptr<base::ListValue> SnippetsToListValue(
-    const NTPSnippet::PtrVector& snippets) {
-  std::unique_ptr<base::ListValue> list(new base::ListValue);
-  for (const auto& snippet : snippets) {
-    std::unique_ptr<base::DictionaryValue> dict = snippet->ToDictionary();
-    list->Append(std::move(dict));
-  }
-  return list;
-}
-
 void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
                   std::set<std::string>* ids) {
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
@@ -176,6 +167,14 @@ void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
   }
 }
 
+void EraseNullptrs(NTPSnippet::PtrVector* snippets) {
+  snippets->erase(
+      std::remove_if(
+          snippets->begin(), snippets->end(),
+          [](const std::unique_ptr<NTPSnippet>& snippet) { return !snippet; }),
+      snippets->end());
+}
+
 void WrapImageFetchedCallback(
     const NTPSnippetsService::ImageFetchedCallback& callback,
     const GURL& snippet_id_url,
@@ -189,22 +188,22 @@ NTPSnippetsService::NTPSnippetsService(
     PrefService* pref_service,
     sync_driver::SyncService* sync_service,
     SuggestionsService* suggestions_service,
-    scoped_refptr<base::SequencedTaskRunner> file_task_runner,
     const std::string& application_language_code,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
-    std::unique_ptr<ImageFetcher> image_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),
-      file_task_runner_(file_task_runner),
       application_language_code_(application_language_code),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
-      image_fetcher_(std::move(image_fetcher)) {
+      image_fetcher_(std::move(image_fetcher)),
+      database_(std::move(database)) {
   snippets_fetcher_->SetCallback(base::Bind(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 
@@ -225,38 +224,18 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 }
 
 void NTPSnippetsService::Init(bool enabled) {
-  DCHECK(state_ == State::NOT_INITED);
-  state_ = State::INITED;
-
-  enabled_ = enabled;
-  if (enabled_) {
-    // |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)));
-    }
-
-    // Get any existing snippets immediately from prefs.
-    LoadDiscardedSnippetsFromPrefs();
-    LoadSnippetsFromPrefs();
-
-    // If we don't have any snippets yet, start a fetch.
-    if (snippets_.empty())
-      FetchSnippets();
-  }
-
-  RescheduleFetching();
+  explicitly_disabled_ = !enabled;
+  EnterState(!explicitly_disabled_ ? State::INITED : State::DISABLED);
 }
 
+// Inherited from KeyedService.
 void NTPSnippetsService::Shutdown() {
-  DCHECK(state_ == State::INITED);
-  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() {
@@ -265,6 +244,8 @@ void NTPSnippetsService::FetchSnippets() {
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
+  if (!ready())
+    return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount);
 }
@@ -274,7 +255,7 @@ void NTPSnippetsService::RescheduleFetching() {
   if (!scheduler_)
     return;
 
-  if (enabled_) {
+  if (ready()) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
         GetFetchingIntervalWifiCharging(), GetFetchingIntervalWifi(now),
@@ -308,10 +289,17 @@ void NTPSnippetsService::FetchSnippetImage(
 }
 
 void NTPSnippetsService::ClearSnippets() {
-  snippets_.clear();
+  // snippets-internals can call ClearSnippets while the service is ready.

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

Er, isn't it acceptable for anyone to call ClearSnippets while the service is
ready? It's part of the public interface after all.

[dgn, 2016/06/03 19:02:24]

True, removed that comment.

+  if (!ready() && state_ != State::DISABLED)
+    return;
 
-  StoreSnippetsToPrefs();
+  if (snippets_.empty())
+    return;
 
+  database_->Delete(snippets_);
+  snippets_.clear();
+
+  DVLOG(1) << "[ClearSnippets] NTPSnippetsServiceLoaded";
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 }
@@ -321,12 +309,15 @@ std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
   if (!suggestions_service_)
     return std::set<std::string>();
 
-  // TODO(treib) this should just call GetSnippetHostsFromPrefs
+  // TODO(treib): This should just call GetSnippetHostsFromPrefs.
   return GetSuggestionsHostsImpl(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
+  if (!ready())
+    return false;
+
   auto it =
       std::find_if(snippets_.begin(), snippets_.end(),
                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
@@ -334,18 +325,26 @@ bool NTPSnippetsService::DiscardSnippet(const std::string& 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);
-  StoreDiscardedSnippetsToPrefs();
-  StoreSnippetsToPrefs();
+
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
   return true;
 }
 
 void NTPSnippetsService::ClearDiscardedSnippets() {
+  if (!ready())
+    return;
+
+  database_->Delete(discarded_snippets_);
   discarded_snippets_.clear();
-  StoreDiscardedSnippetsToPrefs();
+
   FetchSnippets();
 }
 
@@ -357,6 +356,30 @@ 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;
+  }
+
+  if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone()) {

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

I think the ConfigurationDone check is redundant - if it's active, then
configuration must be done too.

[dgn, 2016/06/03 19:02:24]

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.

+    DVLOG(1) << "[GetDisabledReason] config not done";
+    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;
@@ -367,21 +390,40 @@ int NTPSnippetsService::GetMaxSnippetCountForTesting() {
 
 // sync_driver::SyncServiceObserver implementation.
 void NTPSnippetsService::OnStateChanged() {
-  if (IsSyncStateIncompatible()) {
-    ClearSnippets();
-    FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                      NTPSnippetsServiceDisabled());
+  DVLOG(1) << "[OnStateChanged]";
+  EnterState(GetStateForDependenciesStatus());

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

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

[dgn, 2016/06/03 19:02:24]

Done.

+}
+
+void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
+  if (state_ == State::SHUT_DOWN)
     return;
+
+  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();
+            });
+
+  if (!snippets_.empty()) {
+    DVLOG(1) << "[OnDatabaseLoaded] got snippets";
+    LoadingSnippetsFinished();
   }
 
-  // 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(GetStateForDependenciesStatus());
 }
 
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
+  DCHECK(ready() || state_ == State::DISABLED);
+
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
     return;
@@ -389,16 +431,19 @@ void NTPSnippetsService::OnSuggestionsChanged(
   // 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.
-  snippets_.erase(
-      std::remove_if(snippets_.begin(), snippets_.end(),
-                     [&hosts](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return !hosts.count(snippet->best_source().url.host());
-                     }),
-      snippets_.end());
-
-  StoreSnippetsToPrefs();
+  // 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));
+  }
+  EraseNullptrs(&snippets_);
+  // Then delete the removed snippets from the database.
+  database_->Delete(to_delete);
+
   StoreSnippetHostsToPrefs(hosts);
 
+  DVLOG(1) << "[OnSuggestionsChanged] NTPSnippetsServiceLoaded";
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
@@ -407,18 +452,26 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
+  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));
+    DVLOG(1) << "[OnFetchFinished] histogram -> NumArticlesFetched";
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
+
+  DVLOG(1) << "[OnFetchFinished]";
   LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
+  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);
@@ -462,44 +515,25 @@ void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
                        }),
         new_snippets.end());
     int num_snippets_discarded = num_new_snippets - new_snippets.size();
+    DVLOG(1) << "[MergeSnippets] histogram -> IncompleteSnippetsAfterFetch";
     UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
                           num_snippets_discarded > 0);
     if (num_snippets_discarded > 0) {
+      DVLOG(1) << "[MergeSnippets] histogram -> NumIncompleteSnippets";
       UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
                                   num_snippets_discarded);
     }
   }
+
+  // Save the new snippets to the DB.
+  database_->Save(new_snippets);
+
   // Insert the new snippets at the front.
   snippets_.insert(snippets_.begin(),
                    std::make_move_iterator(new_snippets.begin()),
                    std::make_move_iterator(new_snippets.end()));
 }
 
-void NTPSnippetsService::LoadSnippetsFromPrefs() {
-  NTPSnippet::PtrVector prefs_snippets;
-  bool success = NTPSnippet::AddFromListValue(
-      *pref_service_->GetList(prefs::kSnippets), &prefs_snippets);
-  DCHECK(success) << "Failed to parse snippets from prefs";
-  MergeSnippets(std::move(prefs_snippets));
-  LoadingSnippetsFinished();
-}
-
-void NTPSnippetsService::StoreSnippetsToPrefs() {
-  pref_service_->Set(prefs::kSnippets, *SnippetsToListValue(snippets_));
-}
-
-void NTPSnippetsService::LoadDiscardedSnippetsFromPrefs() {
-  discarded_snippets_.clear();
-  bool success = NTPSnippet::AddFromListValue(
-      *pref_service_->GetList(prefs::kDiscardedSnippets), &discarded_snippets_);
-  DCHECK(success) << "Failed to parse discarded snippets from prefs";
-}
-
-void NTPSnippetsService::StoreDiscardedSnippetsToPrefs() {
-  pref_service_->Set(prefs::kDiscardedSnippets,
-                     *SnippetsToListValue(discarded_snippets_));
-}
-
 std::set<std::string> NTPSnippetsService::GetSnippetHostsFromPrefs() const {
   std::set<std::string> hosts;
   const base::ListValue* list = pref_service_->GetList(prefs::kSnippetHosts);
@@ -521,34 +555,46 @@ void NTPSnippetsService::StoreSnippetHostsToPrefs(
 }
 
 void NTPSnippetsService::LoadingSnippetsFinished() {
+  DCHECK(ready() || state_ == State::INITED);
+
   // Remove expired snippets.
   base::Time expiry = base::Time::Now();
 
-  snippets_.erase(
-      std::remove_if(snippets_.begin(), snippets_.end(),
-                     [&expiry](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return snippet->expiry_date() <= expiry;
-                     }),
-      snippets_.end());
-
-  // If there are more snippets now than we want to show, drop the extra ones
-  // from the end of the list.
-  if (snippets_.size() > kMaxSnippetCount)
+  // 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));
+  }
+  EraseNullptrs(&snippets_);
+
+  // If there are still more snippets than we want to show, move the extra ones
+  // over into |to_delete| as well.
+  if (snippets_.size() > kMaxSnippetCount) {
+    to_delete.insert(
+        to_delete.end(),
+        std::make_move_iterator(snippets_.begin() + kMaxSnippetCount),
+        std::make_move_iterator(snippets_.end()));
     snippets_.resize(kMaxSnippetCount);
+  }
 
-  StoreSnippetsToPrefs();
+  // Move expired discarded snippets over into |to_delete| as well.
+  for (std::unique_ptr<NTPSnippet>& snippet : discarded_snippets_) {
+    if (snippet->expiry_date() <= expiry)
+      to_delete.emplace_back(std::move(snippet));
+  }
+  EraseNullptrs(&discarded_snippets_);
 
-  discarded_snippets_.erase(
-      std::remove_if(discarded_snippets_.begin(), discarded_snippets_.end(),
-                     [&expiry](const std::unique_ptr<NTPSnippet>& snippet) {
-                       return snippet->expiry_date() <= expiry;
-                     }),
-      discarded_snippets_.end());
-  StoreDiscardedSnippetsToPrefs();
+  // Finally, actually delete the removed snippets from the DB.
+  database_->Delete(to_delete);
 
+  DVLOG(1) << "[LoadingSnippetsFinished] NTPSnippetsServiceLoaded -> Histogram "
+              "NumArticles";
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               snippets_.size());
   if (snippets_.empty() && !discarded_snippets_.empty()) {
+    DVLOG(1) << "[LoadingSnippetsFinished] NTPSnippetsServiceLoaded -> "
+                "Histogram NumArticlesZeroDueToDiscarded";
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          discarded_snippets_.size());
   }
@@ -575,13 +621,99 @@ 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::LoadFromDB() {
+  database_->Load(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
+                             base::Unretained(this)));
+}
+
+void NTPSnippetsService::Enable() {
+  // We might already have snippets from loading the DB and want to avoid a
+  // double load.
+  if (snippets_.empty())
+    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() {
+  ClearSnippets();
+
+  suggestions_service_subscription_.reset();
+  RescheduleFetching();
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceDisabled());
+}
+
+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::INITED:
+      DCHECK(state_ == State::NOT_INITED);
+      state_ = State::INITED;
+      LoadFromDB();
+      DVLOG(1) << "Entering state: INITED";
+      return;
+
+    case State::READY:
+      DCHECK(state_ == State::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 || state_ == State::INITED);
+      if (state_ == State::DISABLED)
+        return;
+
+      DVLOG(1) << "Entering state: DISABLED";
+      state_ = State::DISABLED;
+      Disable();
+      return;
+
+    case State::SHUT_DOWN:
+      DCHECK(state_ == State::INITED || state_ == State::DISABLED ||
+             state_ == State::READY);
+      DVLOG(1) << "Entering state: SHUT_DOWN";
+      state_ = State::SHUT_DOWN;
+      return;
+  }
 }
 
 }  // namespace ntp_snippets