📰Fix SnippetsService's status reporting

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 241 matching lines @@
     FetchSnippetsFromHosts(GetSuggestionsHosts(), force_request);
   else
     fetch_after_load_ = true;
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     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);
 }
 
 void NTPSnippetsService::RescheduleFetching() {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
   if (ready()) {
@@ skipping 137 matching lines @@
             });
 
   ClearExpiredSnippets();
   FinishInitialization();
 }
 
 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());
 
   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_->DeleteSnippets(to_delete);
 
   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);
 }
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
   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).
     DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
 
   ClearExpiredSnippets();
 
   // If there are more snippets than we want to show, delete the extra ones.
   if (snippets_.size() > kMaxSnippetCount) {
     NTPSnippet::PtrVector to_delete(
         std::make_move_iterator(snippets_.begin() + kMaxSnippetCount),
         std::make_move_iterator(snippets_.end()));
     snippets_.resize(kMaxSnippetCount);
     database_->DeleteSnippets(to_delete);
   }
 
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               snippets_.size());
   if (snippets_.empty() && !dismissed_snippets_.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          dismissed_snippets_.size());
   }
 
+  UpdateCategoryStatus(ContentSuggestionsCategoryStatus::AVAILABLE);
   NotifyNewSuggestions();
 }
 
 void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
   DCHECK(ready());
 
   // Remove new snippets that we already have, or that have been dismissed.
   std::set<std::string> old_snippet_ids;
   InsertAllIDs(dismissed_snippets_, &old_snippet_ids);
   InsertAllIDs(snippets_, &old_snippet_ids);
@@ skipping 162 matching lines @@
 
   const NTPSnippet& snippet = *it->get();
   image_fetcher_->StartOrQueueNetworkRequest(
       snippet.id(), snippet.salient_image_url(), callback);
 }
 
 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_) {
     suggestions_service_subscription_ =
         suggestions_service_->AddCallback(base::Bind(
             &NTPSnippetsService::OnSuggestionsChanged, base::Unretained(this)));
   }
 
   RescheduleFetching();
 }
@@ skipping 27 matching lines @@
     image_fetcher_->SetImageFetcherDelegate(this);
     image_fetcher_->SetDataUseServiceName(
         data_use_measurement::DataUseUserData::NTP_SNIPPETS);
   }
 
   // Note: Initializing the status service will run the callback right away with
   // the current state.
   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();
 }
 
 void NTPSnippetsService::OnDisabledReasonChanged(
     DisabledReason disabled_reason) {
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceDisabledReasonChanged(disabled_reason));
 
   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:
       EnterState(
           State::DISABLED,
           ContentSuggestionsCategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
       break;
 
     case DisabledReason::SIGNED_OUT:
       EnterState(State::DISABLED, ContentSuggestionsCategoryStatus::SIGNED_OUT);
       break;
   }
 }
 
 void NTPSnippetsService::EnterState(State state,
                                     ContentSuggestionsCategoryStatus status) {
-  if (status != category_status_) {
-    category_status_ = status;
-    NotifyCategoryStatusChanged();
-  }
+  UpdateCategoryStatus(status);
 
   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::DISABLED);
 
       bool fetch_snippets = snippets_.empty() || 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);
-
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       EnterStateDisabled();
       return;
 
     case State::SHUT_DOWN:
       DVLOG(1) << "Entering state: SHUT_DOWN";
       state_ = State::SHUT_DOWN;
       EnterStateShutdown();
       return;
@@ skipping 20 matching lines @@
     suggestion.set_snippet_text(snippet->snippet());
     suggestion.set_publish_date(snippet->publish_date());
     suggestion.set_publisher_name(snippet->best_source().publisher_name);
     suggestion.set_score(snippet->score());
     result.emplace_back(std::move(suggestion));
   }
   observer_->OnNewSuggestions(ContentSuggestionsCategory::ARTICLES,
                               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_);
   }
 }
 
 }  // namespace ntp_snippets