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

chrome/browser/ntp_snippets/ntp_bookmark_helper.h

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

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

What year is it?!
Also no "(c)"

[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.
+
+#ifndef CHROME_BROWSER_NTP_SNIPPETS_NTP_BOOKMARK_HELPER_H_
+#define CHROME_BROWSER_NTP_SNIPPETS_NTP_BOOKMARK_HELPER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace bookmarks {
+class BookmarkModel;
+}  // namespace bookmarks
+
+namespace content {
+class NavigationHandle;

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

nit: class WebContents

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

Done. Out of curiosity: Do I need to redeclare it even if the included header
(web_cont_observer) declares it?

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

Generally the rule is to include/forward-declare everything you use.

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

Thanks.

+}  // namespace content
+
+using bookmarks::BookmarkModel;

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

No using in headers please

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

Done.

+
+namespace ntp_snippets {

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

This shouldn't be in the ntp_snippets namespace; that's only used for the
component itself.

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

Done.

+
+class BookmarkNavigationObserver;
+
+// A bridge class between platform-specific content::WebContentsObserver and the
+// abstract interface ntp_snippets::BookmarkNavigationObserver.
+class NTPBookmarkHelper

[tschumann, 2016/07/29 10:18:43]

nit: I have an issue with any class or library having sufixes like 'helper' or
'utils'.
If it's purpose is to bridge between the two classes why not give it a
corresponding name? 'bridge' might be misleading as it's used in JNI context.
Let's discuss this offline.

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

Done.

+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<NTPBookmarkHelper> {
+ public:
+  ~NTPBookmarkHelper() override;
+
+  static void CreateForWebContentsWithBookmarkModel(

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

Is this a common pattern? What if there already is an instance for this
WebContents?

[blundell, 2016/07/29 09:59:23]

drive-by: Yes, this is the common pattern here. WebContentsUserData is different
from KeyedService in that WCUD is not created lazily on a call to the getter;
it's only created via an explicit call to CreateForWebContents(). In this CL,
that's being added to tab_helpers.cc (the correct location). 

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

Acknowledged.

+      content::WebContents* web_contents,
+      BookmarkModel* bookmark_model);
+
+ private:
+  explicit NTPBookmarkHelper(
+      content::WebContents* web_contents,
+      BookmarkModel* bookmark_model);
+  friend class content::WebContentsUserData<NTPBookmarkHelper>;
+
+  // Overridden from content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+
+  BookmarkModel* bookmark_model_;
+  std::unique_ptr<ntp_snippets::BookmarkNavigationObserver>
+      navigation_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(NTPBookmarkHelper);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // CHROME_BROWSER_NTP_SNIPPETS_NTP_BOOKMARK_HELPER_H_