Do not select old bookmarks in ntp_snippets::BookmarkSuggestionsProvider

components/ntp_snippets/bookmarks/bookmark_last_visit_utils.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 "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "url/gurl.h"
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
 namespace ntp_snippets {
 
 namespace {
 
-struct RecentBookmark {
-  const bookmarks::BookmarkNode* node;
-  bool visited_recently;
-};
-
 const char kBookmarkLastVisitDateKey[] = "last_visited";
 const char kBookmarkDismissedFromNTP[] = "dismissed_from_ntp";
 
 base::Time ParseLastVisitDate(const std::string& date_string) {
   int64_t date = 0;
   if (!base::StringToInt64(date_string, &date))
     return base::Time::UnixEpoch();
   return base::Time::FromInternalValue(date);
 }
 
@@ skipping 84 matching lines @@
 std::vector<const BookmarkNode*> GetRecentlyVisitedBookmarks(
     BookmarkModel* bookmark_model,
     int min_count,
     int max_count,
     const base::Time& min_visit_time,
     bool creation_date_fallback) {
   // Get all the bookmark URLs.
   std::vector<BookmarkModel::URLAndTitle> bookmark_urls;
   bookmark_model->GetBookmarks(&bookmark_urls);
 
-  std::vector<RecentBookmark> bookmarks;
-  int recently_visited_count = 0;
+  std::vector<const BookmarkNode*> recent_not_dismissed_bookmarks;
   // Find for each bookmark the most recently visited BookmarkNode and find out
   // whether it is visited since |min_visit_time|.
   for (const BookmarkModel::URLAndTitle& url_and_title : bookmark_urls) {
     // Get all bookmarks for the given URL.
     std::vector<const BookmarkNode*> bookmarks_for_url;
     bookmark_model->GetNodesByURL(url_and_title.url, &bookmarks_for_url);
     DCHECK(!bookmarks_for_url.empty());
 
     // Find the most recent node (minimal w.r.t.
     // CompareBookmarksByLastVisitDate).
     const BookmarkNode* most_recent = *std::min_element(
         bookmarks_for_url.begin(), bookmarks_for_url.end(),
         [creation_date_fallback](const BookmarkNode* a, const BookmarkNode* b) {
           return CompareBookmarksByLastVisitDate(a, b, creation_date_fallback);
         });
 
-    // Find out if it has been _visited_ recently enough.
-    bool visited_recently =
-        min_visit_time < GetLastVisitDateForBookmark(
-                             most_recent, /*creation_date_fallback=*/false);
-    if (visited_recently)
-      recently_visited_count++;
-    if (!IsDismissedFromNTPForBookmark(most_recent))
-      bookmarks.push_back({most_recent, visited_recently});

[Marc Treib, 2016/09/01 09:46:01]

We want to keep this, so that the creation_date_fallback behavior keeps working
(in case the PMs change their minds again and we need it after all).

[vitaliii, 2016/09/01 10:15:41]

Done.

+    if (min_visit_time < GetLastVisitDateForBookmark(
+                             most_recent, /* creation_date_fallback */ false) &&
+        !IsDismissedFromNTPForBookmark(most_recent)) {
+      recent_not_dismissed_bookmarks.push_back(most_recent);
+    }
   }
 
-  if (recently_visited_count < min_count) {
-    // There aren't enough recently-visited bookmarks. Fill the list up to
-    // |min_count| with older bookmarks (in particular those with only a
-    // creation date, if creation_date_fallback is true).
-    max_count = min_count;
+  if (recent_not_dismissed_bookmarks.size() < static_cast<size_t>(min_count)) {
+    // There aren't enough recently-visited bookmarks, do not show any.

[Marc Treib, 2016/09/01 09:46:01]

This is not the behavior we want (sorry, I must have been unclear in our
discussions): We still want to show the recently-visited bookmarks that we have
in this case, even if it's only one.

[vitaliii, 2016/09/01 10:15:41]

Done.

+    return std::vector<const BookmarkNode*>();
   } else {
-    // Remove the bookmarks that are not recently visited; we do not need them.
-    // (We might end up with fewer than |min_count| bookmarks if all the recent
-    // ones are dismissed.)
-    bookmarks.erase(
-        std::remove_if(bookmarks.begin(), bookmarks.end(),
-                       [](const RecentBookmark& bookmark) {
-                         return !bookmark.visited_recently;
-                       }),
-        bookmarks.end());
+    // Sort the bookmarks we can show by date.
+    std::sort(recent_not_dismissed_bookmarks.begin(),
+              recent_not_dismissed_bookmarks.end(),
+              [creation_date_fallback](const BookmarkNode* first,
+                                       const BookmarkNode* second) {
+                return CompareBookmarksByLastVisitDate(first, second,
+                                                       creation_date_fallback);
+              });
+
+    // Return the first |max_count| items from |recent_not_dismissed_bookmarks|.
+    if (recent_not_dismissed_bookmarks.size() <=
+        static_cast<size_t>(max_count)) {
+      return recent_not_dismissed_bookmarks;
+    } else {
+      return std::vector<const BookmarkNode*>(
+          recent_not_dismissed_bookmarks.begin(),
+          recent_not_dismissed_bookmarks.begin() + max_count);
+    }
   }
-
-  // Sort the remaining entries by date.
-  std::sort(bookmarks.begin(), bookmarks.end(),
-            [creation_date_fallback](const RecentBookmark& a,
-                                     const RecentBookmark& b) {
-              return CompareBookmarksByLastVisitDate(a.node, b.node,
-                                                     creation_date_fallback);
-            });
-
-  // Insert the first |max_count| items from |bookmarks| into |result|.

[Marc Treib, 2016/09/01 09:46:01]

I think the problem is here: We're missing a check that the bookmark dates
aren't older than min_visit_time. I.e. break out of the loop if the current
bookmark is too old.

[vitaliii, 2016/09/01 10:15:41]

Done.

-  std::vector<const BookmarkNode*> result;
-  for (const RecentBookmark& bookmark : bookmarks) {
-    result.push_back(bookmark.node);
-    if (result.size() >= static_cast<size_t>(max_count))
-      break;
-  }
-  return result;
 }
 
 std::vector<const BookmarkNode*> GetDismissedBookmarksForDebugging(
     BookmarkModel* bookmark_model) {
   // Get all the bookmark URLs.
   std::vector<BookmarkModel::URLAndTitle> bookmarks;
   bookmark_model->GetBookmarks(&bookmarks);
 
   // Remove the bookmark URLs which have at least one non-dismissed bookmark.
   bookmarks.erase(
@@ skipping 15 matching lines @@
   // Insert into |result|.
   std::vector<const BookmarkNode*> result;
   for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
     result.push_back(
         bookmark_model->GetMostRecentlyAddedUserNodeForURL(bookmark.url));
   }
   return result;
 }
 
 }  // namespace ntp_snippets