[NTP Snippets] More reliable service initialization

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 189 matching lines @@
       database_(std::move(database)),
       fetch_after_load_(false) {
   // TODO(dgn) should be removed after branch point (https:://crbug.com/617585).
   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 or disable the snippets
-  // service.
-  if (sync_service_)
-    sync_service_observer_.Add(sync_service_);
-
-  // Will trigger the transition to the READY state.
+  // We transition to other states while finalizing the initialization, when the
+  // database is done loading.
   database_->Load(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
                              base::Unretained(this)));
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
   DCHECK(state_ == State::SHUT_DOWN);
 }
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
@@ skipping 151 matching lines @@
 
 // static
 int NTPSnippetsService::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 void NTPSnippetsService::OnStateChanged() {
-  if (!initialized())
+  if (state_ == State::SHUT_DOWN)
     return;
 
   DVLOG(1) << "[OnStateChanged]";
   EnterState(GetStateForDependenciesStatus());
 }
 
 void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
   DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN);
   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();
             });
 
-  // Change state after we started loading the snippets. During startup, the
-  // Sync service might not be completely loaded when we initialize this
-  // service. Whether we had snippets in the last season is used to guess if
-  // we should enable the service or not. See |GetStateForDependenciesStatus|.
-  EnterState(GetStateForDependenciesStatus());
-
-  LoadingSnippetsFinished();
-
-  // 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();
-  }
+  ClearExpiredSnippets();
+  FinishInitialization();
 }
 
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
   DCHECK(initialized());
 
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
     return;
 
@@ skipping 110 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(ready());
-
-  // Remove expired snippets.
+void NTPSnippetsService::ClearExpiredSnippets() {
   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));
   }
   Compact(&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);
-  }
-
   // 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));
   }
   Compact(&discarded_snippets_);
 
   // Finally, actually delete the removed snippets from the DB.
   database_->Delete(to_delete);
 
-  UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
-                              snippets_.size());
-  if (snippets_.empty() && !discarded_snippets_.empty()) {
-    UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
-                         discarded_snippets_.size());
-  }
-
-  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                    NTPSnippetsServiceLoaded());
-
   // If there are any snippets left, schedule a timer for the next expiry.
   if (snippets_.empty() && discarded_snippets_.empty())
     return;
 
   base::Time next_expiry = base::Time::Max();
   for (const auto& snippet : snippets_) {
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   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::Bind(&NTPSnippetsService::ClearExpiredSnippets,
                                  base::Unretained(this)));
 }
 
+void NTPSnippetsService::LoadingSnippetsFinished() {

[Bernhard Bauer, 2016/06/09 15:21:33]

This is now more FetchingSnippetsFinished(). You could also inline it into
OnFetchFinished(), as that's the only call site.

[dgn, 2016/06/09 16:03:38]

Done.

+  DCHECK(ready());
+
+  ClearExpiredSnippets();
+
+  // If there are still more snippets than we want to show, move the extra ones
+  // over into |to_delete| as well.

[Marc Treib, 2016/06/09 15:52:44]

nit: remove "as well"

[dgn, 2016/06/09 16:03:38]

Done.

+  NTPSnippet::PtrVector to_delete;
+  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);
+  }
+  database_->Delete(to_delete);
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
+                              snippets_.size());
+  if (snippets_.empty() && !discarded_snippets_.empty()) {
+    UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
+                         discarded_snippets_.size());
+  }
+
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceLoaded());
+}
+
 void NTPSnippetsService::EnterStateEnabled(bool fetch_snippets) {
   if (fetch_snippets)
     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)));
@@ skipping 18 matching lines @@
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceShutdown());
 
   expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
 
   if (sync_service_)
     sync_service_observer_.Remove(sync_service_);
 }
 
+void NTPSnippetsService::FinishInitialization() {
+  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 or disable the snippets
+  // service.
+  if (sync_service_)
+    sync_service_observer_.Add(sync_service_);
+
+  // Change state after we started loading the snippets. During startup, the
+  // Sync service might not be completely loaded when we initialize this
+  // service. Whether we had snippets in the last season is used to guess if

[Bernhard Bauer, 2016/06/09 15:21:33]

"last session" :)

[dgn, 2016/06/09 16:03:38]

Thanks. The comment was outdated also u_u

+  // we should enable the service or not. See |GetStateForDependenciesStatus|.
+  EnterState(GetStateForDependenciesStatus());
+
+  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
+                    NTPSnippetsServiceLoaded());
+
+  // Start a fetch if we don't have any snippets yet, or a fetch was requested
+  // earlier.
+  if (ready() && (snippets_.empty() || fetch_after_load_)) {
+    fetch_after_load_ = false;
+    FetchSnippets();
+  }
+}
+
 NTPSnippetsService::State NTPSnippetsService::GetStateForDependenciesStatus() {
   switch (GetDisabledReason()) {
     case DisabledReason::NONE:
       return State::READY;
 
     case DisabledReason::HISTORY_SYNC_STATE_UNKNOWN:
       // 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.
+      // state is and we just return the current one. If things change,
+      // |OnStateChanged| will call this function again to update the state.
       DVLOG(1) << "Sync configuration incomplete, continuing based on the "
              "current state.";
-      return snippets_.empty() ? State::DISABLED : State::READY;
+      return state_;
 
     case DisabledReason::EXPLICITLY_DISABLED:
     case DisabledReason::HISTORY_SYNC_DISABLED:
       return State::DISABLED;
   }
 
   // All cases should be handled by the above switch
   NOTREACHED();
   return State::DISABLED;
 }
 
 void NTPSnippetsService::EnterState(State state) {
+  if (state == state_)
+    return;
+
   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;
+      DCHECK(state_ == State::NOT_INITED || state_ == State::DISABLED);
 
       // If the service was previously disabled, we will need to start a fetch
       // because otherwise there won't be any.
-      bool fetch_snippets = state_ == State::DISABLED;
+      bool fetch_snippets = state_ == State::DISABLED || fetch_after_load_;
       DVLOG(1) << "Entering state: READY";
       state_ = State::READY;
+      fetch_after_load_ = false;
       EnterStateEnabled(fetch_snippets);
       return;
     }
 
     case State::DISABLED:
-      DCHECK(state_ == State::NOT_INITED || state_ == State::READY ||
-             state_ == State::DISABLED);
-      if (state_ == State::DISABLED)
-        return;
+      DCHECK(state_ == State::NOT_INITED || state_ == State::READY);
 
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       EnterStateDisabled();
       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;
       EnterStateShutdown();
       return;
   }
 }
 
 void NTPSnippetsService::ClearDeprecatedPrefs() {
   pref_service_->ClearPref(prefs::kDeprecatedSnippets);
   pref_service_->ClearPref(prefs::kDeprecatedDiscardedSnippets);
 }
 
 }  // namespace ntp_snippets