[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 b1e5fac85aeff9941e248a7713c9f2ebda436d4e..6c652fbd508b70951987b2b44ac58c83b743c9b3 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.cc
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -12,11 +12,13 @@
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"

[Marc Treib, 2016/12/12 09:29:15]

Included twice now

[vitaliii, 2016/12/12 15:41:58]

Done.

+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #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"
@@ -29,6 +31,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
 
+using base::ContainsValue;
 using content::DownloadItem;
 using content::DownloadManager;
 using ntp_snippets::Category;
@@ -49,6 +52,9 @@ const char kOfflinePageDownloadsPrefix = 'O';
 
 const char* kMaxSuggestionsCountParamName = "downloads_max_count";
 
+// Maximal number of dismissed asset download IDs stored at any time.
+const int kMaxDismissedIdCount = 100;
+
 bool CompareOfflinePagesMostRecentlyCreatedFirst(const OfflinePageItem& left,
                                                  const OfflinePageItem& right) {
   return left.creation_time > right.creation_time;
@@ -186,12 +192,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);
 }
 
@@ -262,7 +264,7 @@ void DownloadSuggestionsProvider::GetDismissedSuggestionsForDebugging(
 void DownloadSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   DCHECK_EQ(provided_category_, category);
-  StoreAssetDismissedIDsToPrefs(std::set<std::string>());
+  StoreAssetDismissedIDsToPrefs(std::vector<std::string>());
   StoreOfflinePageDismissedIDsToPrefs(std::set<std::string>());
   AsynchronouslyFetchAllDownloadsAndSubmitSuggestions();
 }
@@ -274,6 +276,10 @@ void DownloadSuggestionsProvider::RegisterProfilePrefs(
   registry->RegisterListPref(kDismissedOfflinePageDownloadSuggestions);
 }
 
+int DownloadSuggestionsProvider::GetMaxDismissedCountForTesting() {
+  return kMaxDismissedIdCount;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
@@ -293,10 +299,11 @@ void DownloadSuggestionsProvider::
     std::vector<DownloadItem*> all_downloads;
     download_manager_->GetAllDownloads(&all_downloads);
 
-    dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+    std::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();

[Marc Treib, 2016/12/12 09:29:15]

nit: The other scope also as a |dismissed_ids| variable - please give this one a
different name.

[vitaliii, 2016/12/12 15:41:58]

Done.
I renamed both.

 
     for (const DownloadItem* item : all_downloads) {
-      if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId()))) {
+      if (ContainsValue(dismissed_ids,
+                        GetAssetDownloadPerCategoryID(item->GetId()))) {
         suggestions.push_back(ConvertDownloadItem(*item));
       }
     }
@@ -369,7 +376,7 @@ void DownloadSuggestionsProvider::ManagerGoingDown(DownloadManager* manager) {
 }
 
 void DownloadSuggestionsProvider::OnDownloadUpdated(DownloadItem* item) {
-  if (base::ContainsValue(cached_asset_downloads_, item)) {
+  if (ContainsValue(cached_asset_downloads_, item)) {
     if (item->GetFileExternallyRemoved()) {
       InvalidateSuggestion(GetAssetDownloadPerCategoryID(item->GetId()));
     } else {
@@ -450,25 +457,23 @@ 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::vector<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 (!ContainsValue(old_dismissed_ids, within_category_id)) {
       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) {
@@ -566,12 +571,13 @@ bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
     return false;
   }
 
-  if (base::ContainsValue(cached_asset_downloads_, item)) {
+  if (ContainsValue(cached_asset_downloads_, item)) {
     return false;
   }
 
-  std::set<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
-  if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId()))) {
+  std::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  if (ContainsValue(dismissed_ids,
+                    GetAssetDownloadPerCategoryID(item->GetId()))) {
     return false;
   }
 
@@ -709,31 +715,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.

[Marc Treib, 2016/12/12 09:29:15]

vector now :)
Also, this sentence doesn't grammar (or I just don't get it)

[vitaliii, 2016/12/12 15:41:58]

Done.

+// See crbug.com/672758.
+std::vector<std::string>
 DownloadSuggestionsProvider::ReadAssetDismissedIDsFromPrefs() const {
-  return ntp_snippets::prefs::ReadDismissedIDsFromPrefs(
-      *pref_service_, kDismissedAssetDownloadSuggestions);
+  std::vector<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::vector<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>
@@ -752,22 +765,45 @@ 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::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+    // The suggestion may be double dismissed from previously opened NTPs.
+    if (!ContainsValue(dismissed_ids, suggestion_id.id_within_category())) {
+      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() > kMaxDismissedIdCount) {
+        dismissed_ids.erase(dismissed_ids.begin());
+      }
+      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::vector<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);
+    }
   }
 }