Add UMA metrics for Android Chrome Google Search.

chrome/browser/google/google_search_counter.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_search_counter.h"
 
 #include "base/logging.h"
 #include "chrome/browser/google/google_util.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 
-namespace {
-
-// 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) {
-  const content::PageTransition stripped_transition =
-      PageTransitionStripQualifier(entry.GetTransitionType());
-  DCHECK(google_util::IsGoogleSearchUrl(entry.GetURL()));
-  return stripped_transition == content::PAGE_TRANSITION_GENERATED;
-}
-
-// 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;
-}
-
-}  // namespace
-
 // static
 void GoogleSearchCounter::RegisterForNotifications() {
   GoogleSearchCounter::GetInstance()->RegisterForNotificationsInternal();
 }
 
 // static
 GoogleSearchCounter* GoogleSearchCounter::GetInstance() {
   return Singleton<GoogleSearchCounter>::get();
 }
 
+bool GoogleSearchCounter::ShouldRecordCommittedDetails(
+    const content::NotificationDetails& details) const {
+  const content::LoadCommittedDetails* commit =
+      content::Details<content::LoadCommittedDetails>(details).ptr();
+  const content::NavigationEntry& entry = *commit->entry;
+
+  // First see if this is a Google search URL at all.
+  if (!google_util::IsGoogleSearchUrl(entry.GetURL()))
+    return false;
+
+  // Avoid double counting. Sometimes, when a navigation entry is committed, we
+  // receive two notifications events for the same page.

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

Nit: notifications events -> notifications/notification events (pick one)

What does this comment mean, though?  Why are we getting two notifications, and
what types are they?  This seems strange and wrong.

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

Done.
When I submit a search query from Android Chrome omnibox to the Instant search
base page, I am seeing two commit entry notifications for the same search query.
I am not sure why I am getting two notifications. They are of
PAGE_TRANSITION_GENERATED type. I am testing my changes against a custom google
server base url which has my local changes. When I test against google.com, I am
not seeing these double notifications for search URLs. I didn't have time to
confirm that the problem is in my server side code. Therefore, I added this
check to work around this issue. Once I land this CL, I will investigate this
issue and remove this check. Added a TODO to do that.

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

:(  This would normally cause me to reject this patch until you determine the
underlying cause of the issue, even if you don't actually have the cause fixed
in this change.  That way we can be sure we're not also missing some unknown
other problems.

Why do you not have time to figure this out?  Why can't this patch land when
it's ready and be merged if it's reasonable?

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

My server side changes are still in prototype stage. It will take more than a
day to figure out this issue. (I already spent a whole day to confirm that it is
something related to my server side code).

Given the size of this CL, it will be very hard to merge the entire patch later.
Until my server side changes are in, I am not planning to start any experiments.
Can I can remove this check for now and check in the framework? Framework is
working as expected and I don't expect any problems. Once I figure out the
issue,  I can merge this check if needed.

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

As we discussed offline, I removed this dupe check.

+  return commit->previous_url.is_valid() &&
+      entry.GetURL() != commit->previous_url;
+}
+
 GoogleSearchCounter::GoogleSearchCounter()
     : search_metrics_(new GoogleSearchMetrics) {
 }
 
 GoogleSearchCounter::~GoogleSearchCounter() {
 }
 
 void GoogleSearchCounter::ProcessCommittedEntry(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
+  // Note that GoogleSearchMetrics logs metrics through UMA, which will only
+  // transmit these counts to the server if the user has opted into sending
+  // usage stats.
   const content::LoadCommittedDetails* commit =
       content::Details<content::LoadCommittedDetails>(details).ptr();
   const content::NavigationEntry& entry = *commit->entry;
-
-  // First see if this is a Google search URL at all.
-  if (!google_util::IsGoogleSearchUrl(entry.GetURL()))
-    return;
-
-  // If the commit is a GENERATED commit with a Google search URL, we know it's
-  // an Omnibox search.
-  if (IsOmniboxGoogleSearchNavigation(entry)) {
-    // Note that GoogleSearchMetrics logs metrics through UMA, which will only
-    // transmit these counts to the server if the user has opted into sending
-    // usage stats.
-    search_metrics_->RecordGoogleSearch(GoogleSearchMetrics::AP_OMNIBOX);
-  } else if (IsSearchAppGoogleSearchNavigation(entry)) {
-    search_metrics_->RecordGoogleSearch(GoogleSearchMetrics::AP_SEARCH_APP);
-  } else {
-    // For all other cases that we have not yet implemented or care to measure,
-    // we log a generic "catch-all" metric.
-    search_metrics_->RecordGoogleSearch(GoogleSearchMetrics::AP_OTHER);
+  if (ShouldRecordCommittedDetails(details)) {
+    search_metrics_->RecordGoogleSearch(
+        google_util::GetGoogleSearchAccessPointForNavEntry(entry));
   }
 }
 
 void GoogleSearchCounter::SetSearchMetricsForTesting(
     GoogleSearchMetrics* search_metrics) {
   DCHECK(search_metrics);
   search_metrics_.reset(search_metrics);
 }
 
 void GoogleSearchCounter::RegisterForNotificationsInternal() {
   // We always listen for all COMMITTED navigations from all sources, as any
   // one of them could be a navigation of interest.
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                  content::NotificationService::AllSources());
 }
 
 void GoogleSearchCounter::Observe(int type,
                                   const content::NotificationSource& source,
                                   const content::NotificationDetails& details) {
   switch (type) {
     case content::NOTIFICATION_NAV_ENTRY_COMMITTED:
       ProcessCommittedEntry(source, details);
       break;
     default:
       NOTREACHED();
   }
 }