📰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;
+  LOG(INFO) << "DGN - Initiating fetch";
+  EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE_LOADING);

[Marc Treib, 2016/07/29 10:19:47]

I don't think this is right. AVAILABLE_LOADING is meant to be the initial state,
when the service is active but the very first fetch hasn't completed yet. We
shouldn't go back into this state while a background fetch is running.

[dgn, 2016/07/29 10:42:48]

Ah ok I didn't understand AVAILABLE_LOADING properly. So for the snippets
service, it would apply only when we just initialized, db has no snippets and we
started a fetch?

[Marc Treib, 2016/07/29 10:45:46]

Also while we're waiting for results from the DB (which is also async).

[dgn, 2016/07/29 16:37:46]

What about when the service was disabled, so we cleared the snippets. Shouldn't
we go DISABLED -> AVAILABLE_LOADING -> AVAILABLE ?

Also, INITIALIZING is the initial state, from what I can see. WDYT about the new
proposal?

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

Yes, that sounds right.
SGTM

   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);
 
+  // TODO(dgn) Why do we do that BEFORE fetching?
   NotifyNewSuggestions();

[dgn, 2016/07/29 09:58:36]

Why do we do that BEFORE fetching?

[Marc Treib, 2016/07/29 10:19:47]

We potentially deleted some snippets above, so we should arguably tell observers
that something changed.
We should at least check if we actually deleted anything though.
Probably we should also only notify if we're already AVAILABLE.

[dgn, 2016/07/29 10:42:48]

Wouldn't they be notified of the new list when the fetch finishes anyway?

[Marc Treib, 2016/07/29 10:45:46]

True, but that might take any amount of time, and might not ever complete
successfully.

[dgn, 2016/07/29 16:37:46]

Added a comment about this.

 
   FetchSnippetsFromHosts(hosts, /*force_request=*/false);
 }
 
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
   if (!ready())
     return;
 
   if (snippets) {
@@ skipping 245 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)));
 
+  // TODO(dgn): If we started a fetch as a result of the state being updated
+  // and not having snippets from the DB, because of the call below we would
+  // still notify the client with no snippets and force the "AVAILABLE" status.
+  // WAI?
+  // IMO that seems wrong. Skipping this call will put us either:
+  //  - in AVAILABLE_LOADING: because of the pending fetch started above.
+  //  - in INITIALIZING status: If it's right on startup and because we don't
+  //    have sync data, the fetch can't start. We appropriately report that we
+  //    are still initializing.
   NotifyNewSuggestions();

[dgn, 2016/07/29 09:58:36]

If we started a fetch as a result of the state being updated
and not having snippets from the DB, because of the call below we would
still notify the client with no snippets and force the "AVAILABLE" status.
WAI?
IMO that seems wrong. Skipping this call will put us either:
 - in AVAILABLE_LOADING: because of the pending fetch started above.
 - in INITIALIZING status: If it's right on startup and because we don't
   have sync data, the fetch can't start. We appropriately report that we
   are still initializing.

[Marc Treib, 2016/07/29 10:19:47]

I guess this is here because we "probably" loaded some snippets from the DB. It
seems we should check that here, and then either go into AVAILABLE and notify,
or go into AVAILABLE_LOADING and start a fetch.

[dgn, 2016/07/29 10:42:48]

So the intent is that while a fetch is pending and we have 0 suggestions
locally, we are in AVAILABLE_LOADING, but if we have suggestions or no fetch is
pending, we are in AVAILABLE?

[Marc Treib, 2016/07/29 10:45:46]

I think so, yes. We should also be in _LOADING if we're waiting for history sync
state and the like (if that still exists...)

Philipp, please chime in :)

 }
 
 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);
+    case DisabledReason::NONE: {
+      // Do not change the status. As the service becomes enabled, it will
+      // either start a new fetch (and transition to |AVAILABLE_LOADING|), or
+      // send cached suggestions (and transition to |AVAILABLE|).
+      EnterState(State::READY, category_status_);
       break;
+    }
 
     case DisabledReason::HISTORY_SYNC_STATE_UNKNOWN:
       // HistorySync is not initialized yet, so we don't know what the actual
       // 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.";
       EnterState(state_, ContentSuggestionsCategoryStatus::INITIALIZING);
       break;
 
@@ skipping 20 matching lines @@
 
     case DisabledReason::HISTORY_SYNC_DISABLED:
       EnterState(State::DISABLED,
                  ContentSuggestionsCategoryStatus::HISTORY_SYNC_DISABLED);
       break;
   }
 }
 
 void NTPSnippetsService::EnterState(State state,
                                     ContentSuggestionsCategoryStatus status) {
+  LOG(INFO) << "DGN - EnterState(" << (int)state << ", " << (int)status << ")";
+
   if (status != category_status_) {
     category_status_ = status;
     NotifyCategoryStatusChanged();
   }
 
   if (state == state_)
     return;
 
   switch (state) {
     case State::NOT_INITED:
@@ skipping 22 matching lines @@
 
     case State::SHUT_DOWN:
       DVLOG(1) << "Entering state: SHUT_DOWN";
       state_ = State::SHUT_DOWN;
       EnterStateShutdown();
       return;
   }
 }
 
 void NTPSnippetsService::NotifyNewSuggestions() {
+  EnterState(state_, ContentSuggestionsCategoryStatus::AVAILABLE);

[Marc Treib, 2016/07/29 10:19:47]

Huh, seems like we should already be in this state once we call this? Or
possibly enter it immediately afterwards I guess. In any case, changing state
here seems wrong...

+
   // TODO(pke): Remove this as soon as this becomes a pure provider.
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
   if (!observer_)
     return;
 
   std::vector<ContentSuggestion> result;
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets_) {
     if (!snippet->is_complete())
       continue;
     ContentSuggestion suggestion(
         MakeUniqueID(ContentSuggestionsCategory::ARTICLES, snippet->id()),
         snippet->best_source().url);
     suggestion.set_amp_url(snippet->best_source().amp_url);
     suggestion.set_title(snippet->title());
     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() {
+  LOG(INFO) << "DGN - NotifyCategoryStatusChanged " << (int)category_status_;
   if (observer_) {
     observer_->OnCategoryStatusChanged(ContentSuggestionsCategory::ARTICLES,
                                        category_status_);
   }
 }
 
 }  // namespace ntp_snippets