[NTP Snippets] NTPSnippetsService: notify about new suggestions only in changed categories

components/ntp_snippets/remote/ntp_snippets_service.cc

Index: components/ntp_snippets/remote/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_service.cc b/components/ntp_snippets/remote/ntp_snippets_service.cc
index 6767c607ce0ad903bed82866138ef8ee74635752..2c26c17c5ecce2522cdc148ecad17b3f6b8b06df 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -380,7 +380,7 @@ void NTPSnippetsService::ClearCachedSuggestions(Category category) {
   database_->DeleteImages(GetSnippetIDVector(content->snippets));
   content->snippets.clear();
 
-  NotifyNewSuggestions();
+  NotifyNewSuggestions(category);
 }
 
 void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
@@ -581,6 +581,8 @@ void NTPSnippetsService::OnFetchFinished(
   for (const auto& item : categories_) {
     Category category = item.first;
     UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
+    // TODO(sfiera): notify only when a category changed above.

[dgn, 2016/10/20 14:07:33]

todo not needed anymore?

[Marc Treib, 2016/10/20 14:29:50]

I think it's still needed: We still notify about all categories, even if not all
of them were changed above.
Fixing this might be as simple as adding an "if (content->provided_by_server)",
but I didn't want to do that in the same CL.

+    NotifyNewSuggestions(category);
   }
 
   // TODO(sfiera): equivalent metrics for non-articles.
@@ -592,9 +594,6 @@ void NTPSnippetsService::OnFetchFinished(
                          content.dismissed.size());
   }
 
-  // TODO(sfiera): notify only when a category changed above.
-  NotifyNewSuggestions();
-
   // Reschedule after a successful fetch. This resets all currently scheduled
   // fetches, to make sure the fallback interval triggers only if no wifi fetch
   // succeeded, and also that we don't do a background fetch immediately after
@@ -885,7 +884,10 @@ void NTPSnippetsService::FinishInitialization() {
 
   // 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();
+  for (const auto& item : categories_) {
+    Category category = item.first;
+    NotifyNewSuggestions(category);
+  }
 }
 
 void NTPSnippetsService::OnSnippetsStatusChanged(
@@ -967,35 +969,34 @@ void NTPSnippetsService::EnterState(State state) {
   RescheduleFetching(false);
 }
 
-void NTPSnippetsService::NotifyNewSuggestions() {
-  for (const auto& item : categories_) {
-    Category category = item.first;
-    const CategoryContent& content = item.second;
-
-    std::vector<ContentSuggestion> result;
-    for (const std::unique_ptr<NTPSnippet>& snippet : content.snippets) {
-      // TODO(sfiera): if a snippet is not going to be displayed, move it
-      // directly to content.dismissed on fetch. Otherwise, we might prune
-      // other snippets to get down to kMaxSnippetCount, only to hide one of the
-      // incomplete ones we kept.
-      if (!snippet->is_complete())
-        continue;
-      ContentSuggestion suggestion(category, snippet->id(),
-                                   snippet->best_source().url);
-      suggestion.set_amp_url(snippet->best_source().amp_url);
-      suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
-      suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
-      suggestion.set_publish_date(snippet->publish_date());
-      suggestion.set_publisher_name(
-          base::UTF8ToUTF16(snippet->best_source().publisher_name));
-      suggestion.set_score(snippet->score());
-      result.emplace_back(std::move(suggestion));
-    }
+void NTPSnippetsService::NotifyNewSuggestions(Category category) {
+  DCHECK(base::ContainsKey(categories_, category));
+  const CategoryContent& content = categories_[category];
+  DCHECK(IsCategoryStatusAvailable(content.status));
 
-    DVLOG(1) << "NotifyNewSuggestions(): " << result.size()
-             << " items in category " << category;
-    observer()->OnNewSuggestions(this, category, std::move(result));
+  std::vector<ContentSuggestion> result;
+  for (const std::unique_ptr<NTPSnippet>& snippet : content.snippets) {
+    // TODO(sfiera): if a snippet is not going to be displayed, move it
+    // directly to content.dismissed on fetch. Otherwise, we might prune
+    // other snippets to get down to kMaxSnippetCount, only to hide one of the
+    // incomplete ones we kept.
+    if (!snippet->is_complete())
+      continue;
+    ContentSuggestion suggestion(category, snippet->id(),
+                                 snippet->best_source().url);
+    suggestion.set_amp_url(snippet->best_source().amp_url);
+    suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
+    suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
+    suggestion.set_publish_date(snippet->publish_date());
+    suggestion.set_publisher_name(
+        base::UTF8ToUTF16(snippet->best_source().publisher_name));
+    suggestion.set_score(snippet->score());
+    result.emplace_back(std::move(suggestion));
   }
+
+  DVLOG(1) << "NotifyNewSuggestions(" << category << "): " << result.size()
+           << " items.";
+  observer()->OnNewSuggestions(this, category, std::move(result));
 }
 
 void NTPSnippetsService::UpdateCategoryStatus(Category category,