[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..b29791506ea5587a8df16985b55739a458f8c180 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 CompareOfflinePagesForSorting(const OfflinePageItem& left,

[vitaliii, 2016/12/08 17:40:32]

Could you have this consistent (naming, struct) with the struct
|OrderDownloadsMostRecentlyDownloadedFirst| below?
I would call it |OrderOfflinePagesMostRecentlyCreatedFirst| or call both 
|Compare.*| instead of |Order.*|.

[vitaliii, 2016/12/08 18:17:20]

Actually, I understood that then the struct is ugly to use as a function, so
please make both comparators a function and use |Compare| as a prefix.

[dewittj, 2016/12/08 19:09:02]

Done.

[dewittj, 2016/12/08 19:09:02]

Done.

+                                   const OfflinePageItem& right) {
+  return left.creation_time > right.creation_time;
+}
+
 int GetMaxSuggestionsCount() {
   bool assets_enabled =
       base::FeatureList::IsEnabled(features::kAssetDownloadSuggestionsFeature);
@@ -309,9 +314,24 @@ void DownloadSuggestionsProvider::OfflinePageModelLoaded(
 void DownloadSuggestionsProvider::OfflinePageAdded(

[vitaliii, 2016/12/08 17:40:32]

Do I understand right that OfflinePage can be either deleted or added, but not
updated?
If I am right, this feels a bit strange, since you have last accessed time
field, which may be of interest to someone. Previously my assumption was that
model change included this. As far as I am aware we do not use it in Zine as of
now, so this is rather checking out, but not a call to action. 

[dewittj, 2016/12/08 19:09:02]

Currently pages can be accessed (and thus altered) but there is no observer
event fired when that happens.  I am looking into adding an updated event.

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

Acknowledged.

     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;
+  }
+
+  cached_offline_page_downloads_.push_back(added_page);

[vitaliii, 2016/12/08 17:40:32]

You also need to check that this offline page is not dismissed (see
|CacheAssetDownloadIfNeeded| for an example and
|ReadOfflinePageDismissedIDsFromPrefs| for a source of dismissed ids).
I was impressed that this passed our tests, so I will have a good look at them
later.

[vitaliii, 2016/12/08 18:17:20]

Ok, technically you do not need to check this unless we get offline paged added
twice without a removal in between. So please have a 
DCHECK(!ReadOfflinePageDismissedIDsFromPrefs().count(...)) 
instead to check that this page is not dismissed.
Sorry for changing my mind.

[dewittj, 2016/12/08 19:09:02]

I didn't add that check because I assumed that new pages could not possibly
already be dismissed.

[dewittj, 2016/12/08 19:09:02]

Done.

+
+  if (static_cast<int>(cached_offline_page_downloads_.size()) >
+      GetMaxSuggestionsCount()) {
+    auto oldest_page_iterator = std::max_element(
+        cached_offline_page_downloads_.begin(),
+        cached_offline_page_downloads_.end(),
+        &CompareOfflinePagesForSorting);
+    cached_offline_page_downloads_.erase(oldest_page_iterator);

[vitaliii, 2016/12/08 17:40:32]

Could you instead do 
(*oldest_page_iterator) = added_page
if the oldest page is older than the new one and not to add it in advance?
This would omit erasing and do nothing if there is no need to add the page.

[dewittj, 2016/12/08 19:09:02]

Done.

+  }
+
+  SubmitContentSuggestions();
 }
 
 void DownloadSuggestionsProvider::OfflinePageDeleted(
@@ -658,7 +678,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 CompareOfflinePagesForSorting(*left, *right);
         });
     items.resize(max_suggestions_count);
   }