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

chrome/browser/ui/tab_helpers.cc

Index: chrome/browser/ui/tab_helpers.cc
diff --git a/chrome/browser/ui/tab_helpers.cc b/chrome/browser/ui/tab_helpers.cc
index 85c3e1d5118b996cde98f1f0002cd640c582e82a..a73aafa6b7bce6edf751563ad17275f449eed486 100644
--- a/chrome/browser/ui/tab_helpers.cc
+++ b/chrome/browser/ui/tab_helpers.cc
@@ -78,6 +78,11 @@
 #include "components/zoom/zoom_controller.h"
 #endif  // BUILDFLAG(ANDROID_JAVA_UI)
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"

[Avi (use Gerrit), 2016/08/01 17:01:41]

Why is this different than the ANDROID_JAVA_UI block above here and therefore in
need of its own header block?

[Philipp Keck, 2016/08/02 13:05:33]

The only documentation I found on these flags is this:
https://codesearch.chromium.org/chromium/src/chrome/common/features.gni?q=and...

So, if I understood that correctly, ANDROID_JAVA_UI is the subset of those
OS_ANDROID where we don't only compile for the Android platform, but also use
the whole Java UI stuff listed there.

Most things in the header block above seem to be pretty UI specific ("zoom",
"banners", and everything from c/b/ui). The AppBannerManager, for instance, is
only compiled if ANDROID_JAVA_UI is set
(https://codesearch.chromium.org/chromium/src/chrome/chrome_browser.gypi?q=app...).
So probably that's the reason why they are in that section.

The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its
dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI. So if we
use ANDROID_JAVA_UI here, we're missing the cases where OS_ANDROID &&
!ANDROID_JAVA_UI.

According to bauerb@, those cases might not exist, because the two are always
the same (the gni file linked above also indicates that). If, in the future,
ANDROID_JAVA_UI is removed completely, the two header blocks can become one
again. Probably the right direction would be to use OS_ANDROID throughout this
file, but I don't want to do these changes on this CL.

If you prefer, we can use ANDROID_JAVA_UI (below) instead of OS_ANDROID in this
file for consistency, even though it seems to be the deprecated one.

[Ted C, 2016/08/03 01:11:40]

Does this code work with the webui based NTP or just the java based android one?

If the latter, then it seems pretty straight forward to follow the conventions
of
ANDROID_JAVA_UI.

You are correct that the current state of the world is somewhat confusing.
ANDROID_JAVA_UI was introduced with the hope that we could sever the pretty
strong dependencies we have in the code that on Android we are using the
java based UI.  OS_ANDROID would remain for things that apply on Android
regardless of the UI built on top.

While this might conceptually fall into the latter, I'd be very strongly tempted
to leave it in ANDROID_JAVA_UI in this class for consistency (especially if
the logic only applies to the java NTP).

I'll try to figure out from sievers@ (the introducer of ANDROID_JAVA_UI) what
the goals are so we can avoid this going forward, but I'd vote (for this change)
that we err on the side of consistency.

+#include "chrome/browser/ntp_snippets/bookmark_last_visit_updater.h"
+#endif  // defined(OS_ANDROID)
+
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
 #include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 #endif
@@ -218,6 +223,13 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
     banners::AppBannerManagerDesktop::CreateForWebContents(web_contents);
 #endif
 
+#if defined(OS_ANDROID)
+  BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel(
+      web_contents,
+      BookmarkModelFactory::GetForProfile(
+          Profile::FromBrowserContext(web_contents->GetBrowserContext())));

[Avi (use Gerrit), 2016/08/01 17:01:41]

You should totally fix BookmarkModelFactory::GetForProfile to take a
BrowserContext* instead so you don't need the FromBrowserContext here.

Also, again, how is this not ANDROID_JAVA_UI?

[Philipp Keck, 2016/08/02 13:05:33]

Added BookmarkModelFactory::GetForBrowserContext(content::BrowserContext*)
instead, seems to be the usual naming convention. I couldn't find any
GetForProfile(content::BrowserContext*) in the codebase.
Theoretically, all places that call GetForProfile could now call
GetForBrowserContext instead and we could remove GetForProfile, but that would
need to go on a separate CL.
See above.

+#endif  // defined(OS_ANDROID)
+
   // --- Feature tab helpers behind flags ---
 
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)