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

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index d48eb7b747a3fd64c839d25958b888d10d4cbce1..5f587c80b1f681a6d501aa5bb191dff3f5fcef0d 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -188,6 +188,8 @@
 #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"
@@ -1516,12 +1518,15 @@ void MetricsService::LogLoadStarted(content::WebContents* web_contents) {
 
   // 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"));
   }
 }