[NTP::Downloads] Limit number of dismissed IDs instead of pruning.

chrome/browser/ntp_snippets/download_suggestions_provider.cc

Index: chrome/browser/ntp_snippets/download_suggestions_provider.cc
diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider.cc b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
index 7dfb81d6a5de40afa57ad19a561fe23753aaf929..e4dcfe539f2a5b5d3349fab793e212c6fdfadbcd 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.cc
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -17,6 +17,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/ntp_snippets/features.h"
@@ -49,6 +50,9 @@ const char kOfflinePageDownloadsPrefix = 'O';
 
 const char* kMaxSuggestionsCountParamName = "downloads_max_count";
 
+// Maximal number of dismissed asset download IDs stored at any time.
+const int kMaxDismissedIdNum = 100;

[Marc Treib, 2016/12/09 16:37:26]

nit: s/Num/Count/ (IMO that's much more common)

[vitaliii, 2016/12/09 17:30:36]

Done.

+
 int GetMaxSuggestionsCount() {
   bool assets_enabled =
       base::FeatureList::IsEnabled(features::kAssetDownloadSuggestionsFeature);
@@ -182,12 +186,8 @@ CategoryInfo DownloadSuggestionsProvider::GetCategoryInfo(Category category) {
 void DownloadSuggestionsProvider::DismissSuggestion(
     const ContentSuggestion::ID& suggestion_id) {
   DCHECK_EQ(provided_category_, suggestion_id.category());
-  std::set<std::string> dismissed_ids =
-      ReadDismissedIDsFromPrefs(CorrespondsToOfflinePage(suggestion_id));
-  dismissed_ids.insert(suggestion_id.id_within_category());
-  StoreDismissedIDsToPrefs(CorrespondsToOfflinePage(suggestion_id),
-                           dismissed_ids);
 
+  AddToDismissedStorageIfNeeded(suggestion_id);
   RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(suggestion_id);
 }
 
@@ -258,7 +258,7 @@ void DownloadSuggestionsProvider::GetDismissedSuggestionsForDebugging(
 void DownloadSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   DCHECK_EQ(provided_category_, category);
-  StoreAssetDismissedIDsToPrefs(std::set<std::string>());
+  StoreAssetDismissedIDsToPrefs(std::list<std::string>());
   StoreOfflinePageDismissedIDsToPrefs(std::set<std::string>());
   AsynchronouslyFetchAllDownloadsAndSubmitSuggestions();
 }
@@ -289,10 +289,12 @@ void DownloadSuggestionsProvider::
     std::vector<DownloadItem*> all_downloads;
     download_manager_->GetAllDownloads(&all_downloads);
 
-    dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+    std::list<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
 
     for (const DownloadItem* item : all_downloads) {
-      if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId()))) {
+      std::string id = GetAssetDownloadPerCategoryID(item->GetId());
+      if (std::find(dismissed_ids.begin(), dismissed_ids.end(), id) !=

[Marc Treib, 2016/12/09 16:37:26]

optional: base::ContainsValue (from stl_util.h) is IMO a bit more readable.

[vitaliii, 2016/12/09 17:30:36]

Done.
Thanks, it is definitely more readable. I was trying to find it, but did not
manage, which is funny, because it was already used in the same file.

+          dismissed_ids.end()) {

[Marc Treib, 2016/12/09 16:37:26]

Hm, so this is now O(n*m) instead of O((n+m)*log(n)) (n = number of dismissed, m
= number of downloads). I *guess* that's okay?

[vitaliii, 2016/12/09 17:30:36]

This is fine, because this method is used only in tests.
Everywhere else we never check multiple items (since we do not prune anymore).

         suggestions.push_back(ConvertDownloadItem(*item));
       }
     }
@@ -424,25 +426,24 @@ void DownloadSuggestionsProvider::FetchAssetsDownloads() {
 
   std::vector<DownloadItem*> all_downloads;
   download_manager_->GetAllDownloads(&all_downloads);
-  std::set<std::string> old_dismissed_ids = ReadAssetDismissedIDsFromPrefs();
-  std::set<std::string> retained_dismissed_ids;
+  std::list<std::string> old_dismissed_ids = ReadAssetDismissedIDsFromPrefs();
   cached_asset_downloads_.clear();
   for (const DownloadItem* item : all_downloads) {
     std::string within_category_id =
         GetAssetDownloadPerCategoryID(item->GetId());
-    if (!old_dismissed_ids.count(within_category_id)) {
+    if (std::find(old_dismissed_ids.begin(), old_dismissed_ids.end(),
+                  within_category_id) == old_dismissed_ids.end()) {
       if (IsDownloadCompleted(*item)) {
         cached_asset_downloads_.push_back(item);
       }
-    } else {
-      retained_dismissed_ids.insert(within_category_id);
     }
   }
 
-  if (old_dismissed_ids.size() != retained_dismissed_ids.size()) {
-    StoreAssetDismissedIDsToPrefs(retained_dismissed_ids);
-  }
-
+  // We do not prune dismissed IDs, because it is not possible to ensure that
+  // the list of downloads is complete (i.e. DownloadManager has finished
+  // reading them).
+  // TODO(vitaliii): Prune dismissed IDs once the |OnLoaded| notification is
+  // provided. See crbug.com/672758.
   const int max_suggestions_count = GetMaxSuggestionsCount();
   if (static_cast<int>(cached_asset_downloads_.size()) >
       max_suggestions_count) {
@@ -544,8 +545,10 @@ bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
     return false;
   }
 
-  std::set<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
-  if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId()))) {
+  std::list<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  std::string id = GetAssetDownloadPerCategoryID(item->GetId());
+  if (std::find(dismissed_ids.begin(), dismissed_ids.end(), id) !=
+      dismissed_ids.end()) {
     return false;
   }
 
@@ -683,31 +686,38 @@ void DownloadSuggestionsProvider::InvalidateSuggestion(
   ContentSuggestion::ID suggestion_id(provided_category_, id_within_category);
   observer()->OnSuggestionInvalidated(this, suggestion_id);
 
-  std::set<std::string> dismissed_ids =
-      ReadDismissedIDsFromPrefs(CorrespondsToOfflinePage(suggestion_id));
-  auto it = dismissed_ids.find(id_within_category);
-  if (it != dismissed_ids.end()) {
-    dismissed_ids.erase(it);
-    StoreDismissedIDsToPrefs(CorrespondsToOfflinePage(suggestion_id),
-                             dismissed_ids);
-  }
-
+  RemoveFromDismissedStorageIfNeeded(suggestion_id);
   RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(suggestion_id);
 }
 
-std::set<std::string>
+// TODO(vitaliii): Do not use std::list once ensure pruning at the right time.
+// See crbug.com/672758.
+std::list<std::string>
 DownloadSuggestionsProvider::ReadAssetDismissedIDsFromPrefs() const {
-  return ntp_snippets::prefs::ReadDismissedIDsFromPrefs(
-      *pref_service_, kDismissedAssetDownloadSuggestions);
+  std::list<std::string> dismissed_ids;
+  const base::ListValue* list =
+      pref_service_->GetList(kDismissedAssetDownloadSuggestions);
+  for (const std::unique_ptr<base::Value>& value : *list) {
+    std::string dismissed_id;
+    bool success = value->GetAsString(&dismissed_id);
+    DCHECK(success) << "Failed to parse dismissed id from prefs param "
+                    << kDismissedAssetDownloadSuggestions << " into string.";
+    dismissed_ids.push_back(dismissed_id);
+  }
+  return dismissed_ids;
 }
 
 void DownloadSuggestionsProvider::StoreAssetDismissedIDsToPrefs(
-    const std::set<std::string>& dismissed_ids) {
+    const std::list<std::string>& dismissed_ids) {
   DCHECK(std::all_of(
       dismissed_ids.begin(), dismissed_ids.end(),
       [](const std::string& id) { return id[0] == kAssetDownloadsPrefix; }));
-  ntp_snippets::prefs::StoreDismissedIDsToPrefs(
-      pref_service_, kDismissedAssetDownloadSuggestions, dismissed_ids);
+
+  base::ListValue list;
+  for (const std::string& dismissed_id : dismissed_ids) {
+    list.AppendString(dismissed_id);
+  }
+  pref_service_->Set(kDismissedAssetDownloadSuggestions, list);
 }
 
 std::set<std::string>
@@ -726,22 +736,46 @@ void DownloadSuggestionsProvider::StoreOfflinePageDismissedIDsToPrefs(
       pref_service_, kDismissedOfflinePageDownloadSuggestions, dismissed_ids);
 }
 
