[InstantExtended] Fixing how PageLoadSRP is emitted. Previously it was emitted only for Instant sea…

chrome/browser/metrics/metrics_service.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.
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
 // A MetricsService instance is typically created at application startup.  It is
@@ skipping 170 matching lines @@
 #include "chrome/browser/metrics/metrics_log.h"
 #include "chrome/browser/metrics/metrics_log_serializer.h"
 #include "chrome/browser/metrics/metrics_reporting_scheduler.h"
 #include "chrome/browser/metrics/time_ticks_experiment_win.h"
 #include "chrome/browser/metrics/tracking_synchronizer.h"
 #include "chrome/browser/net/http_pipelining_compatibility_client.h"
 #include "chrome/browser/net/network_stats.h"
 #include "chrome/browser/omnibox/omnibox_log.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_otr_state.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/metrics/entropy_provider.h"
 #include "chrome/common/metrics/metrics_log_manager.h"
 #include "chrome/common/net/test_server_locations.h"
 #include "chrome/common/pref_names.h"
@@ skipping 1308 matching lines @@
 void MetricsService::LogLoadStarted(content::WebContents* web_contents) {
   content::RecordAction(content::UserMetricsAction("PageLoad"));
   HISTOGRAM_ENUMERATION("Chrome.UmaPageloadCounter", 1, 2);
   IncrementPrefValue(prefs::kStabilityPageLoadCount);
   IncrementLongPrefsValue(prefs::kUninstallMetricsPageLoadCount);
   // We need to save the prefs, as page load count is a critical stat, and it
   // might be lost due to a crash :-(.
 
   // Track whether the page loaded is a search results page.
   if (web_contents) {
-    SearchTabHelper* search_tab_helper =
-        SearchTabHelper::FromWebContents(web_contents);
-    if (search_tab_helper) {
-      if (search_tab_helper->model()->mode().is_search_results())
-        content::RecordAction(content::UserMetricsAction("PageLoadSRP"));
-    }
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());
+    bool is_search_results =
+        TemplateURLServiceFactory::GetForProfile(profile)->
+            IsSearchResultsPageFromDefaultSearchProvider(
+                web_contents->GetLastCommittedURL());

[rpetterson, 2013/07/31 22:49:21]

My biggest dilemma is here. I have available the LastCommittedURL and the
VisibleURL. Based on the comments in web_contents.h, I think LastCommittedURL is
the desired one. However, this results in the following behavior:
Visit www.google.com and entry a search query. Metric does not show up until the
SRP becomes the last committed url which is typically when the user navigates
away. I see the potential to lose metrics here. I also worry about the
correlation between PageLoad and this metric.

If I use the GetVisibleURL we get extra hits as pages are visible, but not
necessarily committed for loading.

Thoughts and suggestions are appreciated.

[Mark P, 2013/07/31 23:16:49]

What do you mean by this?

I know we emit PageLoads a bit more often than makes sense to me; that's not a
problem we need to tackle now.  Does using GetVisibleURL mean we're
over-aggressive at marking PageLoads as a search results page?

[Peter Kasting, 2013/07/31 23:56:51]

What you want depends on what this metric means and where it gets called from.

Loads go through a couple of stages.  First a load is made pending.  This means
the load has been requested, and we've sent a request to the server, but we
haven't heard back.  Then a load is committed, which means the server has
replied and we've begun transferring the page.  Finally the load is completed. 
At any stage in this process, a load can be cancelled.

So the question is, what is "LogLoadStarted" supposed to mean?  Is it supposed
to mean "log that the user requested that some load happen"?  In that case, you
want to be called as soon as an entry becomes pending, and you want the pending
entry.  Or does it mean "log that a load began to actually happen", in which
case you probably want to be called when the load commits, and get the committed
entry?

From your description above, it sounds as if you're getting called when a load
is made pending, which would explain why, when you look at the committed entry,
you don't see what you expected until the load after this one (e.g. when the
user navigates away from the page, and thus the navigation away becomes
pending).

I can't tell you what the metric is supposed to mean, so I can't tell you the
right way to go.

+    if (is_search_results)
+        content::RecordAction(
+            content::UserMetricsAction("PageLoadSRPonDefaultSearchProvider"));
   }
 }
 
 void MetricsService::LogRendererCrash(content::RenderProcessHost* host,
                                       base::TerminationStatus status,
                                       int exit_code) {
   Profile* profile = Profile::FromBrowserContext(host->GetBrowserContext());
   ExtensionService* service = profile->GetExtensionService();
   bool was_extension_process =
       service && service->process_map()->Contains(host->GetID());
@@ skipping 219 matching lines @@
   if (local_state) {
     const PrefService::Preference* uma_pref =
         local_state->FindPreference(prefs::kMetricsReportingEnabled);
     if (uma_pref) {
       bool success = uma_pref->GetValue()->GetAsBoolean(&result);
       DCHECK(success);
     }
   }
   return result;
 }