[NTP::Downloads] Do not show old downloads.

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 2b43b4eb9c4f3f1b10eb12df0fca10267a5916ae..82a2a149cf4ce1a50068aea8f201f2c6916d6416 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.cc
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -16,6 +16,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/clock.h"
 #include "base/time/time.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
@@ -44,14 +45,21 @@ using offline_pages::OfflinePageModelQueryBuilder;
 namespace {
 
 const int kDefaultMaxSuggestionsCount = 5;
+const int kDefaultMaxDownloadAgeDays = 6 * 7;  // 6 weeks
 const char kAssetDownloadsPrefix = 'D';
 const char kOfflinePageDownloadsPrefix = 'O';
 
 const char* kMaxSuggestionsCountParamName = "downloads_max_count";
+const char* kMaxDownloadAgeDaysParamName = "downloads_max_age_days";
 
-bool CompareOfflinePagesMostRecentlyCreatedFirst(const OfflinePageItem& left,
-                                                 const OfflinePageItem& right) {
-  return left.creation_time > right.creation_time;
+base::Time GetOfflinePagePublishedTime(const OfflinePageItem& item) {
+  return item.creation_time;
+}
+
+bool CompareOfflinePagesMostRecentlyPublishedFirst(
+    const OfflinePageItem& left,
+    const OfflinePageItem& right) {
+  return GetOfflinePagePublishedTime(left) > GetOfflinePagePublishedTime(right);
 }
 
 int GetMaxSuggestionsCount() {
@@ -63,6 +71,15 @@ int GetMaxSuggestionsCount() {
       kMaxSuggestionsCountParamName, kDefaultMaxSuggestionsCount);
 }
 
+int GetMaxDownloadAgeDays() {
+  bool assets_enabled =
+      base::FeatureList::IsEnabled(features::kAssetDownloadSuggestionsFeature);
+  return variations::GetVariationParamByFeatureAsInt(
+      assets_enabled ? features::kAssetDownloadSuggestionsFeature

[tschumann, 2017/02/02 14:30:33]

+Jan, can you double-check this is sane?

It's the first time I see a variation parameter being bound to different
features? Does that mean we need to be very careful how to roll out experiments?
Or do we need to set both?

[jkrcal, 2017/02/02 16:06:16]

After offline discussion, this is sane, please add a comment to explain.

[vitaliii, 2017/02/07 10:26:05]

Done.

I added comments. 

|GetVariationParamByFeatureAsInt| returns std::string(), if the feature is
turned off.
Since one of the features may be disabled, we choose the enabled one.

For the experiments, setting both sounds like the safest option.

+                     : features::kOfflinePageDownloadSuggestionsFeature,
+      kMaxDownloadAgeDaysParamName, kDefaultMaxDownloadAgeDays);
+}
+
 std::string GetOfflinePagePerCategoryID(int64_t raw_offline_page_id) {
   // Raw ID is prefixed in order to avoid conflicts with asset downloads.
   return std::string(1, kOfflinePageDownloadsPrefix) +
@@ -91,7 +108,7 @@ bool CorrespondsToOfflinePage(const ContentSuggestion::ID& suggestion_id) {
   return false;
 }
 
-bool IsDownloadCompleted(const DownloadItem& item) {
+bool IsAssetDownloadCompleted(const DownloadItem& item) {
   return item.GetState() == DownloadItem::DownloadState::COMPLETE &&
          !item.GetFileExternallyRemoved();
 }
@@ -100,8 +117,8 @@ base::Time GetAssetDownloadPublishedTime(const DownloadItem& item) {
   return item.GetStartTime();
 }
 
-bool CompareDownloadsMostRecentlyDownloadedFirst(const DownloadItem* left,
-                                                 const DownloadItem* right) {
+bool CompareDownloadsMostRecentlyPublishedFirst(const DownloadItem* left,
+                                                const DownloadItem* right) {
   return GetAssetDownloadPublishedTime(*left) >
          GetAssetDownloadPublishedTime(*right);
 }
@@ -127,7 +144,8 @@ DownloadSuggestionsProvider::DownloadSuggestionsProvider(
     offline_pages::OfflinePageModel* offline_page_model,
     content::DownloadManager* download_manager,
     DownloadHistory* download_history,
-    PrefService* pref_service)
+    PrefService* pref_service,
+    std::unique_ptr<base::Clock> clock)
     : ContentSuggestionsProvider(observer),
       category_status_(CategoryStatus::AVAILABLE_LOADING),
       provided_category_(Category::FromKnownCategory(
@@ -136,6 +154,7 @@ DownloadSuggestionsProvider::DownloadSuggestionsProvider(
       download_manager_(download_manager),
       download_history_(download_history),
       pref_service_(pref_service),
+      clock_(std::move(clock)),
       is_asset_downloads_initialization_complete_(false),
       weak_ptr_factory_(this) {
   observer->OnCategoryStatusChanged(this, provided_category_, category_status_);
@@ -345,7 +364,7 @@ void DownloadSuggestionsProvider::OfflinePageAdded(
     auto oldest_page_iterator =
         std::max_element(cached_offline_page_downloads_.begin(),
                          cached_offline_page_downloads_.end(),
-                         &CompareOfflinePagesMostRecentlyCreatedFirst);
+                         &CompareOfflinePagesMostRecentlyPublishedFirst);
     *oldest_page_iterator = added_page;
   }
 
@@ -418,7 +437,7 @@ void DownloadSuggestionsProvider::OnDownloadDestroyed(
 
   item->RemoveObserver(this);
 
-  if (!IsDownloadCompleted(*item)) {
+  if (!IsAssetDownloadCompleted(*item)) {
     return;
   }
   // TODO(vitaliii): Implement a better way to clean up dismissed IDs (in case
@@ -492,7 +511,8 @@ void DownloadSuggestionsProvider::FetchAssetsDownloads() {
         GetAssetDownloadPerCategoryID(item->GetId());
     if (old_dismissed_ids.count(within_category_id)) {
       retained_dismissed_ids.insert(within_category_id);
-    } else if (IsDownloadCompleted(*item)) {
+    } else if (IsAssetDownloadCompleted(*item) &&
+               !IsDownloadOld(GetAssetDownloadPublishedTime(*item))) {
       cached_asset_downloads_.push_back(item);
       // We may already observe this item and, therefore, we remove the
       // observer first.
@@ -516,7 +536,7 @@ void DownloadSuggestionsProvider::FetchAssetsDownloads() {
     std::nth_element(cached_asset_downloads_.begin(),
                      cached_asset_downloads_.begin() + max_suggestions_count,
                      cached_asset_downloads_.end(),
-                     &CompareDownloadsMostRecentlyDownloadedFirst);
+                     &CompareDownloadsMostRecentlyPublishedFirst);
     cached_asset_downloads_.resize(max_suggestions_count);
   }
 }
@@ -568,7 +588,7 @@ ContentSuggestion DownloadSuggestionsProvider::ConvertOfflinePage(
   } else {
     suggestion.set_title(offline_page.title);
   }
-  suggestion.set_publish_date(offline_page.creation_time);
+  suggestion.set_publish_date(GetOfflinePagePublishedTime(offline_page));
   suggestion.set_publisher_name(base::UTF8ToUTF16(offline_page.url.host()));
   auto extra = base::MakeUnique<ntp_snippets::DownloadSuggestionExtra>();
   extra->is_download_asset = false;
@@ -596,9 +616,17 @@ ContentSuggestion DownloadSuggestionsProvider::ConvertDownloadItem(
   return suggestion;
 }
 
+bool DownloadSuggestionsProvider::IsDownloadOld(
+    const base::Time& published_time) {
+  // TODO(vitaliii): Also consider last access time here once it is collected
+  // for asset downloads.
+  return published_time <
+         clock_->Now() - base::TimeDelta::FromDays(GetMaxDownloadAgeDays());
+}
+
 bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
     const content::DownloadItem* item) {
-  if (!IsDownloadCompleted(*item)) {
+  if (!IsAssetDownloadCompleted(*item)) {
     return false;
   }
 
@@ -615,9 +643,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(),
-        &CompareDownloadsMostRecentlyDownloadedFirst);
+    auto oldest = std::max_element(cached_asset_downloads_.begin(),
+                                   cached_asset_downloads_.end(),
+                                   &CompareDownloadsMostRecentlyPublishedFirst);
     if (GetAssetDownloadPublishedTime(*item) <=
         GetAssetDownloadPublishedTime(**oldest)) {
       return false;
@@ -705,7 +733,9 @@ void DownloadSuggestionsProvider::UpdateOfflinePagesCache(
     std::string id_within_category =
         GetOfflinePagePerCategoryID(item.offline_id);
     if (!old_dismissed_ids.count(id_within_category)) {

[tschumann, 2017/02/02 14:30:33]

nit: It's now harder to get the context of the the } else { part.
inverse?
Maybe even split up into single, positive ifs:
if (old_dismissed_ids.count()) {
  retained_dismissed_ids.insert();
  continue;
}
if (IsDownloadOld(...))) {
  continue;
}
items.push_back(...);

[vitaliii, 2017/02/07 10:26:05]

Done. I've inversed the outer |if|.

-      items.push_back(&item);
+      if (!IsDownloadOld(GetOfflinePagePublishedTime(item))) {

[vitaliii, 2017/02/02 14:06:55]

tschumann@, do we want to use last access time for offline pages already?
I believe yes, but I feel like doing it in a separate CL.

[tschumann, 2017/02/02 14:30:33]

i'm fine doing this in one CL.

[vitaliii, 2017/02/07 10:26:05]

Acknowledged.

+        items.push_back(&item);
+      }
     } else {
       retained_dismissed_ids.insert(id_within_category);
     }
@@ -721,7 +751,7 @@ void DownloadSuggestionsProvider::UpdateOfflinePagesCache(
     std::nth_element(
         items.begin(), items.begin() + max_suggestions_count, items.end(),
         [](const OfflinePageItem* left, const OfflinePageItem* right) {
-          return CompareOfflinePagesMostRecentlyCreatedFirst(*left, *right);
+          return CompareOfflinePagesMostRecentlyPublishedFirst(*left, *right);
         });
     items.resize(max_suggestions_count);
   }