[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 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);
   return ntp_snippets::GetParamAsInt(
       assets_enabled ? features::kAssetDownloadSuggestionsFeature
                      : features::kOfflinePageDownloadSuggestionsFeature,
       kMaxSuggestionsCountParamName, kDefaultMaxSuggestionsCount);
 }
 
 std::string GetOfflinePagePerCategoryID(int64_t raw_offline_page_id) {
@@ skipping 238 matching lines @@
 
   callback.Run(std::move(suggestions));
 }
 
 void DownloadSuggestionsProvider::OfflinePageModelLoaded(
     offline_pages::OfflinePageModel* model) {
   DCHECK_EQ(offline_page_model_, model);
   AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
 }
 
 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(
     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 326 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 CompareOfflinePagesForSorting(*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);
   }
 }