📰 Persist category dismissals

components/ntp_snippets/content_suggestions_service.cc

 // Copyright 2016 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/content_suggestions_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #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 {
 
 ContentSuggestionsService::ContentSuggestionsService(
     State state,
     history::HistoryService* history_service,
     PrefService* pref_service)
     : 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;
 
 void ContentSuggestionsService::Shutdown() {
   ntp_snippets_service_ = nullptr;
   suggestions_by_category_.clear();
   providers_by_category_.clear();
   categories_.clear();
   providers_.clear();
   state_ = State::DISABLED;
   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) {
     return CategoryStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
   }
 
   auto iterator = providers_by_category_.find(category);
   if (iterator == providers_by_category_.end())
     return CategoryStatus::NOT_PROVIDED;
 
@@ skipping 88 matching lines @@
   DCHECK(removed) << "The dismissed suggestion " << suggestion_id
                   << " has already been removed. Providers must not call"
                   << " OnNewSuggestions in response to DismissSuggestion.";
 }
 
 void ContentSuggestionsService::DismissCategory(Category category) {
   auto providers_it = providers_by_category_.find(category);
   if (providers_it == providers_by_category_.end())
     return;
 
-  suggestions_by_category_[category].clear();
-  dismissed_providers_by_category_[providers_it->first] = providers_it->second;
-  providers_by_category_.erase(providers_it);
-  categories_.erase(
-      std::find(categories_.begin(), categories_.end(), category));
+  suggestions_by_category_.erase(category);

[dgn, 2016/10/13 14:46:51]

I'm not sure why we just cleared the entry before, since the rest of the time we
erase it (e.g. when category not available.). Did I miss something?

[Marc Treib, 2016/10/13 15:59:00]

No, I think I missed this in a previous CL - erase LG.
But should this happen inside UnregisterCategory?

[dgn, 2016/10/14 21:20:28]

It's not done currently, but it would make sense to do it there. Moving the
call.

+  ContentSuggestionsProvider* provider = providers_it->second;
+
+  UnregisterCategory(category, provider);
+
+  dismissed_providers_by_category_[category] = provider;
+  StoreDismissedCategoriesToPrefs();
 }
 
 void ContentSuggestionsService::RestoreDismissedCategories() {
   // Make a copy as the original will be modified during iteration.
   auto dismissed_providers_by_category_copy = dismissed_providers_by_category_;
   for (const auto& category_provider_pair :
        dismissed_providers_by_category_copy) {
-    RegisterCategoryIfRequired(category_provider_pair.second,
-                               category_provider_pair.first);
+    if (category_provider_pair.second) {
+      RestoreDismissedCategory(category_provider_pair.first);
+    } else {
+      // There is no provider for this category, we just remove the entry.
+      dismissed_providers_by_category_.erase(category_provider_pair.first);
+    }
   }
+  StoreDismissedCategoriesToPrefs();
   DCHECK(dismissed_providers_by_category_.empty());
 }
 
 void ContentSuggestionsService::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void ContentSuggestionsService::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 void ContentSuggestionsService::RegisterProvider(
     std::unique_ptr<ContentSuggestionsProvider> provider) {
   DCHECK(state_ == State::ENABLED);
   providers_.push_back(std::move(provider));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 void ContentSuggestionsService::OnNewSuggestions(
     ContentSuggestionsProvider* provider,
     Category category,
     std::vector<ContentSuggestion> suggestions) {
   if (RegisterCategoryIfRequired(provider, category))
     NotifyCategoryStatusChanged(category);
 
+  if (IsCategoryDismissed(category)) {

[Marc Treib, 2016/10/13 15:59:00]

else if?
If the category was just successfully registered, it can't be dismissed, right?

[dgn, 2016/10/14 21:20:28]

Done.

+    // The category has been registered as a dismissed one. We need to
+    // check if the dismissal can be cleared now that we received new data.
+    if (suggestions.empty())
+      return;
+
+    RestoreDismissedCategory(category);
+    StoreDismissedCategoriesToPrefs();
+
+    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));
 }
 
 void ContentSuggestionsService::OnCategoryStatusChanged(
     ContentSuggestionsProvider* provider,
     Category category,
     CategoryStatus new_status) {
   if (!IsCategoryStatusAvailable(new_status)) {
     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);
+    UnregisterCategory(category, provider);
   } else {
     RegisterCategoryIfRequired(provider, category);
     DCHECK_EQ(new_status, provider->GetCategoryStatus(category));
   }
-  NotifyCategoryStatusChanged(category);
+
+  if (!IsCategoryDismissed(category))
+    NotifyCategoryStatusChanged(category);
 }
 
 void ContentSuggestionsService::OnSuggestionInvalidated(
     ContentSuggestionsProvider* provider,
     const ContentSuggestion::ID& suggestion_id) {
   RemoveSuggestionByID(suggestion_id);
   FOR_EACH_OBSERVER(Observer, observers_,
                     OnSuggestionInvalidated(suggestion_id));
 }
 
