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

chrome/browser/ntp_snippets/download_suggestions_provider.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 "chrome/browser/ntp_snippets/download_suggestions_provider.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/feature_list.h"
 #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"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/pref_util.h"
 #include "components/offline_pages/core/offline_page_model_query.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 #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;
 using ntp_snippets::CategoryInfo;
 using ntp_snippets::CategoryStatus;
 using ntp_snippets::ContentSuggestion;
 using ntp_snippets::prefs::kDismissedAssetDownloadSuggestions;
 using ntp_snippets::prefs::kDismissedOfflinePageDownloadSuggestions;
 using offline_pages::OfflinePageItem;
 using offline_pages::OfflinePageModelQuery;
 using offline_pages::OfflinePageModelQueryBuilder;
 
 namespace {
 
 const int kDefaultMaxSuggestionsCount = 5;
 const char kAssetDownloadsPrefix = 'D';
 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;
 }
 
 int GetMaxSuggestionsCount() {
   bool assets_enabled =
       base::FeatureList::IsEnabled(features::kAssetDownloadSuggestionsFeature);
   return variations::GetVariationParamByFeatureAsInt(
       assets_enabled ? features::kAssetDownloadSuggestionsFeature
@@ skipping 117 matching lines @@
       /*has_more_action=*/false,
       /*has_reload_action=*/false,
       /*has_view_all_action=*/download_manager_ui_enabled_,
       /*show_if_empty=*/false,
       l10n_util::GetStringUTF16(IDS_NTP_DOWNLOADS_SUGGESTIONS_SECTION_EMPTY));
 }
 
 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);
 }
 
 void DownloadSuggestionsProvider::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ntp_snippets::ImageFetchedCallback& callback) {
   // TODO(vitaliii): Fetch proper thumbnail from OfflinePageModel once it is
   // available there.
   // TODO(vitaliii): Provide site's favicon for assets downloads or file type.
   // See crbug.com/631447.
@@ skipping 50 matching lines @@
                    weak_ptr_factory_.GetWeakPtr(), callback));
   } else {
     GetPagesMatchingQueryCallbackForGetDismissedSuggestions(
         callback, std::vector<OfflinePageItem>());
   }
 }
 
 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();
 }
 
 // static
 void DownloadSuggestionsProvider::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
   registry->RegisterListPref(kDismissedAssetDownloadSuggestions);
   registry->RegisterListPref(kDismissedOfflinePageDownloadSuggestions);
 }
 
+int DownloadSuggestionsProvider::GetMaxDismissedCountForTesting() {
+  return kMaxDismissedIdCount;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 void DownloadSuggestionsProvider::
     GetPagesMatchingQueryCallbackForGetDismissedSuggestions(
         const ntp_snippets::DismissedSuggestionsCallback& callback,
         const std::vector<OfflinePageItem>& offline_pages) const {
   std::set<std::string> dismissed_ids = ReadOfflinePageDismissedIDsFromPrefs();
   std::vector<ContentSuggestion> suggestions;
   for (const OfflinePageItem& item : offline_pages) {
     if (dismissed_ids.count(GetOfflinePagePerCategoryID(item.offline_id))) {
       suggestions.push_back(ConvertOfflinePage(item));
     }
   }
 
   if (download_manager_) {
     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));
       }
     }
   }
 
   callback.Run(std::move(suggestions));
 }
 
 void DownloadSuggestionsProvider::OfflinePageModelLoaded(
     offline_pages::OfflinePageModel* model) {
@@ skipping 52 matching lines @@
   }
 }
 
 void DownloadSuggestionsProvider::ManagerGoingDown(DownloadManager* manager) {
   DCHECK_EQ(download_manager_, manager);
   UnregisterDownloadItemObservers();
   download_manager_ = nullptr;
 }
 
 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 {
       // The download may have changed.
       SubmitContentSuggestions();
     }
   } else {
     // Unfinished downloads may become completed.
     if (CacheAssetDownloadIfNeeded(item)) {
       SubmitContentSuggestions();
@@ skipping 60 matching lines @@
 }
 
 void DownloadSuggestionsProvider::FetchAssetsDownloads() {
   if (!download_manager_) {
     // The manager has gone down or was explicitly turned off.
     return;
   }
 
   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) {
     // Partially sorts |downloads| such that:
     // 1) The element at the index |max_suggestions_count| is changed to the
     //    element which would occur on this position if |downloads| was sorted;
     // 2) All of the elements before index |max_suggestions_count| are less than
     //    or equal to the elements after it.
     std::nth_element(cached_asset_downloads_.begin(),
                      cached_asset_downloads_.begin() + max_suggestions_count,
@@ skipping 77 matching lines @@
   suggestion.set_download_suggestion_extra(std::move(extra));
   return suggestion;
 }
 
 bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
     const content::DownloadItem* item) {
   if (!IsDownloadCompleted(*item)) {
     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;
   }
 
   DCHECK_LE(static_cast<int>(cached_asset_downloads_.size()),
             GetMaxSuggestionsCount());
   if (static_cast<int>(cached_asset_downloads_.size()) ==
       GetMaxSuggestionsCount()) {
     auto oldest = std::max_element(
         cached_asset_downloads_.begin(), cached_asset_downloads_.end(),
         &CompareDownloadsMostRecentlyDownloadedFirst);
@@ skipping 117 matching lines @@
   if (notify) {
     SubmitContentSuggestions();
   }
 }
 
 void DownloadSuggestionsProvider::InvalidateSuggestion(
     const std::string& id_within_category) {
   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>
 DownloadSuggestionsProvider::ReadOfflinePageDismissedIDsFromPrefs() const {
   return ntp_snippets::prefs::ReadDismissedIDsFromPrefs(
       *pref_service_, kDismissedOfflinePageDownloadSuggestions);
 }
 
 void DownloadSuggestionsProvider::StoreOfflinePageDismissedIDsToPrefs(
     const std::set<std::string>& dismissed_ids) {
   DCHECK(std::all_of(dismissed_ids.begin(), dismissed_ids.end(),
                      [](const std::string& id) {
                        return id[0] == kOfflinePageDownloadsPrefix;
                      }));
   ntp_snippets::prefs::StoreDismissedIDsToPrefs(
       pref_service_, kDismissedOfflinePageDownloadSuggestions, dismissed_ids);
 }
 
-std::set<std::string> DownloadSuggestionsProvider::ReadDismissedIDsFromPrefs(
-    bool for_offline_page_downloads) const {
-  if (for_offline_page_downloads) {
-    return ReadOfflinePageDismissedIDsFromPrefs();
-  }
-  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) {
+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 {
-    StoreAssetDismissedIDsToPrefs(dismissed_ids);
+    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);
+    }
   }
 }
 
+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 {
+    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);
+    }
+  }
+}
+
 void DownloadSuggestionsProvider::UnregisterDownloadItemObservers() {
   DCHECK_NE(download_manager_, nullptr);
 
   std::vector<DownloadItem*> all_downloads;
   download_manager_->GetAllDownloads(&all_downloads);
 
   for (DownloadItem* item : all_downloads) {
     item->RemoveObserver(this);
   }
 }