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

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"
 #include "base/feature_list.h"
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/pref_util.h"
 #include "components/offline_pages/core/offline_page_model_query.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
 
 using content::DownloadItem;
 using content::DownloadManager;
 using ntp_snippets::Category;
 using ntp_snippets::CategoryInfo;
 using ntp_snippets::CategoryStatus;
 using ntp_snippets::ContentSuggestion;
 using ntp_snippets::prefs::kDismissedAssetDownloadSuggestions;
 using ntp_snippets::prefs::kDismissedOfflinePageDownloadSuggestions;
 using offline_pages::OfflinePageItem;
 using offline_pages::OfflinePageModelQuery;
 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() {
   bool assets_enabled =
       base::FeatureList::IsEnabled(features::kAssetDownloadSuggestionsFeature);
   return variations::GetVariationParamByFeatureAsInt(
       assets_enabled ? features::kAssetDownloadSuggestionsFeature
                      : features::kOfflinePageDownloadSuggestionsFeature,
       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) +
          base::IntToString(raw_offline_page_id);
 }
 
 std::string GetAssetDownloadPerCategoryID(uint32_t raw_download_id) {
   // Raw ID is prefixed in order to avoid conflicts with offline page downloads.
   return std::string(1, kAssetDownloadsPrefix) +
          base::UintToString(raw_download_id);
 }
 
 // Determines whether |suggestion_id| corresponds to offline page suggestion or
 // asset download based on |id_within_category| prefix.
 bool CorrespondsToOfflinePage(const ContentSuggestion::ID& suggestion_id) {
   const std::string& id_within_category = suggestion_id.id_within_category();
   if (!id_within_category.empty()) {
     if (id_within_category[0] == kOfflinePageDownloadsPrefix) {
       return true;
     }
     if (id_within_category[0] == kAssetDownloadsPrefix) {
       return false;
     }
   }
   NOTREACHED() << "Unknown id_within_category " << id_within_category;
   return false;
 }
 
-bool IsDownloadCompleted(const DownloadItem& item) {
+bool IsAssetDownloadCompleted(const DownloadItem& item) {
   return item.GetState() == DownloadItem::DownloadState::COMPLETE &&
          !item.GetFileExternallyRemoved();
 }
 
 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);
 }
 
 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;
   builder.RequireSupportedByDownload(
       OfflinePageModelQuery::Requirement::INCLUDE_MATCHING);
   return builder.Build(model->GetPolicyController());
 }
 
 }  // namespace
 
 DownloadSuggestionsProvider::DownloadSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     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(
           ntp_snippets::KnownCategories::DOWNLOADS)),
       offline_page_model_(offline_page_model),
       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_);
 
   DCHECK(offline_page_model_ || download_manager_);
   if (offline_page_model_) {
     offline_page_model_->AddObserver(this);
   }
 
   if (download_manager_) {
@@ skipping 189 matching lines @@
             0U);
 
   int max_suggestions_count = GetMaxSuggestionsCount();
   if (static_cast<int>(cached_offline_page_downloads_.size()) <
       max_suggestions_count) {
     cached_offline_page_downloads_.push_back(added_page);
   } else if (max_suggestions_count > 0) {
     auto oldest_page_iterator =
         std::max_element(cached_offline_page_downloads_.begin(),
                          cached_offline_page_downloads_.end(),
-                         &CompareOfflinePagesMostRecentlyCreatedFirst);
+                         &CompareOfflinePagesMostRecentlyPublishedFirst);
     *oldest_page_iterator = added_page;
   }
 
   SubmitContentSuggestions();
 }
 
 void DownloadSuggestionsProvider::OfflinePageDeleted(
     int64_t offline_id,
     const offline_pages::ClientId& client_id) {
   DCHECK(offline_page_model_);
@@ skipping 52 matching lines @@
   // we may need to retrieve all downloads, but |OnDownloadRemoved| is called
   // before the download is removed from the list.
 }
 
 void DownloadSuggestionsProvider::OnDownloadDestroyed(
     content::DownloadItem* item) {
   DCHECK(is_asset_downloads_initialization_complete_);
 
   item->RemoveObserver(this);
 
-  if (!IsDownloadCompleted(*item)) {
+  if (!IsAssetDownloadCompleted(*item)) {
     return;
   }
   // TODO(vitaliii): Implement a better way to clean up dismissed IDs (in case
   // some calls are missed).
   InvalidateSuggestion(GetAssetDownloadPerCategoryID(item->GetId()));
 }
 
 void DownloadSuggestionsProvider::OnHistoryQueryComplete() {
   is_asset_downloads_initialization_complete_ = true;
   if (download_manager_) {
@@ skipping 53 matching lines @@
   std::vector<DownloadItem*> all_downloads;
   download_manager_->GetAllDownloads(&all_downloads);
   std::set<std::string> old_dismissed_ids = ReadAssetDismissedIDsFromPrefs();
   std::set<std::string> retained_dismissed_ids;
   cached_asset_downloads_.clear();
   for (DownloadItem* item : all_downloads) {
     std::string within_category_id =
         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.
       item->RemoveObserver(this);
       item->AddObserver(this);
     }
   }
 
   if (old_dismissed_ids.size() != retained_dismissed_ids.size()) {
     StoreAssetDismissedIDsToPrefs(retained_dismissed_ids);
   }
 
   const int max_suggestions_count = GetMaxSuggestionsCount();
   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(),
-                     &CompareDownloadsMostRecentlyDownloadedFirst);
+                     &CompareDownloadsMostRecentlyPublishedFirst);
     cached_asset_downloads_.resize(max_suggestions_count);
   }
 }
 
 void DownloadSuggestionsProvider::
     AsynchronouslyFetchAllDownloadsAndSubmitSuggestions() {
   FetchAssetsDownloads();
   AsynchronouslyFetchOfflinePagesDownloads(/*notify=*/true);
 }
 