@@ skipping 39 matching lines @@
 }
 
 void ContentSuggestionsService::HistoryServiceBeingDeleted(
     history::HistoryService* history_service) {
   history_service_observer_.RemoveAll();
 }
 
 bool ContentSuggestionsService::RegisterCategoryIfRequired(
     ContentSuggestionsProvider* provider,
     Category category) {
+  // Search for the category in both the regular and dismissed lists.
   auto it = providers_by_category_.find(category);

[Marc Treib, 2016/10/13 15:59:00]

Can we early-out here if |it != providers_by_category_.end()|?

[dgn, 2016/10/14 21:20:28]

Done, also rewrote the function since it removes branches below.

-  if (it != providers_by_category_.end()) {
-    DCHECK_EQ(it->second, provider);
-    return false;
+  auto dismissed_it = dismissed_providers_by_category_.find(category);
+
+  // We will need to register the provider for the category if it's absent from
+  // both. Note that the initialisation of dismissed categories registers them
+  // with |nullptr| for providers, we need to check for that too.
+  bool is_dismissed = dismissed_it != dismissed_providers_by_category_.end();
+  bool is_new = it == providers_by_category_.end() &&
+                (!is_dismissed || !dismissed_it->second);

[Marc Treib, 2016/10/13 15:59:00]

Maybe |category_is_dismissed| and |provider_is_new|? The two variables don't
really refer to the same thing.

[dgn, 2016/10/14 21:20:28]

Done.

+
+  if (is_new) {
+    if (is_dismissed) {
+      dismissed_it->second = provider;
+      return false;
+    }
+    RegisterCategoryInternal(category, provider);
+  } else {
+    DCHECK_EQ(is_dismissed ? dismissed_it->second : it->second, provider);
   }
 
-  auto dismissed_it = dismissed_providers_by_category_.find(category);
-  if (dismissed_it != dismissed_providers_by_category_.end()) {
-    DCHECK_EQ(dismissed_it->second, provider);
-    dismissed_providers_by_category_.erase(dismissed_it);
-  }
+  return is_new;
+}
+
+void ContentSuggestionsService::RegisterCategoryInternal(
+    Category category,
+    ContentSuggestionsProvider* provider) {
+  DCHECK(providers_by_category_.find(category) == providers_by_category_.end());

[Marc Treib, 2016/10/13 15:59:00]

optional nit: base::ContainsKey

[dgn, 2016/10/14 21:20:28]

Thanks! I was annoyed the STL doesn't provide anything like that and didn't
think about checking //base :D

+  DCHECK(!IsCategoryDismissed(category));
 
   providers_by_category_[category] = provider;
   categories_.push_back(category);
   SortCategories();
   if (IsCategoryStatusAvailable(provider->GetCategoryStatus(category))) {
     suggestions_by_category_.insert(
         std::make_pair(category, std::vector<ContentSuggestion>()));
   }
-  return true;
+}
+
+void ContentSuggestionsService::UnregisterCategory(
+    Category category,
+    ContentSuggestionsProvider* provider) {
+  if (IsCategoryDismissed(category))

[Marc Treib, 2016/10/13 15:59:00]

Why?

[dgn, 2016/10/14 21:20:28]

Because when OnCategoryStatusChanged() is called with a dismissed category, we
would hit a DCHECK. Rewrote that bit to make it more clear.

+    return;
+
+  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));
 }
 
 bool ContentSuggestionsService::RemoveSuggestionByID(
     const ContentSuggestion::ID& suggestion_id) {
   std::vector<ContentSuggestion>* suggestions =
       &suggestions_by_category_[suggestion_id.category()];
   auto position =
       std::find_if(suggestions->begin(), suggestions->end(),
                    [&suggestion_id](const ContentSuggestion& suggestion) {
                      return suggestion_id == suggestion.id();
@@ skipping 29 matching lines @@
         if (bookmarks_empty) {
           if (left.IsKnownCategory(KnownCategories::BOOKMARKS))
             return false;
           if (right.IsKnownCategory(KnownCategories::BOOKMARKS))
             return true;
         }
         return category_factory_.CompareCategories(left, right);
       });
 }
 
+bool ContentSuggestionsService::IsCategoryDismissed(Category category) const {
+  return dismissed_providers_by_category_.find(category) !=
+         dismissed_providers_by_category_.end();

[Marc Treib, 2016/10/13 15:59:00]

optional nit: base::ContainsKey

[dgn, 2016/10/14 21:20:28]

Done.

+}
+
+void ContentSuggestionsService::RestoreDismissedCategory(Category category) {
+  auto dismissed_it = dismissed_providers_by_category_.find(category);
+  DCHECK(dismissed_it != dismissed_providers_by_category_.end());
+
+  // Keep the reference to the provider and remove it from the dismissed ones,
+  // because the category registration enforces that it's not dismissed.
+  ContentSuggestionsProvider* provider = dismissed_it->second;

[Marc Treib, 2016/10/13 15:59:01]

Can provider be null here? If not, DCHECK that?

[dgn, 2016/10/14 21:20:28]

Good catch, it's possible, yes.

+  dismissed_providers_by_category_.erase(dismissed_it);
+
+  RegisterCategoryInternal(category, provider);
+}
+
+void ContentSuggestionsService::RestoreDismissedCategoriesFromPrefs() {
+  // This must only be called at startup.
+  DCHECK(dismissed_providers_by_category_.empty());
+  DCHECK(providers_by_category_.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;
+    }
+
+    // When the provider is registered, it will be moved to this map.

[Marc Treib, 2016/10/13 15:59:00]

nit: s/moved to/stored in/ ? "moved to" sounds like it was already around
somewhere.

[dgn, 2016/10/14 21:20:28]

Done.

+    dismissed_providers_by_category_[category_factory()->FromIDValue(id)] =
+        nullptr;
+  }
+}
+
+void ContentSuggestionsService::StoreDismissedCategoriesToPrefs() {
+  base::ListValue list;
+  for (const auto& category_provider_pair : dismissed_providers_by_category_) {
+    list.AppendInteger(category_provider_pair.first.id());
+  }
+
+  pref_service_->Set(prefs::kDismissedCategories, list);
+}
+
 }  // namespace ntp_snippets