Add UMA metrics for Android Chrome Google Search.

chrome/browser/google/google_util.cc

 // Copyright (c) 2012 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 "chrome/browser/google/google_util.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/google/core/browser/google_switches.h"
 #include "components/google/core/browser/google_url_tracker.h"
 #include "components/url_fixer/url_fixer.h"
+#include "content/public/browser/navigation_entry.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
 #include "url/gurl.h"
 
 // Only use Link Doctor on official builds.  It uses an API key, too, but
 // seems best to just disable it, for more responsive error pages and to reduce
 // server load.
 #if defined(GOOGLE_CHROME_BUILD)
 #define LINKDOCTOR_SERVER_REQUEST_URL "https://www.googleapis.com/rpc"
 #else
@@ skipping 35 matching lines @@
 
 // True if |url| is a valid URL with HTTP or HTTPS scheme. If |port_permission|
 // is DISALLOW_NON_STANDARD_PORTS, this also requires |url| to use the standard
 // port for its scheme (80 for HTTP, 443 for HTTPS).
 bool IsValidURL(const GURL& url, google_util::PortPermission port_permission) {
   return url.is_valid() && url.SchemeIsHTTPOrHTTPS() &&
       (url.port().empty() ||
        (port_permission == google_util::ALLOW_NON_STANDARD_PORTS));
 }
 
+// Returns true iff |entry| represents a Google search from the Omnibox.
+// This method assumes that we have already verified that |entry|'s URL is a
+// Google search URL.
+bool IsOmniboxGoogleSearchNavigation(const content::NavigationEntry& entry) {

[Peter Kasting, 2014/06/19 21:20:32]

Given that these two functions are extremely short and only called once, I think
it would be better to inline their contents below.  This would avoid the need to
check or comment about the status of IsGoogleSearchURL().

[kmadhusu, 2014/06/19 22:52:49]

Done.

+  const content::PageTransition stripped_transition =
+      PageTransitionStripQualifier(entry.GetTransitionType());
+  DCHECK(google_util::IsGoogleSearchUrl(entry.GetURL()));
+  return stripped_transition == content::PAGE_TRANSITION_GENERATED;

[Peter Kasting, 2014/06/19 21:20:32]

Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR?  GENERATED
alone could be from different sources.

[kmadhusu, 2014/06/19 22:52:49]

I moved this code from GoogleSearchCounter to here. I retained the original
code. But it seems reasonable to check for PAGE_TRANSITION_FROM_ADDRESS_BAR. Can
you give some examples of other sources? So that I can test and verify that I am
not causing any regression after adding this check. 

[Peter Kasting, 2014/06/19 22:58:05]

I would grep the codebase for PAGE_TRANSITION_GENERATED to see.  Maybe a
bookmark of a Google search?

[kmadhusu, 2014/06/19 23:21:45]

Okay.

[kmadhusu, 2014/06/20 01:15:07]

Updated the check.

+}
+
+// Returns true iff |entry| represents a Google search from the Google Search
+// App. This method assumes that we have already verified that |entry|'s URL is
+// a Google search URL.
+bool IsSearchAppGoogleSearchNavigation(const content::NavigationEntry& entry) {
+  DCHECK(google_util::IsGoogleSearchUrl(entry.GetURL()));
+  return entry.GetURL().query().find("source=search_app") !=
+         std::string::npos;

[Peter Kasting, 2014/06/19 21:20:32]

Nit: Indent 4, not even

[kmadhusu, 2014/06/19 22:52:49]

Fixed.

+}
+
 }  // namespace
 
 
 namespace google_util {
 
 // Global functions -----------------------------------------------------------
 
 bool HasGoogleSearchQueryParam(const std::string& str) {
   url::Component query(0, str.length()), key, value;
   while (url::ExtractQueryKeyValue(str.c_str(), &query, &key, &value)) {
@@ skipping 123 matching lines @@
       (!is_home_page_base && HasGoogleSearchQueryParam(url.query()));
 }
 
 bool IsYoutubeDomainUrl(const GURL& url,
                         SubdomainPermission subdomain_permission,
                         PortPermission port_permission) {
   return IsValidURL(url, port_permission) &&
       IsValidHostName(url.host(), "youtube", subdomain_permission);
 }
 
+GoogleSearchMetrics::AccessPoint GetGoogleSearchAccessPointForNavEntry(
+    const content::NavigationEntry& entry) {
+  DCHECK(google_util::IsGoogleSearchUrl(entry.GetURL()));
+
+  // If the commit is a GENERATED commit with a Google search URL, we know it's
+  // an Omnibox search.

[Peter Kasting, 2014/06/19 21:20:32]

This comment goes into detail that isn't visible here.  Either eliminate it or
change it to only refer to externally-visible aspects of
IsOmniboxGoogleSearchNavigation().

[kmadhusu, 2014/06/19 22:52:49]

Inlined IsOmniboxGoogleSearchNavigation() definition here. Keeping the comments
as such.

+  if (IsOmniboxGoogleSearchNavigation(entry))
+    return GoogleSearchMetrics::AP_OMNIBOX;
+
+  // For all other cases that we have not yet implemented or care to measure,
+  // we log a generic "catch-all" metric.
+  return IsSearchAppGoogleSearchNavigation(entry) ?
+      GoogleSearchMetrics::AP_SEARCH_APP : GoogleSearchMetrics::AP_OTHER;
+}
+
 }  // namespace google_util