@@ skipping 31 matching lines @@
                                                     offline_page.offline_id)),
       offline_page.url);
 
   if (offline_page.title.empty()) {
     // TODO(vitaliii): Remove this fallback once the OfflinePageModel provides
     // titles for all (relevant) OfflinePageItems.
     suggestion.set_title(base::UTF8ToUTF16(offline_page.url.spec()));
   } 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;
   extra->offline_page_id = offline_page.offline_id;
   suggestion.set_download_suggestion_extra(std::move(extra));
   return suggestion;
 }
 
 ContentSuggestion DownloadSuggestionsProvider::ConvertDownloadItem(
     const DownloadItem& download_item) const {
   ContentSuggestion suggestion(
       ContentSuggestion::ID(provided_category_, GetAssetDownloadPerCategoryID(
                                                     download_item.GetId())),
       download_item.GetOriginalUrl());
   suggestion.set_title(
       download_item.GetTargetFilePath().BaseName().LossyDisplayName());
   suggestion.set_publish_date(GetAssetDownloadPublishedTime(download_item));
   suggestion.set_publisher_name(
       base::UTF8ToUTF16(download_item.GetURL().host()));
   auto extra = base::MakeUnique<ntp_snippets::DownloadSuggestionExtra>();
   extra->target_file_path = download_item.GetTargetFilePath();
   extra->mime_type = download_item.GetMimeType();
   extra->is_download_asset = true;
   suggestion.set_download_suggestion_extra(std::move(extra));
   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;
   }
 
   if (base::ContainsValue(cached_asset_downloads_, item)) {
     return false;
   }
 
   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(),
-        &CompareDownloadsMostRecentlyDownloadedFirst);
+    auto oldest = std::max_element(cached_asset_downloads_.begin(),
+                                   cached_asset_downloads_.end(),
+                                   &CompareDownloadsMostRecentlyPublishedFirst);
     if (GetAssetDownloadPublishedTime(*item) <=
         GetAssetDownloadPublishedTime(**oldest)) {
       return false;
     }
 
     *oldest = item;
   } else {
     cached_asset_downloads_.push_back(item);
   }
 
@@ skipping 66 matching lines @@
   DCHECK(!offline_page_model_ || offline_page_model_->is_loaded());
 
   std::set<std::string> old_dismissed_ids =
       ReadOfflinePageDismissedIDsFromPrefs();
   std::set<std::string> retained_dismissed_ids;
   std::vector<const OfflinePageItem*> items;
   // Filtering out dismissed items and pruning dismissed IDs.
   for (const OfflinePageItem& item : all_download_offline_pages) {
     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);
     }
   }
 
   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 CompareOfflinePagesMostRecentlyCreatedFirst(*left, *right);
+          return CompareOfflinePagesMostRecentlyPublishedFirst(*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 74 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);
   }
 }