Fix clicks on NTP tiles not contributing to Most Visited tiles

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 687 matching lines @@
   params.uses_post = uses_post;
   params.post_data = body;
   params.source_site_instance = source_site_instance;
   if (redirect_chain.size() > 0)
     params.redirect_chain = redirect_chain;
   params.should_replace_current_entry = should_replace_current_entry;
   params.user_gesture = user_gesture;
 
   if (render_frame_host->web_ui()) {
     // Web UI pages sometimes want to override the page transition type for
     // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for

[Charlie Reis, 2016/06/23 22:48:15]

Looks like we already used to do this here, back when the NTP was a WebUI page.

We can't really make the NTP WebUI, but maybe we can abstract this part to
handle both WebUI and something from the ContentBrowserClient?  We could have a
function that returns an appropriate PageTransition override, for example. 
(That would let ChromeContentBrowserClient decide if it's an NTP or not, without
content/ having to care.)

Would we also want clear the referrer and treat it as a browser-initiated
navigation?

[Marc Treib, 2016/06/24 10:21:41]

Yup! Ghosts of NTPs past...
ContentBrowserClient looks like a good place for this logic. There's already
"similar" methods there, like GetEffectiveURL, IsHandledURL etc.
That seems to be what happened before, so...probably?

For reference, the old code was:

chrome::NavigateParams params(this, url,	
                              ui::PAGE_TRANSITION_AUTO_BOOKMARK);	
params.referrer = content::Referrer();	
params.source_contents = source_contents;	
params.disposition = disposition;	
params.is_renderer_initiated = false;	
params.initiating_profile = profile_;	
chrome::Navigate(&params);

     // automatically generated suggestions).  We don't override other types
     // like TYPED because they have different implications (e.g., autocomplete).
     if (ui::PageTransitionCoreTypeIs(
         params.transition, ui::PAGE_TRANSITION_LINK))
       params.transition = render_frame_host->web_ui()->GetLinkTransitionType();
 
     // Note also that we hide the referrer for Web UI pages. We don't really
     // want web sites to see a referrer of "chrome://blah" (and some
     // chrome: URLs might have search terms or other stuff we don't want to
     // send to the site), so we send no referrer.
     params.referrer = Referrer();
 
     // Navigations in Web UI pages count as browser-initiated navigations.
     params.is_renderer_initiated = false;
+  } else if (source_site_instance &&
+             source_site_instance->GetSiteURL().spec() ==
+                 "chrome-search://remote-ntp/" &&

[Marc Treib, 2016/06/22 12:36:34]

Oh man, we have too many NTP URLs. I didn't even know about this one. From the
top of my head:
- chrome://newtab (the "initial" newtab URL).
- Unless you're in an incognito or guest profile, it gets rewritten to either
  - chrome-search://local-ntp/local-ntp.html, or
  - something like https://www.google.com/_/chrome/newtab (assuming Google is
the DSE)
- Apparently, at some point that gets rewritten again to this
chrome-search://remote-ntp one.
- chrome-native://newtab (on Android).

There are various "is this an NTP URL" functions around, but I'm not sure any of
them will work at this particular point in time. At the least, we should:
- Use the existing constants (e.g.
https://cs.chromium.org/chromium/src/chrome/common/url_constants.cc?rcl=0&l=742)
instead of hardcoding strings.
- Not compare full URL strings directly, but GURL.host() and GURL.scheme().
- Also handle the local NTP.

+             ui::PageTransitionCoreTypeIs(params.transition,
+                                          ui::PAGE_TRANSITION_LINK)) {
+    // Make any click on the new tab page a AUTO_BOOKMARK, consistently with
+    // native implementations like Android's.
+    params.transition = ui::PAGE_TRANSITION_AUTO_BOOKMARK;
   }
 
   if (delegate_)
     delegate_->RequestOpenURL(render_frame_host, params);
 }
 
 void NavigatorImpl::RequestTransferURL(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url,
     SiteInstance* source_site_instance,
@@ skipping 446 matching lines @@
   if (pending_entry != controller_->GetVisibleEntry() ||
       !should_preserve_entry) {
     controller_->DiscardPendingEntry(true);
 
     // Also force the UI to refresh.
     controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content