📰 Persist category dismissals

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index f52a9809d09dcae42dbad4cd9a62ff9598870423..1229e4a2bf1bb04d8f054c8c59ae571383e2347e 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -13,6 +13,9 @@
 #include "base/location.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/values.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "ui/gfx/image/image.h"
 
 namespace ntp_snippets {
@@ -24,10 +27,13 @@ ContentSuggestionsService::ContentSuggestionsService(
     : state_(state),
       history_service_observer_(this),
       ntp_snippets_service_(nullptr),
+      pref_service_(pref_service),
       user_classifier_(pref_service) {
   // Can be null in tests.
   if (history_service)
     history_service_observer_.Add(history_service);
+
+  RestoreDismissedCategoriesFromPrefs();
 }
 
 ContentSuggestionsService::~ContentSuggestionsService() = default;
@@ -42,11 +48,19 @@ void ContentSuggestionsService::Shutdown() {
   FOR_EACH_OBSERVER(Observer, observers_, ContentSuggestionsServiceShutdown());
 }
 
+// static
+void ContentSuggestionsService::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterListPref(prefs::kDismissedCategories);
+}
+
 CategoryStatus ContentSuggestionsService::GetCategoryStatus(
     Category category) const {
-  if (state_ == State::DISABLED) {
+  if (state_ == State::DISABLED)
     return CategoryStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
-  }
+
+  if (IsCategoryDismissed(category))
+    return CategoryStatus::NOT_PROVIDED;
 
   auto iterator = providers_by_category_.find(category);
   if (iterator == providers_by_category_.end())
@@ -57,6 +71,9 @@ CategoryStatus ContentSuggestionsService::GetCategoryStatus(
 
 base::Optional<CategoryInfo> ContentSuggestionsService::GetCategoryInfo(
     Category category) const {
+  if (IsCategoryDismissed(category))
+    return base::Optional<CategoryInfo>();
+
   auto iterator = providers_by_category_.find(category);
   if (iterator == providers_by_category_.end())
     return base::Optional<CategoryInfo>();
@@ -65,6 +82,9 @@ base::Optional<CategoryInfo> ContentSuggestionsService::GetCategoryInfo(
 
 const std::vector<ContentSuggestion>&
 ContentSuggestionsService::GetSuggestionsForCategory(Category category) const {
+  if (IsCategoryDismissed(category))
+    return no_suggestions_;
+
   auto iterator = suggestions_by_category_.find(category);
   if (iterator == suggestions_by_category_.end())
     return no_suggestions_;
@@ -150,9 +170,8 @@ void ContentSuggestionsService::DismissCategory(Category category) {
   if (providers_it == providers_by_category_.end())
     return;
 
-  providers_by_category_.erase(providers_it);
-  categories_.erase(
-      std::find(categories_.begin(), categories_.end(), category));
+  dismissed_categories_.insert(category);
+  StoreDismissedCategoriesToPrefs();
 }
 
 void ContentSuggestionsService::AddObserver(Observer* observer) {
@@ -176,20 +195,57 @@ void ContentSuggestionsService::OnNewSuggestions(
     ContentSuggestionsProvider* provider,
     Category category,
     std::vector<ContentSuggestion> suggestions) {
-  if (RegisterCategoryIfRequired(provider, category))
-    NotifyCategoryStatusChanged(category);
+  if (!suggestions.empty()) {
+    dismissed_categories_.erase(category);
+    StoreDismissedCategoriesToPrefs();
+  }
 
-  if (!IsCategoryStatusAvailable(provider->GetCategoryStatus(category)))
-    return;
+  OnNewSuggestionsInternal(provider, category, std::move(suggestions));
+}
 
-  suggestions_by_category_[category] = std::move(suggestions);
+void ContentSuggestionsService::OnNewSuggestionBatch(
+    ContentSuggestionsProvider* provider,
+    SuggestionBatch suggestion_batch) {
+  // Update the dismissed suggestions. Remote suggestions are received at once
+  // from the server. Because of that, we always have the full list of valid
+  // remote categories according to the server. We remove invalid categories or
+  // the ones for which we got new suggestions to show.
+  for (auto it = dismissed_categories_.begin();
+       it != dismissed_categories_.end();) {
+    // Do nothing to the local sections, we only clean up the remote ones here.
+    if (!it->IsRemote()) {
+      ++it;
+      continue;
+    }
 
-  // The positioning of the bookmarks category depends on whether it's empty.
-  // TODO(treib): Remove this temporary hack, crbug.com/640568.
-  if (category.IsKnownCategory(KnownCategories::BOOKMARKS))
-    SortCategories();
+    auto entry = suggestion_batch.find(*it);
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnNewSuggestions(category));
+    // If the category is not included in the new bundle, it's not valid anymore
+    // and we can just remove it.
+    // TODO(dgn) Also remove all the related local data?
+    if (entry == suggestion_batch.end()) {
+      DVLOG(1) << "Dismissed category " << it->id()
+               << " unknown on the server.";
+      it = dismissed_categories_.erase(it);
+      continue;
+    }
+
+    // The category has new suggestions to show, it's not dismissed anymore.
+    if (!entry->second.empty()) {
+      DVLOG(1) << "Dismissed category " << it->id()
+               << " has new suggestions to show.";
+      it = dismissed_categories_.erase(it);
+      continue;
+    }
+
+    ++it;
+  }
+
+  StoreDismissedCategoriesToPrefs();
+
+  for (auto it = suggestion_batch.begin(); it != suggestion_batch.end(); ++it) {
+    OnNewSuggestionsInternal(provider, it->first, std::move(it->second));
+  }
 }
 
 void ContentSuggestionsService::OnCategoryStatusChanged(
@@ -200,10 +256,12 @@ void ContentSuggestionsService::OnCategoryStatusChanged(
     suggestions_by_category_.erase(category);
   }
   if (new_status == CategoryStatus::NOT_PROVIDED) {
-    DCHECK(providers_by_category_.find(category) !=
-           providers_by_category_.end());
-    DCHECK_EQ(provider, providers_by_category_.find(category)->second);
-    DismissCategory(category);
+    auto providers_it = providers_by_category_.find(category);
+    DCHECK(providers_it != providers_by_category_.end());
+    DCHECK_EQ(provider, providers_it->second);
+    providers_by_category_.erase(providers_it);
+    categories_.erase(
+        std::find(categories_.begin(), categories_.end(), category));
   } else {
     RegisterCategoryIfRequired(provider, category);
     DCHECK_EQ(new_status, provider->GetCategoryStatus(category));
@@ -284,6 +342,30 @@ bool ContentSuggestionsService::RegisterCategoryIfRequired(
   return true;
 }
 
+void ContentSuggestionsService::OnNewSuggestionsInternal(
+    ContentSuggestionsProvider* provider,
+    Category category,
+    std::vector<ContentSuggestion> suggestions) {
+  // Ignore notifications concerning dismissed categories.
+  if (IsCategoryDismissed(category))
+    return;
+
+  if (RegisterCategoryIfRequired(provider, category))
+    NotifyCategoryStatusChanged(category);
+
+  if (!IsCategoryStatusAvailable(provider->GetCategoryStatus(category)))
+    return;
+
+  suggestions_by_category_[category] = std::move(suggestions);
+
+  // The positioning of the bookmarks category depends on whether it's empty.
+  // TODO(treib): Remove this temporary hack, crbug.com/640568.
+  if (category.IsKnownCategory(KnownCategories::BOOKMARKS))
+    SortCategories();
+
+  FOR_EACH_OBSERVER(Observer, observers_, OnNewSuggestions(category));
+}
+
 bool ContentSuggestionsService::RemoveSuggestionByID(
     const ContentSuggestion::ID& suggestion_id) {
   std::vector<ContentSuggestion>* suggestions =
@@ -306,11 +388,19 @@ bool ContentSuggestionsService::RemoveSuggestionByID(
 }
 
 void ContentSuggestionsService::NotifyCategoryStatusChanged(Category category) {
+  // Ignore notifications concerning dismissed categories.
+  if (IsCategoryDismissed(category))

[Marc Treib, 2016/10/11 07:52:55]

I thought changing the status should make the dismissal go away?

[dgn, 2016/10/11 19:14:01]

It currently works just by the fact that when status changes or we received new
suggestions, we run RegisterCategoryIfNecessary, which will make the category
reappear. With this patch we have a more explicit way of controlling it.

But I'll wait for Michael's patch to land, since it does mostly the same as this
already.

+    return;
+
   FOR_EACH_OBSERVER(
       Observer, observers_,
       OnCategoryStatusChanged(category, GetCategoryStatus(category)));
 }
 
+bool ContentSuggestionsService::IsCategoryDismissed(Category category) const {
+  return dismissed_categories_.find(category) != dismissed_categories_.end();
+}
+
 void ContentSuggestionsService::SortCategories() {
   auto it = suggestions_by_category_.find(
       category_factory_.FromKnownCategory(KnownCategories::BOOKMARKS));
@@ -331,4 +421,30 @@ void ContentSuggestionsService::SortCategories() {
       });
 }
 
+void ContentSuggestionsService::RestoreDismissedCategoriesFromPrefs() {
+  // This must only be called at startup.
+  DCHECK(dismissed_categories_.empty());
+
+  const base::ListValue* list =
+      pref_service_->GetList(prefs::kDismissedCategories);
+  for (const std::unique_ptr<base::Value>& entry : *list) {
+    int id = 0;
+    if (!entry->GetAsInteger(&id)) {
+      DLOG(WARNING) << "Invalid category pref value: " << *entry;
+      continue;
+    }
+
+    dismissed_categories_.insert(category_factory()->FromIDValue(id));

[Marc Treib, 2016/10/11 07:52:55]

This might create the category, and the category order is atm defined by order
of creation. That's probably not what we want...

[dgn, 2016/10/11 19:14:01]

To be addressed by registering the articles at the same time as the local
categories.

+  }
+}
+
+void ContentSuggestionsService::StoreDismissedCategoriesToPrefs() {
+  base::ListValue list;
+  for (Category category : dismissed_categories_) {
+    list.AppendInteger(category.id());
+  }
+
+  pref_service_->Set(prefs::kDismissedCategories, list);
+}
+
 }  // namespace ntp_snippets