Add a fallback to creation date for Recent bookmarks on NTP

components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc

Index: components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
diff --git a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
index 7090d1281a185e1bfc740092db81d18213b1f331..201fadce53c265b58010ad58574949864c18674b 100644
--- a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
+++ b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
@@ -28,12 +28,18 @@ using bookmarks::BookmarkNode;
 namespace {
 
 const int kMaxBookmarks = 10;
+const int kMaxBookmarksIfNoVisitData = 3;
 const int kMaxBookmarkAgeInDays = 42;
 
 const char* kMaxBookmarksParamName = "max_count";
+const char* kMaxBookmarksIfNoVisitDataParamName = "max_count_no_visit_data";
 const char* kMaxBookmarkAgeInDaysParamName = "max_age_in_days";
 
-base::Time GetThresholdTime() {
+base::Time GetThresholdTime(bool no_visit_data) {
+  // If we have no data, we list bookmarks by creation date, no age limit.
+  if (no_visit_data)
+    return base::Time();
+
   std::string age_in_days_string = variations::GetVariationParamValueByFeature(
     ntp_snippets::kBookmarkSuggestionsFeature, kMaxBookmarkAgeInDaysParamName);
   int age_in_days = 0;
@@ -43,14 +49,16 @@ base::Time GetThresholdTime() {
   return base::Time::Now() - base::TimeDelta::FromDays(age_in_days);
 }
 
-int GetMaxCount() {
+int GetMaxCount(bool no_visit_data) {
   std::string max_count_string = variations::GetVariationParamValueByFeature(
-    ntp_snippets::kBookmarkSuggestionsFeature, kMaxBookmarksParamName);
+      ntp_snippets::kBookmarkSuggestionsFeature,
+      no_visit_data ? kMaxBookmarksIfNoVisitDataParamName
+                    : kMaxBookmarksParamName);
   int max_count = 0;
   if (base::StringToInt(max_count_string, &max_count))
     return max_count;
 
-  return kMaxBookmarks;
+  return no_visit_data ? kMaxBookmarksIfNoVisitData : kMaxBookmarks;
 }
 
 }  // namespace
@@ -67,7 +75,8 @@ BookmarkSuggestionsProvider::BookmarkSuggestionsProvider(
           category_factory->FromKnownCategory(KnownCategories::BOOKMARKS)),
       bookmark_model_(bookmark_model),
       fetch_requested_(false),
-      end_of_list_last_visit_date_(GetThresholdTime()) {
+      no_visit_data_(false),
+      end_of_list_last_visit_date_(GetThresholdTime(no_visit_data_)) {
   bookmark_model_->AddObserver(this);
   FetchBookmarks();
 }
@@ -150,14 +159,15 @@ void BookmarkSuggestionsProvider::OnWillChangeBookmarkMetaInfo(
     BookmarkModel* model,
     const BookmarkNode* node) {
   // Store the last visit date of the node that is about to change.
-  node_to_change_last_visit_date_ =
-      GetLastVisitDateForBookmarkIfNotDismissed(node);
+  node_to_change_last_visit_date_ = GetLastVisitDateForBookmarkIfNotDismissed(
+      node, no_visit_data_);
 }
 
 void BookmarkSuggestionsProvider::BookmarkMetaInfoChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
-  base::Time time = GetLastVisitDateForBookmarkIfNotDismissed(node);
+  base::Time time = GetLastVisitDateForBookmarkIfNotDismissed(
+      node, no_visit_data_);
   if (time == node_to_change_last_visit_date_ ||
       time < end_of_list_last_visit_date_)
     return;
@@ -173,7 +183,7 @@ void BookmarkSuggestionsProvider::BookmarkNodeRemoved(
       int old_index,
       const bookmarks::BookmarkNode* node,
       const std::set<GURL>& no_longer_bookmarked) {
-  if (GetLastVisitDateForBookmarkIfNotDismissed(node) <
+  if (GetLastVisitDateForBookmarkIfNotDismissed(node, no_visit_data_) <
       end_of_list_last_visit_date_)
     return;
 
@@ -189,7 +199,8 @@ ContentSuggestion BookmarkSuggestionsProvider::ConvertBookmark(
 
   suggestion.set_title(bookmark->GetTitle());
   suggestion.set_snippet_text(base::string16());
-  suggestion.set_publish_date(GetLastVisitDateForBookmark(bookmark));
+  suggestion.set_publish_date(
+      GetLastVisitDateForBookmark(bookmark, no_visit_data_));
   suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark->url().host()));
   return suggestion;
 }
@@ -199,9 +210,12 @@ void BookmarkSuggestionsProvider::FetchBookmarksInternal() {
 
   NotifyStatusChanged(CategoryStatus::AVAILABLE);
 
-  base::Time threshold_time = GetThresholdTime();
+  no_visit_data_ = !IsLastVisitDataAvailable(bookmark_model_);

[Marc Treib, 2016/08/17 11:29:06]

Hm, so this means, a new user will get 3 bookmarks. They visit one of them, and
now they only get a single one (because the other two have creation dates more
than 6 weeks ago).
Is that the behavior we want? I think I'd be happier with some form of graceful
fallback, rather than this hard switch between two modes.

+
+  base::Time threshold_time = GetThresholdTime(no_visit_data_);
   std::vector<const BookmarkNode*> bookmarks = GetRecentlyVisitedBookmarks(
-      bookmark_model_, GetMaxCount(), threshold_time);
+      bookmark_model_, no_visit_data_, GetMaxCount(no_visit_data_),
+      threshold_time);
 
   std::vector<ContentSuggestion> suggestions;
   for (const BookmarkNode* bookmark : bookmarks)