📰Fix SnippetsService's status reporting

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 c9c3441405458e2bb7981fcd4e68f6519f716515..71133d1229ed3815bc11e63e775cbf138fa14251 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -259,6 +259,10 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
     bool force_request) {
   if (!ready())
     return;
+
+  if (snippets_.empty())
+    UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE_LOADING);

[Marc Treib, 2016/08/01 09:03:31]

Hm, so this status effectively means "no suggestions, but a fetch is in
progress"? And the UI will use it to show a loading indicator? Alright, makes
sense I suppose :)

[dgn, 2016/08/01 10:54:40]

Yes, that's how I understand it and the doc of the enum. 

+
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             kMaxSnippetCount, force_request);
 }
@@ -416,6 +420,8 @@ void NTPSnippetsService::OnDatabaseError() {
   EnterState(State::SHUT_DOWN, ContentSuggestionsCategoryStatus::LOADING_ERROR);
 }
 
+// TODO(dgn): name clash between content suggestions and suggestions hosts.
+// method name should be changed.
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
   DCHECK(initialized());
@@ -439,6 +445,9 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
   StoreSnippetHostsToPrefs(hosts);
 
+  // We removed some suggestions, so we want to let the client know about that.
+  // The fetch might take a long time or not complete so we don't want to wait
+  // for its callback.
   NotifyNewSuggestions();
 
   FetchSnippetsFromHosts(hosts, /*force_request=*/false);
@@ -449,6 +458,10 @@ void NTPSnippetsService::OnFetchFinished(
   if (!ready())
     return;
 
+  DCHECK(category_status_ == ContentSuggestionsCategoryStatus::AVAILABLE ||

[Philipp Keck, 2016/08/01 09:14:10]

What if we shut down during a fetch? Or if the user signs out of Chrome during a
fetch? Will the fetch be canceled, or can this callback still be fired?
Otherwise, this DCHECK would fail. But if that's a possible case, we should
instead just return (i.e. ignore the fetch results because during the fetch we
decided that we don't want it anymore).

[Marc Treib, 2016/08/01 09:34:34]

I thought about that too :)  But there's a ready() check above, so if any of
those things happened, we won't get here.

+         category_status_ ==
+             ContentSuggestionsCategoryStatus::AVAILABLE_LOADING);
+
   if (snippets) {
     // Sparse histogram used because the number of snippets is small (bound by
     // kMaxSnippetCount).
@@ -476,6 +489,7 @@ void NTPSnippetsService::OnFetchFinished(
                          dismissed_snippets_.size());
   }
 
+  UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE);
   NotifyNewSuggestions();
 }
 
@@ -658,6 +672,11 @@ void NTPSnippetsService::EnterStateEnabled(bool fetch_snippets) {
   if (fetch_snippets)
     FetchSnippets(/*force_request=*/false);
 
+  // FetchSnippets should set the status to |AVAILABLE_LOADING| if relevant,
+  // otherwise we transition to |AVAILABLE| here.
+  if (category_status_ != ContentSuggestionsCategoryStatus::AVAILABLE_LOADING)
+    UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE);
+
   // If host restrictions are enabled, register for host list updates.
   // |suggestions_service_| can be null in tests.
   if (snippets_fetcher_->UsesHostRestrictions() && suggestions_service_) {
@@ -705,6 +724,8 @@ void NTPSnippetsService::FinishInitialization() {
   snippets_status_service_->Init(base::Bind(
       &NTPSnippetsService::OnDisabledReasonChanged, base::Unretained(this)));
 
+  // Always notify here even if we got nothing from the database, because we
+  // don't know how long the fetch will take or if it will even complete.
   NotifyNewSuggestions();
 }
 
@@ -715,7 +736,8 @@ void NTPSnippetsService::OnDisabledReasonChanged(
 
   switch (disabled_reason) {
     case DisabledReason::NONE:
-      EnterState(State::READY, ContentSuggestionsCategoryStatus::AVAILABLE);
+      // Do not change the status. That will be done in EnterStateEnabled()
+      EnterState(State::READY, category_status_);
       break;
 
     case DisabledReason::EXPLICITLY_DISABLED:
@@ -732,10 +754,7 @@ void NTPSnippetsService::OnDisabledReasonChanged(
 
 void NTPSnippetsService::EnterState(State state,
                                     ContentSuggestionsCategoryStatus status) {
-  if (status != category_status_) {
-    category_status_ = status;
-    NotifyCategoryStatusChanged();
-  }
+  UpdateCategoryStatus(status);
 
   if (state == state_)
     return;
@@ -759,7 +778,6 @@ void NTPSnippetsService::EnterState(State state,
 
     case State::DISABLED:
       DCHECK(state_ == State::NOT_INITED || state_ == State::READY);
-
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       EnterStateDisabled();
@@ -800,7 +818,12 @@ void NTPSnippetsService::NotifyNewSuggestions() {
                               std::move(result));
 }
 
-void NTPSnippetsService::NotifyCategoryStatusChanged() {
+void NTPSnippetsService::UpdateCategoryStatus(
+    ContentSuggestionsCategoryStatus status) {
+  if (status == category_status_)
+    return;
+
+  category_status_ = status;
   if (observer_) {
     observer_->OnCategoryStatusChanged(ContentSuggestionsCategory::ARTICLES,
                                        category_status_);