-std::set<std::string> DownloadSuggestionsProvider::ReadDismissedIDsFromPrefs(
-    bool for_offline_page_downloads) const {
-  if (for_offline_page_downloads) {
-    return ReadOfflinePageDismissedIDsFromPrefs();
+void DownloadSuggestionsProvider::AddToDismissedStorageIfNeeded(
+    const ContentSuggestion::ID& suggestion_id) {
+  if (CorrespondsToOfflinePage(suggestion_id)) {
+    std::set<std::string> dismissed_ids =
+        ReadOfflinePageDismissedIDsFromPrefs();
+    dismissed_ids.insert(suggestion_id.id_within_category());
+    StoreOfflinePageDismissedIDsToPrefs(dismissed_ids);
+  } else {
+    std::list<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+    // The suggestion may be double dismissed from previously opened NTPs.
+    if (std::find(dismissed_ids.begin(), dismissed_ids.end(),
+                  suggestion_id.id_within_category()) == dismissed_ids.end()) {
+      dismissed_ids.push_back(suggestion_id.id_within_category());
+      // TODO(vitaliii): Remove this workaround once the dismissed ids are
+      // pruned. See crbug.com/672758.
+      while (dismissed_ids.size() > kMaxDismissedIdNum) {
+        dismissed_ids.pop_front();
+      }
+      StoreAssetDismissedIDsToPrefs(dismissed_ids);
+    }
   }
-  return ReadAssetDismissedIDsFromPrefs();
 }
 
-// TODO(vitaliii): Store one set instead of two. See crbug.com/656024.
-void DownloadSuggestionsProvider::StoreDismissedIDsToPrefs(
-    bool for_offline_page_downloads,
-    const std::set<std::string>& dismissed_ids) {
-  if (for_offline_page_downloads) {
-    StoreOfflinePageDismissedIDsToPrefs(dismissed_ids);
+void DownloadSuggestionsProvider::RemoveFromDismissedStorageIfNeeded(
+    const ContentSuggestion::ID& suggestion_id) {
+  if (CorrespondsToOfflinePage(suggestion_id)) {
+    std::set<std::string> dismissed_ids =
+        ReadOfflinePageDismissedIDsFromPrefs();
+    if (dismissed_ids.count(suggestion_id.id_within_category())) {
+      dismissed_ids.erase(suggestion_id.id_within_category());
+      StoreOfflinePageDismissedIDsToPrefs(dismissed_ids);
+    }
   } else {
-    StoreAssetDismissedIDsToPrefs(dismissed_ids);
+    std::list<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+    auto it = std::find(dismissed_ids.begin(), dismissed_ids.end(),
+                        suggestion_id.id_within_category());
+    if (it != dismissed_ids.end()) {
+      dismissed_ids.erase(it);
+      StoreAssetDismissedIDsToPrefs(dismissed_ids);
+    }
   }
 }