Marking last_visited date for bookmarks created from an open tab

chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc

Index: chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
diff --git a/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc b/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
index 001e8e2073d47641aff0adf86872a0bf2f2085f2..995255a44415c782e2389165579b47496e8b77af 100644
--- a/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
+++ b/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
@@ -4,13 +4,13 @@
 
 #include "chrome/browser/ntp_snippets/bookmark_last_visit_updater.h"
 
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 #include "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 #include "content/public/browser/navigation_handle.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(BookmarkLastVisitUpdater);
 
-BookmarkLastVisitUpdater::~BookmarkLastVisitUpdater() {}
-
 // static
 void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel(
     content::WebContents* web_contents,
@@ -19,16 +19,49 @@ void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel(
                                                web_contents, bookmark_model));
 }
 
+BookmarkLastVisitUpdater::~BookmarkLastVisitUpdater() {

[Marc Treib, 2016/08/11 13:03:43]

Why move this? (The ordering is kinda weird anyway, but at least before it was
consistent with the header)

[jkrcal, 2016/08/11 13:59:26]

Done. I've forgotten about the header. Looking at the header now, I realized the
order makes sense (as the constructor is private) so I reverted this change.

+  bookmark_model_->RemoveObserver(this);
+}
+
 BookmarkLastVisitUpdater::BookmarkLastVisitUpdater(
     content::WebContents* web_contents,
     bookmarks::BookmarkModel* bookmark_model)
     : content::WebContentsObserver(web_contents),
-      bookmark_model_(bookmark_model) {}
+      bookmark_model_(bookmark_model),
+      web_contents_(web_contents) {
+  bookmark_model->AddObserver(this);
+}
 
 void BookmarkLastVisitUpdater::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
+  // Mark visited bookmark when the navigation starts (may end somewhere else
+  // due to server-side redirects).
+  NewURLVisited(navigation_handle);
+}
+
+void BookmarkLastVisitUpdater::DidRedirectNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // Mark visited bookmark also after each redirect.
+  NewURLVisited(navigation_handle);
+}
+
+void BookmarkLastVisitUpdater::NewURLVisited(
+    content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() || navigation_handle->IsErrorPage())
     return;
+
   ntp_snippets::UpdateBookmarkOnURLVisitedInMainFrame(
       bookmark_model_, navigation_handle->GetURL());
 }
+
+void BookmarkLastVisitUpdater::BookmarkNodeAdded(
+    bookmarks::BookmarkModel* model,
+    const bookmarks::BookmarkNode* parent,
+    int index) {
+  const GURL& new_bookmark_url = parent->GetChild(index)->url();
+
+  if (new_bookmark_url == web_contents_->GetLastCommittedURL()) {

[Marc Treib, 2016/08/11 13:03:43]

What's this check for?

[jkrcal, 2016/08/11 13:59:26]

Added a comment. Is it clearer now?
(the check is the reason why it is in a tab helper in the first place)

[Marc Treib, 2016/08/11 14:06:23]

Yup, better, thanks!

+    ntp_snippets::UpdateBookmarkOnURLVisitedInMainFrame(
+        bookmark_model_, new_bookmark_url);
+  }
+}