Add a fallback to creation date for Recent bookmarks on NTP

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/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 18 matching lines @@
   // If there are bookmarks for |url|, set their last visit date to now.
   std::string now = FormatLastVisitDate(base::Time::Now());
   for (const BookmarkNode* node : bookmarks_for_url) {
     bookmark_model->SetNodeMetaInfo(node, kBookmarkLastVisitDateKey, now);
     // If the bookmark has been dismissed from NTP before, a new visit overrides
     // such a dismission.
     bookmark_model->DeleteNodeMetaInfo(node, kBookmarkDismissedFromNTP);
   }
 }
 
-base::Time GetLastVisitDateForBookmark(const BookmarkNode* node) {
+base::Time GetLastVisitDateForBookmark(const BookmarkNode* node,
+                                       bool creation_date_fallback) {
   if (!node)
     return base::Time::UnixEpoch();
 
   std::string last_visit_date_string;
-  node->GetMetaInfo(kBookmarkLastVisitDateKey, &last_visit_date_string);
+  if (!node->GetMetaInfo(kBookmarkLastVisitDateKey, &last_visit_date_string) &&
+      creation_date_fallback)
+    return node->date_added();

[Marc Treib, 2016/08/18 09:31:44]

nit: Should have braces if the condition doesn't fit on one line

[Philipp Keck, 2016/08/19 11:20:54]

Done.

+
   return ParseLastVisitDate(last_visit_date_string);
 }
 
-base::Time GetLastVisitDateForBookmarkIfNotDismissed(const BookmarkNode* node) {
+base::Time GetLastVisitDateForBookmarkIfNotDismissed(
+    const BookmarkNode* node,
+    bool creation_date_fallback) {
   if (IsDismissedFromNTPForBookmark(node))
     return base::Time::UnixEpoch();
 
-  return GetLastVisitDateForBookmark(node);
+  return GetLastVisitDateForBookmark(node, creation_date_fallback);
 }
 
 void MarkBookmarksDismissed(BookmarkModel* bookmark_model, const GURL& url) {
   std::vector<const BookmarkNode*> nodes;
   bookmark_model->GetNodesByURL(url, &nodes);
   for (const BookmarkNode* node : nodes)
     bookmark_model->SetNodeMetaInfo(node, kBookmarkDismissedFromNTP, "1");
 }
 
 bool IsDismissedFromNTPForBookmark(const BookmarkNode* node) {
@@ skipping 16 matching lines @@
   for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
     std::vector<const BookmarkNode*> nodes;
     bookmark_model->GetNodesByURL(bookmark.url, &nodes);
     for (const BookmarkNode* node : nodes)
       bookmark_model->DeleteNodeMetaInfo(node, kBookmarkDismissedFromNTP);
   }
 }
 
 std::vector<const BookmarkNode*> GetRecentlyVisitedBookmarks(
     BookmarkModel* bookmark_model,
+    int min_count,
     int max_count,
     const base::Time& min_visit_time) {
   // Get all the bookmark URLs.
-  std::vector<BookmarkModel::URLAndTitle> bookmarks;
-  bookmark_model->GetBookmarks(&bookmarks);
+  std::vector<BookmarkModel::URLAndTitle> bookmark_urls;
+  bookmark_model->GetBookmarks(&bookmark_urls);
 
-  // Remove the URLs for that no bookmark has a recent visit (more recent than
-  // |min_visit_time|). Recent visits to dismissed bookmarks are not considered.
-  bookmarks.erase(
-      std::remove_if(bookmarks.begin(), bookmarks.end(),
-                     [&bookmark_model, &min_visit_time](
-                         const BookmarkModel::URLAndTitle& bookmark) {
-                       // Get all bookmarks for the given URL.
-                       std::vector<const BookmarkNode*> bookmarks_for_url;
-                       bookmark_model->GetNodesByURL(bookmark.url,
-                                                     &bookmarks_for_url);
+  std::vector<RecentBookmark> bookmarks;
+  int recently_visited_count = 0;
+  // 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);
 
-                       // Keep if at least one (non-dismissed) bookmark has been
-                       // recently visited.
-                       for (const BookmarkNode* node : bookmarks_for_url) {
-                         if (GetLastVisitDateForBookmarkIfNotDismissed(node) >
-                             min_visit_time)
-                           return false;
-                       }
-                       // Otherwise erase this URL.
-                       return true;
-                     }),
-      bookmarks.end());
+    // Find the most recent node (minimal w.r.t.
+    // CompareBookmarksByLastVisitDate).
+    std::vector<const BookmarkNode*>::iterator most_recent =
+        std::min_element(bookmarks_for_url.begin(), bookmarks_for_url.end(),
+                         &CompareBookmarksByLastVisitDate);
+    if (most_recent == bookmarks_for_url.end())

[Marc Treib, 2016/08/18 09:31:44]

Can this ever happen? I think it can't, so I'd just remove the check.

[Philipp Keck, 2016/08/19 11:20:54]

Right, done. We could think about adding DCHECK(bookmarks_for_url.size()) above,
but that shouldn't be necessary either (unless we consider concurrent updates to
the bookmark model?).

[Marc Treib, 2016/08/19 11:25:41]

Well, a DCHECK is never stricly necessary, it just documents some pre-condition
or assumption. So it might make sense to add one here.

[Philipp Keck, 2016/08/19 12:27:45]

Done.
https://codereview.chromium.org/2256183004/diff2/20001:40001/components/ntp_s...

+      continue;
+    const BookmarkNode* node = *most_recent;
 
-  // Sort the remaining bookmarks by date.
+    // Find out if it has been _visited_ recently enough.
+    if (GetLastVisitDateForBookmarkIfNotDismissed(
+            node, /*creation_date_fallback=*/false) > min_visit_time) {
+      recently_visited_count++;
+      bookmarks.push_back({node, true});
+    } else {
+      bookmarks.push_back({node, false});
+    }
+  }
+
+  if (recently_visited_count < min_count) {
+    // Fill the list up to |min_count| but do not display more.
+    max_count = min_count;
+  } else {
+    // Remove the bookmarks that are not recently visited; we do no need them.

[Marc Treib, 2016/08/18 09:31:44]

nit: we do noT need them

[Philipp Keck, 2016/08/19 11:20:54]

Done.

+    bookmarks.erase(
+        std::remove_if(bookmarks.begin(), bookmarks.end(),
+                       [](const RecentBookmark& bookmark) {
+                         return !bookmark.visited_recently;
+                       }),
+        bookmarks.end());
+  }
+
+  // Sort the remaining entries by date.
   std::sort(bookmarks.begin(), bookmarks.end(),
-            [&bookmark_model](const BookmarkModel::URLAndTitle& a,
-                              const BookmarkModel::URLAndTitle& b) {
-              return CompareBookmarksByLastVisitDate(
-                  bookmark_model->GetMostRecentlyAddedUserNodeForURL(a.url),
-                  bookmark_model->GetMostRecentlyAddedUserNodeForURL(b.url));
+            [](const RecentBookmark& a, const RecentBookmark& b) {
+              return CompareBookmarksByLastVisitDate(a.node, b.node);
             });
 
   // Insert the first |max_count| items from |bookmarks| into |result|.
   std::vector<const BookmarkNode*> result;
-  for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
-    result.push_back(
-        bookmark_model->GetMostRecentlyAddedUserNodeForURL(bookmark.url));
+  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;
@@ skipping 18 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