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

components/ntp_snippets/remote/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/remote/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 362 matching lines @@
   if (!base::ContainsKey(categories_, category))
     return;
   CategoryContent* content = &categories_[category];
   if (content->snippets.empty())
     return;
 
   database_->DeleteSnippets(GetSnippetIDVector(content->snippets));
   database_->DeleteImages(GetSnippetIDVector(content->snippets));
   content->snippets.clear();
 
-  NotifyNewSuggestions();
+  NotifyNewSuggestions(category);
 }
 
 void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   DCHECK(base::ContainsKey(categories_, category));
 
   std::vector<ContentSuggestion> result;
   const CategoryContent& content = categories_[category];
   for (const std::unique_ptr<NTPSnippet>& snippet : content.dismissed) {
@@ skipping 180 matching lines @@
     }
   }
 
   // We might have gotten new categories (or updated the titles of existing
   // ones), so update the pref.
   StoreCategoriesToPrefs();
 
   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.
   const CategoryContent& content = categories_[articles_category_];
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               content.snippets.size());
   if (content.snippets.empty() && !content.dismissed.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          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
   // a user-initiated one.
   if (fetched_categories)
     RescheduleFetching(true);
 }
 
 void NTPSnippetsService::ArchiveSnippets(Category category,
                                          NTPSnippet::PtrVector* to_archive) {
@@ skipping 127 matching lines @@
 }
 
 void NTPSnippetsService::NukeAllSnippets() {
   std::vector<Category> categories_to_erase;
 
   // Empty the ARTICLES category and remove all others, since they may or may
   // not be personalized.
   for (const auto& item : categories_) {
     Category category = item.first;
 
     ClearCachedSuggestions(category);

[Marc Treib, 2016/10/20 13:04:58]

The bug was triggered from here: ClearCachedSuggestions would call
NotifyNewSuggestions, which would call
ContentSuggestionsService::OnNewSuggestions for all categories, even those that
had already been marked NOT_PROVIDED in a previous iteration. And then
ContentSuggestionsService::OnNewSuggestions would add the category again.

     ClearDismissedSuggestionsForDebugging(category);
 
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
 
     // Remove the category entirely; it may or may not reappear.
     if (category != articles_category_)
       categories_to_erase.push_back(category);
   }
 
   for (Category category : categories_to_erase) {
@@ skipping 122 matching lines @@
         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::OnSnippetsStatusChanged, 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();
+  for (const auto& item : categories_) {
+    Category category = item.first;
+    NotifyNewSuggestions(category);
+  }
 }
 
 void NTPSnippetsService::OnSnippetsStatusChanged(
     SnippetsStatus old_snippets_status,
     SnippetsStatus new_snippets_status) {
   switch (new_snippets_status) {
     case SnippetsStatus::ENABLED_AND_SIGNED_IN:
       if (old_snippets_status == SnippetsStatus::ENABLED_AND_SIGNED_OUT) {
         DCHECK(state_ == State::READY);
         // Clear nonpersonalized suggestions.
@@ skipping 61 matching lines @@
       DVLOG(1) << "Entering state: ERROR_OCCURRED";
       state_ = State::ERROR_OCCURRED;
       EnterStateError();
       break;
   }
 
   // Schedule or un-schedule background fetching after each state change.
   RescheduleFetching(false);
 }
 
-void NTPSnippetsService::NotifyNewSuggestions() {
-  for (const auto& item : categories_) {
-    Category category = item.first;
-    const CategoryContent& content = item.second;
+void NTPSnippetsService::NotifyNewSuggestions(Category category) {
+  DCHECK(base::ContainsKey(categories_, category));
+  const CategoryContent& content = categories_[category];
+  DCHECK(IsCategoryStatusAvailable(content.status));
 
-    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));
-    }
+  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(): " << result.size()
-             << " items in category " << category;
-    observer()->OnNewSuggestions(this, category, std::move(result));
-  }
+  DVLOG(1) << "NotifyNewSuggestions(" << category << "): " << result.size()
+           << " items.";
+  observer()->OnNewSuggestions(this, category, std::move(result));
 }
 
 void NTPSnippetsService::UpdateCategoryStatus(Category category,
                                               CategoryStatus status) {
   DCHECK(base::ContainsKey(categories_, category));
   CategoryContent& content = categories_[category];
   if (status == content.status)
     return;
 
   DVLOG(1) << "UpdateCategoryStatus(): " << category.id() << ": "
@@ skipping 99 matching lines @@
 }
 
 NTPSnippetsService::CategoryContent::CategoryContent() = default;
 NTPSnippetsService::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 NTPSnippetsService::CategoryContent::~CategoryContent() = default;
 NTPSnippetsService::CategoryContent& NTPSnippetsService::CategoryContent::
 operator=(CategoryContent&&) = default;
 
 }  // namespace ntp_snippets