More fine grained understanding of Omnibox navigation counts.

chrome/browser/autocomplete/network_action_predictor.cc

Index: chrome/browser/autocomplete/network_action_predictor.cc
diff --git a/chrome/browser/autocomplete/network_action_predictor.cc b/chrome/browser/autocomplete/network_action_predictor.cc
index 848feeb5b84cf1064b8df75bb19ab480c5151477..68d5f509b4b3dc0794efb8472ca98691eaac56e7 100644
--- a/chrome/browser/autocomplete/network_action_predictor.cc
+++ b/chrome/browser/autocomplete/network_action_predictor.cc
@@ -56,6 +56,22 @@ bool GetURLRowForAutocompleteMatch(Profile* profile,
   return url_db && (url_db->GetRowForURL(match.destination_url, url_row) != 0);
 }
 
+// Return true if the suggestion type is a 'search' type.
+bool IsSearchType(const AutocompleteMatch::Type& type) {

[Peter Kasting, 2012/01/09 20:47:37]

Nit: I wonder if this is consistent with other places we make this
determination.  For example, when we decide whether to show "paste and search"
versus "paste and go", we look at the match transition type and check whether
it's TYPED.

[dominich, 2012/01/09 22:43:43]

Hmm. On the other side of this, when we swap in the prerendered page, we set the
transition type to TYPED, so that might make sense. I do want to be explicit
about not prerendering search recommendations though, and this check definitely
does that. The check for transition type would be harder to explain and my sense
is that it's a more dangerous check to make. Ie, if there's a search suggestion
that's being set to typed, that would lead to a subtle bug here.

[Peter Kasting, 2012/01/09 22:51:30]

In the abstract, yes; the fact that we have many other places that do this
already, though, makes it more sane in context.  (Well, kinda.)
Sure, but if there's a search recommendation being set to HISTORY_WHAT_YOU_TYPED
that'd be wrong too.

The critical bit here is that TYPED is only supposed to be used for when the
user navigates to an actual URL they typed in directly.  If their typed string
didn't look like their navigation string, we're to use GENERATED.  The
HistoryURLProvider relies on this in order to autocomplete correctly so we have
strong incentive to get it right.

The search cases are the ones in which the destination URL doesn't look like the
fill_into_edit, so they match up neatly with "search" versus "navigate".

On interesting case to examine is NavSuggest results.  These actually have their
own type, NAVSUGGEST, but one could have argued they should be SEARCH_SUGGEST. 
However, because they look like the actual URL, their transition would be TYPED
and not GENERATED.  So in that world, the transition type would be more reliable
than the match type.

That said, I think the practical effect of these two routes, today, should
always be the same.  My argument is mainly one of consistency with the rest of
the omnibox code.  Perhaps we should achieve this by adding a helper function on
AutocompleteMatch that basically returns "search" or "navigation" and then
picking a method to implement it.

+  switch (type) {
+    // Matches using the user's default search engine.
+    case AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
+    case AutocompleteMatch::SEARCH_HISTORY:
+    case AutocompleteMatch::SEARCH_SUGGEST:
+      // A match that uses a non-default search engine (e.g. for tab-to-search).

[Peter Kasting, 2012/01/09 20:47:37]

Nit: Unindent 2.

[dominich, 2012/01/09 22:43:43]

Done.

+    case AutocompleteMatch::SEARCH_OTHER_ENGINE:
+      return true;
+
+    default:
+      return false;
+  }
+}
+
 }
 
 const int NetworkActionPredictor::kMaximumDaysToKeepEntry = 14;
@@ -150,10 +166,7 @@ NetworkActionPredictor::Action NetworkActionPredictor::RecommendAction(
   // Downgrade prerender to preconnect if this is a search match or if omnibox
   // prerendering is disabled.
   if (action == ACTION_PRERENDER &&
-      ((match.type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED ||
-       match.type == AutocompleteMatch::SEARCH_SUGGEST ||
-       match.type == AutocompleteMatch::SEARCH_OTHER_ENGINE) ||
-       !prerender::IsOmniboxEnabled(profile_))) {
+      (IsPreconnectable(match) || !prerender::IsOmniboxEnabled(profile_))) {
     action = ACTION_PRECONNECT;
   }
 
@@ -164,18 +177,7 @@ NetworkActionPredictor::Action NetworkActionPredictor::RecommendAction(
 // i.e., it is now quite likely that the user will select the related domain.
 // static
 bool NetworkActionPredictor::IsPreconnectable(const AutocompleteMatch& match) {
-  switch (match.type) {
-    // Matches using the user's default search engine.
-    case AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
-    case AutocompleteMatch::SEARCH_HISTORY:
-    case AutocompleteMatch::SEARCH_SUGGEST:
-      // A match that uses a non-default search engine (e.g. for tab-to-search).
-    case AutocompleteMatch::SEARCH_OTHER_ENGINE:
-      return true;
-
-    default:
-      return false;
-  }
+  return IsSearchType(match.type);
 }
 
 void NetworkActionPredictor::Shutdown() {
@@ -231,10 +233,12 @@ void NetworkActionPredictor::OnOmniboxOpenedUrl(const AutocompleteLog& log) {
   if (log.text.length() < kMinimumUserTextLength)
     return;
 
-  UMA_HISTOGRAM_COUNTS("NetworkActionPredictor.NavigationCount", 1);
+  const AutocompleteMatch& match = log.result.match_at(log.selected_index);
+
+  UMA_HISTOGRAM_BOOLEAN("Prerender.OmniboxNavigationsCouldPrerender",
+      prerender::IsOmniboxEnabled(profile_) && !IsSearchType(match.type));
 
-  const GURL& opened_url =
-      log.result.match_at(log.selected_index).destination_url;
+  const GURL& opened_url = match.destination_url;
 
   const string16 lower_user_text(base::i18n::ToLower(log.text));