Fix clicks on NTP tiles not contributing to Most Visited tiles

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index cce6055d14114802396ae3cbd025d6e14eba2cb0..5e0239060d27315eecc46319a47921855dab3f47 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -1154,6 +1154,26 @@ bool ChromeContentBrowserClient::ShouldAllowOpenURL(
   return true;
 }
 
+void ChromeContentBrowserClient::OverrideOpenURLParams(
+    const content::SiteInstance* site_instance,
+    content::OpenURLParams* params) {
+  DCHECK(params);
+  if (site_instance &&
+      site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) &&

[Charlie Reis, 2016/06/28 00:52:05]

There must be an existing way to check whether a tab is showing the NTP.  (I
don't think we should have subtle checks like this sprinkled around the
codebase, in case they have bugs.)

At first glance, I found search::IsInstantNTP.  Would that or something similar
work?

[mastiz, 2016/06/28 08:20:31]

Unfortunately there is no such predicate in C++ code, which is easy to prove by
searching for kChromeSearchRemoteNtpHost (only one relevant occurrence,
ShouldUseProcessPerSiteForInstantURL, not reusable).

Some cleanup is needed in this space but it's also non-trivial since exact
requirements for the predicate might differ: in this particular codepath, we
don't care about false positives, and it's also the case that URLs get overriden
throughout the code.

I would rather not address this cleanup now. WDYT?

[Marc Treib, 2016/06/28 08:57:07]

Let me add my +1 here. You would indeed think that there's a canonical way to
check "is this an NTP", but there isn't, due to how convoluted the whole thing
is. E.g.: The initial URL is "chrome://newtab". That gets rewritten to something
like "https://www.google.com/_/chrome/newtab", and then at some later point gets
rewritten again to "chrome-search://remote-ntp". Then there's the local NTP
which is different, the incognito NTP which stays at "chrome://newtab", etc etc.

So, this whole mess definitely needs some serious cleanup, but this CL is not
the place.

[Charlie Reis, 2016/06/28 17:57:35]

Yes, it's unfortunate we don't have something we can use here.

That said, the fact that it's so convoluted makes it even more important to have
a canonical way to do it, rather than having lots of different places that all
get it wrong in some way.  Also, I strongly disagree that we don't care about
false positives here, because there are security implications to treating a
navigation as browser-initiated when it's actually renderer-initiated.

That said, if we can be sure that we don't have false positives in this check,
I'm ok with proceeding if you file a bug for abstracting this out and leave a
TODO referencing the bug number.  I would also highly recommend prioritizing
that bug soon.

[mastiz, 2016/06/29 14:20:14]

Added a TODO as suggested.

Honestly, I don't think this is feasible without a major cleanup much beyond
these predicates (i.e. avoid different codepaths for various versions of the
NTP). The problem is not that the predicates themselves are convoluted, but all
the code around.

[Charlie Reis, 2016/06/29 19:27:00]

I posted a comment to the bug, since I think this is worth solving to prevent
future problems.

+      (site_instance->GetSiteURL().host() ==
+           chrome::kChromeSearchRemoteNtpHost ||
+       site_instance->GetSiteURL().host() ==
+           chrome::kChromeSearchLocalNtpHost) &&
+      ui::PageTransitionCoreTypeIs(params->transition,
+                                   ui::PAGE_TRANSITION_LINK)) {
+    // Use AUTO_BOOKMARK for clicks on tiles of the new tab page, consistently
+    // with native implementations like Android's.
+    params->transition = ui::PAGE_TRANSITION_AUTO_BOOKMARK;
+    params->is_renderer_initiated = false;
+    params->referrer = content::Referrer();
+  }
+}
+
 bool ChromeContentBrowserClient::IsSuitableHost(
     content::RenderProcessHost* process_host,
     const GURL& site_url) {