Redoing Issue 36073011: Allowing file:/// in Instant Extended's Most Visited links.

Redoing Issue 36073011: Allowing file:/// in Instant Extended's Most Visited links.

The previous change was rolled back because it prevented the NTP ping, which was needed for UMA. 3 main parts of this CL are:

(1) Redo Issue 36073011, adjusted to code changes. This fixes the file:/// issue, and changes NTP outgoing page transition to AUTO_BOOKMARK (instead of LINK).
(2) Adding logNavigation(pos, provider), which is called on click.
(3) Deprecate ping and chrome://log.html pages.

Caveat: keyboard navigation is not properly handled, since it involves the NTP rather than the iframe.  This will be fixed in a follow-up CL.

For (1):
- Unlike Issue 36073011, we're not enabling LocalNTPTest.LocalNTPJavascriptTest, because recent cleanups causes more failure modes of this test.
- Using IDR_MOST_VISITED_WINDOW_DISPOSITION_UTIL_JS instead of IDR_CLICK_UTIL_JS.
- In local_ntp.js: init(): skipping change to the "recentTabsText" block, since it got deleted.

BUG=306137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257389

[huangs, 2014-03-04 20:00:03]

Redoing the issue. First upload is CL 36073011 adapted to new codebase.  Second
upload is the new stuff (with one synch).  PTAL.

[beaudoin, 2014-03-04 21:33:15]

