Add a tab helper to record the last visit date for each bookmark.

components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Marc Treib, 2016/07/29 09:54:57]

.

[jkrcal, 2016/07/29 12:42:41]

Done.

+// 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_date_helper.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/strings/string_number_conversions.h"
+
+namespace {
+const char kBookmarkLastVisitDate[] = "last_visited";

[Marc Treib, 2016/07/29 09:54:57]

Add "Key" or something like that?

[Marc Treib, 2016/07/29 09:54:57]

nit: empty line before

[jkrcal, 2016/07/29 12:42:41]

Done (both).

+
+base::Time ParseLastVisitDate(const std::string& date_string) {
+  int64_t date;

[Marc Treib, 2016/07/29 09:54:57]

Initialize this? (I think the Win compiler will complain if you don't)

[jkrcal, 2016/07/29 12:42:41]

Done.

+  if (!base::StringToInt64(date_string, &date))
+    return base::Time::UnixEpoch();

[Marc Treib, 2016/07/29 09:54:57]

just base::Time() ?

[jkrcal, 2016/07/29 12:42:41]

Result of base::Time() is platform-dependent (can use different epochs).
Probably not important as on any platform, this will be long in the past but I
like UnixEpoch() for being more predictable.

[Marc Treib, 2016/07/29 14:36:42]

The advantage would be that you could check with .is_null() whether it's a
proper time. But I guess that doesn't matter here, so okay.

[jkrcal, 2016/07/29 15:34:06]

I see. No, it does not matter here.

+  return base::Time::FromInternalValue(date);
+}
+
+std::string FormatLastVisitDate(const base::Time& date) {
+  return base::Int64ToString(date.ToInternalValue());
+}
+
+base::Time GetLastVisitDateImpl(const bookmarks::BookmarkNode* node) {
+  if (node == nullptr)

[Marc Treib, 2016/07/29 09:54:57]

if (!node)

[jkrcal, 2016/07/29 12:42:41]

Done.

+    return base::Time::UnixEpoch();

[Marc Treib, 2016/07/29 09:54:57]

Here too, just base::Time() ?

[jkrcal, 2016/07/29 12:42:41]

Same as above.

+
+  std::string last_visit_date_string;
+  node->GetMetaInfo(kBookmarkLastVisitDate, &last_visit_date_string);
+
+  if (last_visit_date_string.empty()) {
+    // Use creation date if no last visit info present.
+    return node->date_added();
+  }
+
+  return ParseLastVisitDate(last_visit_date_string);
+}
+
+int CompareBookmarksByLastVisitDate(const bookmarks::BookmarkNode* a,
+                                    const bookmarks::BookmarkNode* b) {
+  return (GetLastVisitDateImpl(a) - GetLastVisitDateImpl(b)).InMilliseconds() >

[Marc Treib, 2016/07/29 09:54:57]

return GetLastVisitDateImpl(a) > GetLastVisitDateImpl(b);

[jkrcal, 2016/07/29 12:42:41]

error: cannot convert 'base::TimeDelta' to 'int' in return.
The return value should be actually bool, I fixed it. Does not change anything,
however.

[Marc Treib, 2016/07/29 14:36:42]

">", not "-".

[jkrcal, 2016/07/29 15:34:06]

Done.

+         0;
+}
+
+}  // namespace
+
+namespace ntp_snippets {
+
+void BookmarkLastVisitDateHelper::OnURLVisitedInMainFrame(
+    BookmarkModel* bookmark_model,
+    const GURL& url) {
+

[Marc Treib, 2016/07/29 09:54:57]

nit: no empty line

[jkrcal, 2016/07/29 12:42:41]

Done.

+  const bookmarks::BookmarkNode* node =
+      bookmark_model->GetMostRecentlyAddedUserNodeForURL(url);
+  if (node == nullptr)

[Marc Treib, 2016/07/29 09:54:57]

if (!node)

[jkrcal, 2016/07/29 12:42:41]

Done.

+    return;
+
+  // If there exists a bookmark for |url|, set its last visit date to now.
+  bookmark_model->SetNodeMetaInfo(node, kBookmarkLastVisitDate,
+                                  FormatLastVisitDate(base::Time::Now()));
+}
+
+// static
+base::Time BookmarkLastVisitDateHelper::GetLastVisitDate(
+    const bookmarks::BookmarkNode* node) {
+  return GetLastVisitDateImpl(node);

[Marc Treib, 2016/07/29 09:54:57]

Just put the code here and get rid of the Impl?

[jkrcal, 2016/07/29 12:42:41]

Done.

+}
+
+// static
+void BookmarkLastVisitDateHelper::GetRecentlyVisitedBookmarks(
+      BookmarkModel* bookmark_model,
+      std::vector<const bookmarks::BookmarkNode*>* bookmarks,
+      int max_count,
+      const base::Time& min_time) {
+  // Get all the bookmarks.
+  std::vector<BookmarkModel::URLAndTitle> all_bookmarks;
+  bookmark_model->GetBookmarks(&all_bookmarks);
+  // Remove bookmarks that have not been visited after |min_time|.
+  all_bookmarks.erase(
+      std::remove_if(
+          all_bookmarks.begin(), all_bookmarks.end(),
+          [&](const BookmarkModel::URLAndTitle& bookmark) {

[Marc Treib, 2016/07/29 09:54:57]

Don't use default captures; capture only what you actually need.

[jkrcal, 2016/07/29 12:42:41]

Done.

+            return GetLastVisitDateImpl(
+                       bookmark_model->GetMostRecentlyAddedUserNodeForURL(

[Marc Treib, 2016/07/29 09:54:57]

Can this return null here?
Also I'm not sure what to make of the "MostRecentlyAdded". Can there be multiple
BookmarkNodes for a URL? Are we sure the most recent one is the correct one?

[jkrcal, 2016/07/29 12:42:41]

It can return null but GetLastVisitDate() handles it.

As regards MostRecentlyAdded: good point. This is consistent with what I do in
OnURLVisitedInMainFrame. Only the MostRecentlyAdded has the last_visit meta
data.

If we need to distinguish which of the equivalent bookmarks is actually visited,
I would need to observe at other places as well (where I would have more
information than the URL). I suggest to keep it simple for now, adding a TODO to
investigate further (after M54).

[Marc Treib, 2016/07/29 14:36:42]

My point is mostly: Could the "MostRecentlyAdded" have changed since we stored
the metadata? Then we'd effectively lose the visit date.

[jkrcal, 2016/07/29 15:34:06]

Correct, thanks! I changed it to consider the most recent visit date over all
bookmarks with the same URL.

+                           bookmark.url)) < min_time;
+          }),
+      all_bookmarks.end());
+  // Sort the remaining bookmarks by date.
+  std::sort(all_bookmarks.begin(), all_bookmarks.end(),
+            [&](const BookmarkModel::URLAndTitle& a,
+                const BookmarkModel::URLAndTitle& b) {
+              return CompareBookmarksByLastVisitDate(
+                  bookmark_model->GetMostRecentlyAddedUserNodeForURL(a.url),
+                  bookmark_model->GetMostRecentlyAddedUserNodeForURL(b.url));
+            });
+  // Insert the first |max_count| into |bookmarks|.
+  int count = 0;
+  for (const BookmarkModel::URLAndTitle& bookmark : all_bookmarks) {
+    bookmarks->push_back(
+        bookmark_model->GetMostRecentlyAddedUserNodeForURL(bookmark.url));
+    count++;
+    if (count > max_count)

[Marc Treib, 2016/07/29 09:54:57]

Just check bookmarks.size() and get rid of count?

[jkrcal, 2016/07/29 12:42:41]

Done.

+      break;
+  }
+}
+
+}  // namespace ntp_snippets