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

components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h

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

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

.

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_BOOKMARKS_BOOKMARK_LAST_VISIT_DATE_HELPER_H_
+#define COMPONENTS_NTP_SNIPPETS_BOOKMARKS_BOOKMARK_LAST_VISIT_DATE_HELPER_H_
+
+#include <memory>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
+
+using bookmarks::BookmarkModel;
+using bookmarks::BookmarkNode;

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

No using in headers (you can put them into the .cc though)

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

Done.

+
+namespace ntp_snippets {
+
+class BookmarkNavigationObserver {

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

Any reason for the separate observer interface? The code in chrome/browser needs
to know about the implementation anyway, so I think you could just remove this.

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

Agreed. Not much persuaded this adds any advantage, after all.

+ public:
+  virtual void OnURLVisitedInMainFrame(BookmarkModel* bookmark_model,
+                                       const GURL& url);

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

If you do keep the observer, then this should be pure virtual ("= 0").

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

Done.

+};
+
+// Per-tab class to report visits to bookmarked pages.
+class BookmarkLastVisitDateHelper

[tschumann, 2016/07/29 10:33:00]

so, I'd call this class something like LastVisitedBookmarks.

Actually, I like the separation with the other interface, as it simplifies
naming a lot (nicely separates the two roles the class is playing).

You could then rename the other helper to BookmarkNavigationObserverAdapter or
the like, as it's an adapter for this class to work with Android or iOS.

WDYT?

[Marc Treib, 2016/07/29 10:42:22]

I think there's no reason for a class at all, since it doesn't have any
non-static members (the one non-static method might as well be static). So I'd
prefer calling this what it is: A bunch of helper methods.

[tschumann, 2016/07/29 12:38:56]

right, if we don't need a class, then a function will do as well.
But nevertheless, the functions have a pretty clear purpose and the file/library
names should reflect that.
With these generic helpers and utils libraries it's really hard to anticipate
what functionality is in a given library. I've too often witnessed functionality
of such libraries being hard to find or duplicated in other libraries (because
they weren't found).
They have a tendency to grow into a hodgepodge of vaguely related functionality.

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

Done. I still call the component code "_util" to make clear these are global
functions.
I believe the whole name is specific enough to keep it clear and focused.

+    : public BookmarkNavigationObserver {
+ public:
+  BookmarkLastVisitDateHelper() = default;
+  ~BookmarkLastVisitDateHelper() = default;
+
+  void OnURLVisitedInMainFrame(BookmarkModel* bookmark_model,

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

The bookmark model will always be the same, right? Might as well pass it into
the ctor then.
Or alternatively, right now this class doesn't actually do anything. Might as
well get rid of it and just have some global functions instead.

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

Done global functions.

+                               const GURL& url) override;
+
+  static base::Time GetLastVisitDate(const BookmarkNode* node);
+
+  static void GetRecentlyVisitedBookmarks(
+      BookmarkModel* bookmark_model,
+      std::vector<const BookmarkNode*>* bookmarks,

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

Just return the vector? (Thanks to move semantics, that won't be expensive)
If you do keep the output param, then the order is: first inputs, then outputs.

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

Done.

+      int max_count,
+      const base::Time& min_time);

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

min_visit_time?

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

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BookmarkLastVisitDateHelper);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_BOOKMARKS_BOOKMARK_LAST_VISIT_DATE_HELPER_H_