LGTM with some nits and one security question that maybe someone can answer
better than me.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:14: // Only allow string of 8
alphanumeric characters or less as providers.
// The empty string is considered valid and should be treated as if no
// provider were specified.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:73: std::string
NTPUserDataLogger::GetHistogramNameForProvider(
GetNavigationHistogramNameForProvider(

Maybe add GetImpressionHistogramNameForProvider?

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:168: counter->Add(position);
I'd be tempted to use the exact same pattern as below here:
Always log on NewTabPage.SuggestionsImpression and log on
NewTabPage.SuggestionsImpression.%s if a provider is specified.
(We have reserved NewTabPage.SuggestionsImpression but nobody ever logs on it.
:\)

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:173: // Log the Most Visited
navigation.
for both navigations that have providers and those that dont.

https://codereview.chromium.org/178253008/diff/20001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/178253008/diff/20001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox_extension.cc:1033:
!destination_url.SchemeIs(content::kJavaScriptScheme)) {
Any other scheme we should prevent against? I'd defer to a security expert here.

[huangs, 2014-03-04 22:10:43]

Updated.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:14: // Only allow string of 8
alphanumeric characters or less as providers.
On 2014/03/04 21:33:15, beaudoin wrote:
> // The empty string is considered valid and should be treated as if no
> // provider were specified.

Done.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:73: std::string
NTPUserDataLogger::GetHistogramNameForProvider(
Done; added GetImpressionHistogramNameForProvider() and using it below.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:168: counter->Add(position);
Done; added kImpressionHistogramName.

Note that the routines are not identical: this one does not have 

content::RecordAction(base::UserMetricsAction(...));

at bottom.

https://codereview.chromium.org/178253008/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:173: // Log the Most Visited
navigation.
On 2014/03/04 21:33:15, beaudoin wrote:
> for both navigations that have providers and those that dont.

Done.

[huangs, 2014-03-06 20:47:16]

OWNER review for samarth@ for:

  chrome\browser\resources\local_ntp\*
  chrome\browser\search\*
  chrome\browser\ui\search\*
  chrome\renderer\resources\extensions\searchbox_api.js
  chrome\renderer\searchbox\*

PTAL.

[samarth, 2014-03-06 21:01:21]

Overall looks good, thanks for doing this!

https://codereview.chromium.org/178253008/diff/40001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/178253008/diff/40001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox.cc:170: void SearchBox::LogNavigation(int
position, const base::string16& provider) {
I think LogMostVisitedClick is clearer here (and in SearchIPCRoucter, etc).

Can you also rename LogImpression to LogMostVistedImpression to match while
you're at it?

[huangs, 2014-03-06 21:35:02]

I chose "Navigation" over "Click" because we the routine should also handle
keyboard (right-click, escape, optional tabs, enter). Can I go with
LogMostVisitedNavigation()?

[huangs, 2014-03-07 00:32:49]

Updated, PTAL.

[huangs, 2014-03-07 00:33:40]

https://chromiumcodereview.appspot.com/178253008/diff/40001/chrome/renderer/s...
File chrome/renderer/searchbox/searchbox.cc (right):

https://chromiumcodereview.appspot.com/178253008/diff/40001/chrome/renderer/s...
chrome/renderer/searchbox/searchbox.cc:170: void SearchBox::LogNavigation(int
position, const base::string16& provider) {
Done, but using LogMostVisitedNavigation() (will change if you insist).

[samarth, 2014-03-07 03:22:53]

lgtm

No you're right: LogMostVisitedNavigation is better.

Thanks!
Samarth

[huangs, 2014-03-07 03:59:52]

OWNER review to:

jhawkins@ for:
  chrome\browser\browser_resources.grd
  chrome\browser\resources\local_ntp\*
  chrome\browser\search\most_visited_iframe_source*.*
  chrome\browser\ui\search\*
  chrome\browser\ui\webui\ntp\ntp_user_data_logger*.*
  chrome\renderer\resources\extensions\searchbox_api.js
  chrome\renderer\searchbox\*

dcheng@ for:
  chrome\common\render_messages.h

PTAL. Thanks!

[James Hawkins, 2014-03-07 22:11:32]

Can those 3 pieces of this CL be landed separately?  This CL is really large.

[huangs, 2014-03-07 22:33:40]

This is an "end-to-end plumbing" CL with:
- JS frontend,
- Routines passing data, with very repetitive names.
- The final destination (ntp_user_data_logger*.*) that writes data to UMA.

We also took the opportunity to do some renaming.  Breaking this CL will require
us to cut open pipes and add adaptors.  I think it's less work to have this in 1
CL.  Please reconsider? Thanks!

[huangs, 2014-03-12 15:06:27]

Ping samarth@ and dcheng@.

[huangs, 2014-03-14 17:51:26]

Ping jhawkins@.

OWNER review to tsepez@ for 
chrome\common\render_messages.h

[James Hawkins, 2014-03-14 17:53:51]

On 2014/03/07 03:59:52, huangs1 wrote:
> OWNER review to:
> 
> jhawkins@ for:
>   chrome\browser\browser_resources.grd
>   chrome\browser\resources\local_ntp\*
>   chrome\browser\search\most_visited_iframe_source*.*
>   chrome\browser\ui\search\*
>   chrome\browser\ui\webui\ntp\ntp_user_data_logger*.*
>   chrome\renderer\resources\extensions\searchbox_api.js
>   chrome\renderer\searchbox\*
> 

I'm not in the OWNERS files for most of these directories.

> dcheng@ for:
>   chrome\common\render_messages.h
> 
> PTAL. Thanks!

[huangs, 2014-03-14 17:59:13]

Oh sorry. Was following "git cl owners", but should double check!

[huangs, 2014-03-14 18:06:28]

Ping thakis@ for OWNER review.

[Tom Sepez, 2014-03-14 18:13:01]

Messages LGTM.

[Nico, 2014-03-14 18:27:14]

On 2014/03/14 18:06:28, huangs1 wrote:
> Ping thakis@ for OWNER review.

Which files? Everything?

[huangs, 2014-03-14 20:17:25]

Specifically:

chrome\browser\browser_resources.grd
chrome\browser\resources\local_ntp\*
chrome\browser\search\*
chrome\browser\ui\search\*
chrome\browser\ui\webui\ntp\*
chrome\renderer\resources\extensions\searchbox_api.js
chrome\renderer\searchbox\*

[Nico, 2014-03-14 22:14:52]

lgtm

[huangs, 2014-03-15 00:05:25]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-15 00:05:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/100001

[commit-bot: I haz the power, 2014-03-15 00:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-15 00:06:25]

Failed to apply patch for
chrome/browser/resources/local_ntp/most_visited_util.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/resources/local_ntp/most_visited_util.js
  Hunk #6 FAILED at 198.
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/resources/local_ntp/most_visited_util.js.rej

Patch:       chrome/browser/resources/local_ntp/most_visited_util.js
Index: chrome/browser/resources/local_ntp/most_visited_util.js
diff --git a/chrome/browser/resources/local_ntp/most_visited_util.js
b/chrome/browser/resources/local_ntp/most_visited_util.js
index
d6aa2642f0bf226bcf0188edbe318b9db5c539d9..92bbcad63e721162d31f288448d0c792f266dd4b
100644
--- a/chrome/browser/resources/local_ntp/most_visited_util.js
+++ b/chrome/browser/resources/local_ntp/most_visited_util.js
@@ -8,6 +8,8 @@
  */
 
 <include src="instant_iframe_validation.js">
+<include src="window_disposition_util.js">
+
 
 /**
  * The different types of events that are logged from the NTP.  This enum is
@@ -43,6 +45,7 @@ var NTP_LOGGING_EVENT_TYPE = {
   NTP_MOUSEOVER: 9
 };
 
+
 /**
  * Type of the impression provider for a generic client-provided suggestion.
  * @type {string}
@@ -88,36 +91,20 @@ function parseQueryParams(location) {
  * @param {string} href The destination for the link.
  * @param {string} title The title for the link.
  * @param {string|undefined} text The text for the link or none.
- * @param {string|undefined} ping If specified, a location relative to the
- *     referrer of this iframe, to ping when the link is clicked. Only works if
- *     the referrer is HTTPS.
  * @param {string|undefined} provider A provider name (max 8 alphanumeric
  *     characters) used for logging. Undefined if suggestion is not coming from
  *     the server.
  * @return {HTMLAnchorElement} A new link element.
  */
-function createMostVisitedLink(params, href, title, text, ping, provider) {
+function createMostVisitedLink(params, href, title, text, provider) {
   var styles = getMostVisitedStyles(params, !!text);
   var link = document.createElement('a');
   link.style.color = styles.color;
   link.style.fontSize = styles.fontSize + 'px';
   if (styles.fontFamily)
     link.style.fontFamily = styles.fontFamily;
+
   link.href = href;
-  if ('pos' in params && isFinite(params.pos)) {
-    link.ping = '/log.html?pos=' + params.pos;
-    if (provider)
-      link.ping += '&pr=' + provider;
-    // If a ping parameter was specified, add it to the list of pings, relative
-    // to the referrer of this iframe, which is the default search provider.
-    if (ping) {
-      var parentUrl = document.createElement('a');
-      parentUrl.href = document.referrer;
-      if (parentUrl.protocol == 'https:') {
-        link.ping += ' ' + parentUrl.origin + '/' + ping;
-      }
-    }
-  }
   link.title = title;
   link.target = '_top';
   // Exclude links from the tab order.  The tabIndex is added to the thumbnail
@@ -129,6 +116,20 @@ function createMostVisitedLink(params, href, title, text,
ping, provider) {
     var ntpApiHandle = chrome.embeddedSearch.newTabPage;
     ntpApiHandle.logEvent(NTP_LOGGING_EVENT_TYPE.NTP_MOUSEOVER);
   });
+
+  // Webkit's security policy prevents some Most Visited thumbnails from
+  // working (those with schemes different from http and https). Therefore,
+  // navigateContentWindow is being used in order to get all schemes working.
+  link.addEventListener('click', function handleNavigation(e) {
+    e.preventDefault();
+    var ntpApiHandle = chrome.embeddedSearch.newTabPage;
+    if ('pos' in params && isFinite(params.pos)) {
+      ntpApiHandle.logMostVisitedNavigation(parseInt(params.pos, 10),
+                                            provider || '');
+    }
+    ntpApiHandle.navigateContentWindow(href, getDispositionFromEvent(e));
+  });
+
   return link;
 }
 
@@ -186,7 +187,6 @@ function fillMostVisited(location, fill) {
     data.title = params.ti || '';
     data.direction = params.di || '';
     data.domain = params.dom || '';
-    data.ping = params.ping || '';
     data.provider = params.pr || SERVER_PROVIDER_NAME;
 
     // Log the fact that suggestion was obtained from the server.
@@ -198,7 +198,6 @@ function fillMostVisited(location, fill) {
     if (!data)
       return;
     data.provider = CLIENT_PROVIDER_NAME;
-    delete data.ping;
   }
   if (/^javascript:/i.test(data.url) ||
       /^javascript:/i.test(data.thumbnailUrl) ||

[huangs, 2014-03-15 20:14:04]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-15 20:14:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/120001

[commit-bot: I haz the power, 2014-03-15 20:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-15 20:45:42]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[huangs, 2014-03-16 16:45:55]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-16 16:46:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/130026

[commit-bot: I haz the power, 2014-03-16 18:09:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-16 18:09:17]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[huangs, 2014-03-16 18:36:01]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-16 18:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/130026

[commit-bot: I haz the power, 2014-03-16 18:46:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-16 18:46:01]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[huangs, 2014-03-16 20:41:34]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-16 20:41:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/130026

[huangs, 2014-03-17 05:15:11]

The CQ bit was unchecked by huangs@chromium.org

[huangs, 2014-03-17 05:15:32]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-03-17 05:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/178253008/130026

[commit-bot: I haz the power, 2014-03-17 06:48:20]

Change committed as 257389

[Lei Zhang, 2015-09-24 02:07:14]

https://codereview.chromium.org/178253008/diff/130026/chrome/browser/search/m...
File chrome/browser/search/most_visited_iframe_source_unittest.cc (left):

https://codereview.chromium.org/178253008/diff/130026/chrome/browser/search/m...
chrome/browser/search/most_visited_iframe_source_unittest.cc:12: class
MostVisitedIframeSourceTest : public testing::Test {
BTW, you removed all the tests, but not the test class. (I'm removing this now)