Disable tracking of visits to bookmarks in incognito

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 868f2a027484bb7cddcde3beffd50cabbcd62698..83f88fd3d4aee90140f61e6d8041f8560a3fbe19 100644
--- a/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
+++ b/chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc
@@ -8,6 +8,7 @@
 #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/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
 
 namespace {
@@ -26,7 +27,7 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(BookmarkLastVisitUpdater);
 
 BookmarkLastVisitUpdater::~BookmarkLastVisitUpdater() {
   // In unit-tests on desktop, the bookmark_model is null.
-  if (!bookmark_model_) {
+  if (!bookmark_model_ || !tracking_enabled_) {
     return;

[Bernhard Bauer, 2016/12/12 15:06:37]

FWIW, using a base::ScopedObserver would let us avoid this check, but maybe we
can avoid creating this object completely (see below).

[jkrcal, 2016/12/14 13:40:44]

Done.

   }
   bookmark_model_->RemoveObserver(this);
@@ -46,8 +47,12 @@ BookmarkLastVisitUpdater::BookmarkLastVisitUpdater(
     : content::WebContentsObserver(web_contents),
       bookmark_model_(bookmark_model),
       web_contents_(web_contents) {
+  // Switch off tracking visits for off the record profiles.

[Bernhard Bauer, 2016/12/12 15:06:37]

Nit: the code here uses the term "off the record" because no one has renamed the
method, but outside of the most commonly used term is "incognito", so I think we
should use that as much as possible.

[jkrcal, 2016/12/14 13:40:44]

Done.

+  content::BrowserContext* browser_context = web_contents->GetBrowserContext();
+  tracking_enabled_ = !browser_context->IsOffTheRecord();
+
   // In unit-tests on desktop, the bookmark_model is null.
-  if (!bookmark_model_) {
+  if (!bookmark_model_ || !tracking_enabled_) {

[Bernhard Bauer, 2016/12/12 15:06:38]

So, what does this class actually do if tracking is disabled (or we don't have a
bookmark model)? Could we just skip creating it completely?

[jkrcal, 2016/12/14 13:40:44]

Done.

     return;
   }
   bookmark_model->AddObserver(this);
@@ -70,7 +75,7 @@ void BookmarkLastVisitUpdater::NewURLVisited(
     content::NavigationHandle* navigation_handle) {
   // In unit-tests on desktop, the bookmark_model is null.
   if (!navigation_handle->IsInMainFrame() || navigation_handle->IsErrorPage() ||
-      !bookmark_model_) {
+      !bookmark_model_ || !tracking_enabled_) {
     return;
   }