[Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded

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"
@@ skipping 30 matching lines @@
 using offline_pages::OfflinePageModelQueryBuilder;
 
 namespace {
 
 const int kDefaultMaxSuggestionsCount = 5;
 const char kAssetDownloadsPrefix = 'D';
 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);
   return ntp_snippets::GetParamAsInt(
       assets_enabled ? features::kAssetDownloadSuggestionsFeature
                      : features::kOfflinePageDownloadSuggestionsFeature,
       kMaxSuggestionsCountParamName, kDefaultMaxSuggestionsCount);
 }
 
 std::string GetOfflinePagePerCategoryID(int64_t raw_offline_page_id) {
@@ skipping 26 matching lines @@
 
 bool IsDownloadCompleted(const DownloadItem& item) {
   return item.GetState() == DownloadItem::DownloadState::COMPLETE &&
          !item.GetFileExternallyRemoved();
 }
 
 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,
     const offline_pages::ClientId& client_id) {
   return policy_controller->IsSupportedByDownload(client_id.name_space);
 }
 
 std::unique_ptr<OfflinePageModelQuery> BuildOfflinePageDownloadsQuery(
     offline_pages::OfflinePageModel* model) {
   OfflinePageModelQueryBuilder builder;
@@ skipping 189 matching lines @@
 
 void DownloadSuggestionsProvider::OfflinePageModelLoaded(
     offline_pages::OfflinePageModel* model) {
   DCHECK_EQ(offline_page_model_, model);
   AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
 }
 
 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(
     int64_t offline_id,
     const offline_pages::ClientId& client_id) {
   DCHECK(offline_page_model_);
   if (IsClientIdForOfflinePageDownload(
           offline_page_model_->GetPolicyController(), client_id)) {
     InvalidateSuggestion(GetOfflinePagePerCategoryID(offline_id));
   }
@@ skipping 121 matching lines @@
   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,
                      cached_asset_downloads_.end(),
-                     OrderDownloadsMostRecentlyDownloadedFirst());
+                     &CompareDownloadsMostRecentlyDownloadedFirst);
     cached_asset_downloads_.resize(max_suggestions_count);
   }
 }
 
 void DownloadSuggestionsProvider::
     AsynchronouslyFetchAllDownloadsAndSubmitSuggestions() {
   FetchAssetsDownloads();
   AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
 }
 
@@ skipping 78 matching lines @@
 
   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(),
-                                   OrderDownloadsMostRecentlyDownloadedFirst());
+    auto oldest = std::max_element(
+        cached_asset_downloads_.begin(), cached_asset_downloads_.end(),
+        &CompareDownloadsMostRecentlyDownloadedFirst);
     if (GetAssetDownloadPublishedTime(*item) <=
         GetAssetDownloadPublishedTime(**oldest)) {
       return false;
     }
 
     *oldest = item;
   } else {
     cached_asset_downloads_.push_back(item);
   }
 
@@ skipping 83 matching lines @@
   const int max_suggestions_count = GetMaxSuggestionsCount();
   if (static_cast<int>(items.size()) > max_suggestions_count) {
     // Partially sorts |items| such that:
     // 1) The element at the index |max_suggestions_count| is changed to the
     //    element which would occur on this position if |items| 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(
         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);
   }
 
   cached_offline_page_downloads_.clear();
   for (const OfflinePageItem* item : items) {
     cached_offline_page_downloads_.push_back(*item);
   }
 
   if (old_dismissed_ids.size() != retained_dismissed_ids.size()) {
@@ skipping 75 matching lines @@
 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);
   }
 }