[NTP::Downloads] Fetch assets once the manager is loaded.

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"
 #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 57 matching lines @@
       OfflinePageModelQuery::Requirement::INCLUDE_MATCHING);
   return builder.Build(model->GetPolicyController());
 }
 
 }  // namespace
 
 DownloadSuggestionsProvider::DownloadSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     offline_pages::OfflinePageModel* offline_page_model,
     content::DownloadManager* download_manager,
+    DownloadHistory* download_history,
     PrefService* pref_service,
     bool download_manager_ui_enabled)
     : ContentSuggestionsProvider(observer),
       category_status_(CategoryStatus::AVAILABLE_LOADING),
       provided_category_(Category::FromKnownCategory(
           ntp_snippets::KnownCategories::DOWNLOADS)),
       offline_page_model_(offline_page_model),
       download_manager_(download_manager),
+      download_history_(download_history),
       pref_service_(pref_service),
       download_manager_ui_enabled_(download_manager_ui_enabled),
+      is_download_manager_loaded_(false),
       weak_ptr_factory_(this) {
   observer->OnCategoryStatusChanged(this, provided_category_, category_status_);
 
   DCHECK(offline_page_model_ || download_manager_);
   if (offline_page_model_) {
     offline_page_model_->AddObserver(this);
   }
 
   if (download_manager_) {
     download_manager_->AddObserver(this);
+    // May be nullptr in tests.
+    if (download_history_) {
+      download_history_->AddObserver(this);
+    }
   }
 
-  // We explicitly fetch the asset downloads in case some of |OnDownloadCreated|
-  // happened earlier and, therefore, were missed.
-  AsynchronouslyFetchAllDownloadsAndSubmitSuggestions();
+  if (!download_manager_) {
+    // All downloads are fetched when the download manager is loaded, but if it
+    // is disabled this will never happen, so offline pages are fetched here
+    // instead.
+    AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
+  }
 }
 
 DownloadSuggestionsProvider::~DownloadSuggestionsProvider() {
   if (offline_page_model_) {
     offline_page_model_->RemoveObserver(this);
   }
 
   if (download_manager_) {
     download_manager_->RemoveObserver(this);
     UnregisterDownloadItemObservers();
   }
+
+  if (download_history_) {
+    download_history_->RemoveObserver(this);
+  }
 }
 
 CategoryStatus DownloadSuggestionsProvider::GetCategoryStatus(
     Category category) {
   DCHECK_EQ(provided_category_, category);
   return category_status_;
 }
 
 CategoryInfo DownloadSuggestionsProvider::GetCategoryInfo(Category category) {
   DCHECK_EQ(provided_category_, category);
   return CategoryInfo(
       l10n_util::GetStringUTF16(IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER),
       ntp_snippets::ContentSuggestionsCardLayout::MINIMAL_CARD,
       /*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::vector<std::string>());
+  StoreAssetDismissedIDsToPrefs(std::set<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_offline_ids =
-      ReadOfflinePageDismissedIDsFromPrefs();
+  std::set<std::string> dismissed_ids = ReadOfflinePageDismissedIDsFromPrefs();
   std::vector<ContentSuggestion> suggestions;
   for (const OfflinePageItem& item : offline_pages) {
-    if (dismissed_offline_ids.count(
-            GetOfflinePagePerCategoryID(item.offline_id))) {
+    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);
 
-    std::vector<std::string> dismissed_asset_ids =
-        ReadAssetDismissedIDsFromPrefs();
+    dismissed_ids = ReadAssetDismissedIDsFromPrefs();
 
     for (const DownloadItem* item : all_downloads) {
-      if (ContainsValue(dismissed_asset_ids,
-                        GetAssetDownloadPerCategoryID(item->GetId()))) {
+      if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId()))) {
         suggestions.push_back(ConvertDownloadItem(*item));
       }
     }
   }
 
   callback.Run(std::move(suggestions));
 }
 
 void DownloadSuggestionsProvider::OfflinePageModelLoaded(
     offline_pages::OfflinePageModel* model) {
@@ skipping 37 matching lines @@
   DCHECK(offline_page_model_);
   if (IsClientIdForOfflinePageDownload(
           offline_page_model_->GetPolicyController(), client_id)) {
     InvalidateSuggestion(GetOfflinePagePerCategoryID(offline_id));
   }
 }
 
 void DownloadSuggestionsProvider::OnDownloadCreated(DownloadManager* manager,
                                                     DownloadItem* item) {
   DCHECK_EQ(download_manager_, manager);
+
+  if (!is_download_manager_loaded_) {
+    // This notification is ignored before the manager is loaded, because all
+    // downloads will be queried once it is loaded.
+    return;
+  }
+
   // This is called when new downloads are started and on startup for existing
   // ones. We listen to each item to know when it is destroyed.
   item->AddObserver(this);
   if (CacheAssetDownloadIfNeeded(item)) {
     SubmitContentSuggestions();
   }
 }
 
 void DownloadSuggestionsProvider::ManagerGoingDown(DownloadManager* manager) {
   DCHECK_EQ(download_manager_, manager);
   UnregisterDownloadItemObservers();
   download_manager_ = nullptr;
+  if (download_history_) {
+    download_history_->RemoveObserver(this);
+    download_history_ = nullptr;
+  }
 }
 
 void DownloadSuggestionsProvider::OnDownloadUpdated(DownloadItem* item) {
-  if (ContainsValue(cached_asset_downloads_, item)) {
+  DCHECK(is_download_manager_loaded_);
+  if (base::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();
     }
   }
 }
 
 void DownloadSuggestionsProvider::OnDownloadOpened(DownloadItem* item) {
   // Ignored.
 }
 
 void DownloadSuggestionsProvider::OnDownloadRemoved(DownloadItem* item) {
   // Ignored. We listen to |OnDownloadDestroyed| instead. The reason is that
   // we may need to retrieve all downloads, but |OnDownloadRemoved| is called
   // before the download is removed from the list.
 }
 
 void DownloadSuggestionsProvider::OnDownloadDestroyed(
     content::DownloadItem* item) {
+  DCHECK(is_download_manager_loaded_);
+
   item->RemoveObserver(this);
 
   if (!IsDownloadCompleted(*item)) {
     return;
   }
   // TODO(vitaliii): Implement a better way to clean up dismissed IDs (in case
   // some calls are missed).
   InvalidateSuggestion(GetAssetDownloadPerCategoryID(item->GetId()));
 }
 
+void DownloadSuggestionsProvider::OnHistoryQueryComplete() {
+  is_download_manager_loaded_ = true;
+  AsynchronouslyFetchAllDownloadsAndSubmitSuggestions();
+}
+
+void DownloadSuggestionsProvider::OnDownloadHistoryDestroyed() {
+  DCHECK(download_history_);
+  download_history_->RemoveObserver(this);
+  download_history_ = nullptr;
+}
+
 void DownloadSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   DCHECK_NE(CategoryStatus::NOT_PROVIDED, category_status_);
   DCHECK_NE(CategoryStatus::NOT_PROVIDED, new_status);
   if (category_status_ == new_status) {
     return;
   }
   category_status_ = new_status;
   observer()->OnCategoryStatusChanged(this, provided_category_,
                                       category_status_);
@@ skipping 24 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::vector<std::string> old_dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  std::set<std::string> old_dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  std::set<std::string> retained_dismissed_ids;
   cached_asset_downloads_.clear();
-  for (const DownloadItem* item : all_downloads) {
+  for (DownloadItem* item : all_downloads) {
     std::string within_category_id =
         GetAssetDownloadPerCategoryID(item->GetId());
-    if (!ContainsValue(old_dismissed_ids, within_category_id)) {
+    if (!old_dismissed_ids.count(within_category_id)) {
       if (IsDownloadCompleted(*item)) {
         cached_asset_downloads_.push_back(item);
+        item->RemoveObserver(this);

[tschumann, 2017/01/12 11:06:09]

what is this doing? Which role is 'this' playing here? with so many interfaces
we're implementing this is getting really hard to follow.

[vitaliii, 2017/01/16 08:51:35]

This prevents adding this class as observer twice to the same DownloadItem.
Otherwise, the following could occur:
1) We receive OnDownloadCreated(DownloadA) and start listening;
2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the
latter we have never seen before), we start listening to both.

So either we store what we are listening to or do the trick above.

'this' means DownloadSuggestionsProvider. It is listening to each download item,
because this is the only way to get OnDownloadDestroyed after which the item is
removed from the list of all downloads, so we can query all downloads if needed.

[tschumann, 2017/01/16 09:14:05]

=) A comment explaining this briefly would be great.
What I meant with 'which role is "this" playing?' is what interface AddObserver
takes (in this case it's DownloadItem::Observer). It's another sign of cost
we're paying with one class doing too many things -- it's hard to reason about
what's happening.

Let's have a closer look at the flows and see if we can move some functionality
into another class and keep the provider simpler (800+ lines of code for one
class is massive).

[Marc Treib, 2017/01/16 10:33:05]

Yup - please at least add a comment for now, and let's try to improve this whole
situation in a follow-up.

[vitaliii, 2017/01/16 14:32:39]

Done.

I added a comment about RemoveObserver.

+        item->AddObserver(this);
       }
+    } else {
+      retained_dismissed_ids.insert(within_category_id);
     }
   }
 
-  // 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.
+  if (old_dismissed_ids.size() != retained_dismissed_ids.size()) {
+    StoreAssetDismissedIDsToPrefs(retained_dismissed_ids);
+  }
+
   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 (ContainsValue(cached_asset_downloads_, item)) {
+  if (base::ContainsValue(cached_asset_downloads_, item)) {
     return false;
   }
 
-  std::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
-  if (ContainsValue(dismissed_ids,
-                    GetAssetDownloadPerCategoryID(item->GetId()))) {
+  std::set<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  if (dismissed_ids.count(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);
 
-  RemoveFromDismissedStorageIfNeeded(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);
+  }
+
   RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(suggestion_id);
 }
 
-// TODO(vitaliii): Do not use std::vector, when we ensure that pruning happens
-// at the right time (crbug.com/672758).
-std::vector<std::string>
+std::set<std::string>
 DownloadSuggestionsProvider::ReadAssetDismissedIDsFromPrefs() const {
-  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;
+  return ntp_snippets::prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, kDismissedAssetDownloadSuggestions);
 }
 
 void DownloadSuggestionsProvider::StoreAssetDismissedIDsToPrefs(
-    const std::vector<std::string>& dismissed_ids) {
+    const std::set<std::string>& dismissed_ids) {
   DCHECK(std::all_of(
       dismissed_ids.begin(), dismissed_ids.end(),
       [](const std::string& id) { return id[0] == kAssetDownloadsPrefix; }));
-
-  base::ListValue list;
-  for (const std::string& dismissed_id : dismissed_ids) {
-    list.AppendString(dismissed_id);
-  }
-  pref_service_->Set(kDismissedAssetDownloadSuggestions, list);
+  ntp_snippets::prefs::StoreDismissedIDsToPrefs(
+      pref_service_, kDismissedAssetDownloadSuggestions, dismissed_ids);
 }
 
 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);
 }
 
-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());
+std::set<std::string> DownloadSuggestionsProvider::ReadDismissedIDsFromPrefs(
+    bool for_offline_page_downloads) const {
+  if (for_offline_page_downloads) {
+    return ReadOfflinePageDismissedIDsFromPrefs();
+  }
+  return ReadAssetDismissedIDsFromPrefs();
+}
+
+void DownloadSuggestionsProvider::StoreDismissedIDsToPrefs(
+    bool for_offline_page_downloads,
+    const std::set<std::string>& dismissed_ids) {
+  if (for_offline_page_downloads) {
     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);
-    }
+    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);
   }
 }