📰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 a2cf72c8713c9943f2b19b92e1ee1fe5a8e6b008..ff447c08ba4c31e589d26876ba7cc0d2ccc7391e 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -259,6 +259,8 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
     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);
 }
@@ -416,6 +418,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 +443,7 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
   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);
@@ -705,6 +710,15 @@ void NTPSnippetsService::FinishInitialization() {
   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 :)

 }
 
@@ -714,9 +728,13 @@ void NTPSnippetsService::OnDisabledReasonChanged(
                     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
@@ -757,6 +775,8 @@ void NTPSnippetsService::OnDisabledReasonChanged(
 
 void NTPSnippetsService::EnterState(State state,
                                     ContentSuggestionsCategoryStatus status) {
+  LOG(INFO) << "DGN - EnterState(" << (int)state << ", " << (int)status << ")";
+
   if (status != category_status_) {
     category_status_ = status;
     NotifyCategoryStatusChanged();
@@ -799,6 +819,8 @@ void NTPSnippetsService::EnterState(State state,
 }
 
 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());
@@ -826,6 +848,7 @@ void NTPSnippetsService::NotifyNewSuggestions() {
 }
 
 void NTPSnippetsService::NotifyCategoryStatusChanged() {
+  LOG(INFO) << "DGN - NotifyCategoryStatusChanged " << (int)category_status_;
   if (observer_) {
     observer_->OnCategoryStatusChanged(ContentSuggestionsCategory::ARTICLES,
                                        category_status_);