[Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded

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 1189dd79942bbbc9192d5a45bfca4d5275f14de7..7998a815cada80af2a8e3e6eb1c6f2ae9dbdbe39 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.cc
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -48,6 +48,11 @@ const char kOfflinePageDownloadsPrefix = 'O';
 
 const char* kMaxSuggestionsCountParamName = "downloads_max_count";
 
+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);
@@ -94,12 +99,11 @@ base::Time GetAssetDownloadPublishedTime(const DownloadItem& item) {
   return item.GetStartTime();
 }
 
-struct OrderDownloadsMostRecentlyDownloadedFirst {
-  bool operator()(const DownloadItem* left, const DownloadItem* right) const {
-    return GetAssetDownloadPublishedTime(*left) >
-           GetAssetDownloadPublishedTime(*right);
-  }
-};
+bool CompareDownloadsMostRecentlyDownloadedFirst(const DownloadItem* left,
+                                                 const DownloadItem* right) {
+  return GetAssetDownloadPublishedTime(*left) >
+         GetAssetDownloadPublishedTime(*right);
+}
 
 bool IsClientIdForOfflinePageDownload(
     offline_pages::ClientPolicyController* policy_controller,
@@ -309,9 +313,30 @@ void DownloadSuggestionsProvider::OfflinePageModelLoaded(
 void DownloadSuggestionsProvider::OfflinePageAdded(
     offline_pages::OfflinePageModel* model,
     const offline_pages::OfflinePageItem& added_page) {
-  // TODO(dewittj, vitaliii): Don't refetch everything when this is called.
   DCHECK_EQ(offline_page_model_, model);
-  AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
+  if (!IsClientIdForOfflinePageDownload(model->GetPolicyController(),
+                                        added_page.client_id)) {
+    return;
+  }
+
+  // This is all in one statement so that it is completely compiled out in
+  // release builds.
+  DCHECK_EQ(ReadOfflinePageDismissedIDsFromPrefs().count(

[vitaliii, 2016/12/09 10:02:37]

nit: Why not "DCHECK(!..."?

[dewittj, 2016/12/09 18:43:57]

Feels more correct to compare to an integer rather than implicitly convert to
bool.

+                GetOfflinePagePerCategoryID(added_page.offline_id)),
+            0U);
+
+  if (static_cast<int>(cached_offline_page_downloads_.size()) <
+      GetMaxSuggestionsCount()) {
+    cached_offline_page_downloads_.push_back(added_page);
+  } else {
+    auto oldest_page_iterator =
+        std::max_element(cached_offline_page_downloads_.begin(),
+                         cached_offline_page_downloads_.end(),
+                         &CompareOfflinePagesMostRecentlyCreatedFirst);
+    *oldest_page_iterator = added_page;

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

This could crash if GetMaxSuggestionsCount ever returned 0.

[dewittj, 2016/12/09 18:43:57]

Done.

+  }
+
+  SubmitContentSuggestions();
 }
 
 void DownloadSuggestionsProvider::OfflinePageDeleted(
@@ -453,7 +478,7 @@ void DownloadSuggestionsProvider::FetchAssetsDownloads() {
     std::nth_element(cached_asset_downloads_.begin(),
                      cached_asset_downloads_.begin() + max_suggestions_count,
                      cached_asset_downloads_.end(),
-                     OrderDownloadsMostRecentlyDownloadedFirst());
+                     &CompareDownloadsMostRecentlyDownloadedFirst);
     cached_asset_downloads_.resize(max_suggestions_count);
   }
 }
@@ -552,9 +577,9 @@ bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
             GetMaxSuggestionsCount());
   if (static_cast<int>(cached_asset_downloads_.size()) ==
       GetMaxSuggestionsCount()) {
-    auto oldest = std::max_element(cached_asset_downloads_.begin(),
-                                   cached_asset_downloads_.end(),
-                                   OrderDownloadsMostRecentlyDownloadedFirst());
+    auto oldest = std::max_element(
+        cached_asset_downloads_.begin(), cached_asset_downloads_.end(),
+        &CompareDownloadsMostRecentlyDownloadedFirst);
     if (GetAssetDownloadPublishedTime(*item) <=
         GetAssetDownloadPublishedTime(**oldest)) {
       return false;
@@ -658,7 +683,7 @@ void DownloadSuggestionsProvider::UpdateOfflinePagesCache(
     std::nth_element(
         items.begin(), items.begin() + max_suggestions_count, items.end(),
         [](const OfflinePageItem* left, const OfflinePageItem* right) {
-          return left->creation_time > right->creation_time;
+          return CompareOfflinePagesMostRecentlyCreatedFirst(*left, *right);
         });
     items.resize(max_suggestions_count);
   }