Make a separate histogram for MatchComplete Final Status'es and

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
===================================================================
--- chrome/browser/prerender/prerender_manager.cc	(revision 118385)
+++ chrome/browser/prerender/prerender_manager.cc	(working copy)
@@ -84,6 +84,19 @@
 // Indicates whether a Prerender has been cancelled such that we need
 // a dummy replacement for the purpose of recording the correct PPLT for
 // the Match Complete case.
+// Traditionally, "Match" means that a prerendered page was actually visited &
+// the prerender was used.  Our goal is to have "Match" cases line up in the
+// control group & the experiment group, so that we can make meaningful
+// comparisons of improvements.  However, in the control group, since we don't
+// actually perform prerenders, many of the cancellation reasons cannot be
+// detected.  Therefore, in the Prerender group, when we cancel for one of these
+// reasons, we keep track of a dummy Prerender representing what we would
+// have in the control group.  If that dummy prerender in the prerender group
+// would then be swapped in (but isn't actually b/c it's a dummy), we record
+// this as a MatchComplete.  This allows us to compare MatchComplete's
+// across Prerender & Control group which ideally should be lining up.
+// This ensures that there is no bias in terms of the page load times
+// of the pages forming the difference between the two sets.

[dominich, 2012/01/23 22:41:40]

Could you create an issue for someone to look into minimizing the histograms to
those that can actually be compared/are useful to people?

[tburkard, 2012/01/24 00:59:56]

Created Issue 111130

On 2012/01/23 22:41:40, dominich wrote:

 
 bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) {
   return final_status != FINAL_STATUS_USED &&
@@ -96,6 +109,9 @@
       final_status != FINAL_STATUS_FRAGMENT_MISMATCH &&
       final_status != FINAL_STATUS_CACHE_OR_HISTORY_CLEARED &&
       final_status != FINAL_STATUS_CANCELLED &&
+      final_status != FINAL_STATUS_SESSION_STORAGE_NAMESPACE_MISMATCH &&
+      final_status != FINAL_STATUS_DEVTOOLS_ATTACHED &&
+      final_status != FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING &&
       final_status != FINAL_STATUS_MATCH_COMPLETE_DUMMY;
 }
 
@@ -437,9 +453,7 @@
 
   prerender_list_.push_back(data);
 
-  if (IsControlGroup()) {
-    data.contents_->set_final_status(FINAL_STATUS_CONTROL_GROUP);
-  } else {
+  if (!IsControlGroup()) {
     last_prerender_start_time_ = GetCurrentTimeTicks();
     data.contents_->StartPrerendering(source_render_view_host,
                                       session_storage_namespace);
@@ -534,6 +548,17 @@
   return GetEntryButNotSpecifiedWC(url, NULL);
 }
 
+void PrerenderManager::DestroyAndMarkMatchCompleteAsUsed(
+    PrerenderContents* prerender_contents,
+    FinalStatus final_status) {
+  prerender_contents->set_mc_status(PrerenderContents::MC_REPLACED);
+  histograms_->RecordFinalStatusMatchComplete(
+      prerender_contents->origin(),
+      prerender_contents->experiment_id(),
+      FINAL_STATUS_USED);
+  prerender_contents->Destroy(final_status);
+}
+
 bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
                                                const GURL& url,
                                                const GURL& opener_url) {
@@ -569,20 +594,23 @@
   // would be showing a prerendered contents, but otherwise, don't do anything.
   if (!prerender_contents->prerendering_has_started()) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
+    prerender_contents.release()->Destroy(FINAL_STATUS_USED);

[dominich, 2012/01/23 22:41:40]

Was this leaking before?

[tburkard, 2012/01/24 00:59:56]

No.  Before, the final status in this case was e.g. FINAL_STATUS_CONTROL_GROUP,
and needed not be overwritten.  Now that we have a separate histogram and want
the actual termination status, we have to declare this STATUS_USED, since it
would have been used.

     return false;
   }
 
   // Don't use prerendered pages if debugger is attached to the tab.
   // See http://crbug.com/98541
   if (content::DevToolsAgentHostRegistry::IsDebuggerAttached(web_contents)) {
-    prerender_contents.release()->Destroy(FINAL_STATUS_DEVTOOLS_ATTACHED);
+    DestroyAndMarkMatchCompleteAsUsed(prerender_contents.release(),
+                                      FINAL_STATUS_DEVTOOLS_ATTACHED);
     return false;
   }
 
   // If the prerendered page is in the middle of a cross-site navigation,
   // don't swap it in because there isn't a good way to merge histories.
   if (prerender_contents->IsCrossSiteNavigationPending()) {
-    prerender_contents.release()->Destroy(
+    DestroyAndMarkMatchCompleteAsUsed(
+        prerender_contents.release(),
         FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING);
     return false;
   }
@@ -596,7 +624,8 @@
   DCHECK(new_render_view_host);
   if (old_render_view_host->session_storage_namespace() !=
       new_render_view_host->session_storage_namespace()) {
-    prerender_contents.release()->Destroy(
+    DestroyAndMarkMatchCompleteAsUsed(
+        prerender_contents.release(),
         FINAL_STATUS_SESSION_STORAGE_NAMESPACE_MISMATCH);
     return false;
   }
@@ -705,13 +734,15 @@
       // for the Match Complete group.
       // This is the case if the cancellation is for any reason that would not
       // occur in the control group case.
-      if (NeedMatchCompleteDummyForFinalStatus(final_status)) {
+      if (entry->mc_status() == PrerenderContents::MC_DEFAULT &&
+          NeedMatchCompleteDummyForFinalStatus(final_status)) {
         // TODO(tburkard): I'd like to DCHECK that we are actually prerendering.
         // However, what if new conditions are added and
         // NeedMatchCompleteDummyForFinalStatus, is not being updated.  Not sure
         // what's the best thing to do here.  For now, I will just check whether
         // we are actually prerendering.
         if (ActuallyPrerendering()) {
+          entry->set_mc_status(PrerenderContents::MC_REPLACED);
           PrerenderContents* dummy_replacement_prerender_contents =
               CreatePrerenderContents(
                   entry->prerender_url(),
@@ -722,8 +753,9 @@
               dummy_replacement_prerender_contents->Init()) {
             dummy_replacement_prerender_contents->
                 AddAliasURLsFromOtherPrerenderContents(entry);
+            dummy_replacement_prerender_contents->set_mc_status(
+                PrerenderContents::MC_REPLACEMENT);
             it->contents_ = dummy_replacement_prerender_contents;
-            it->contents_->set_final_status(FINAL_STATUS_MATCH_COMPLETE_DUMMY);
             swapped_in_dummy_replacement = true;
           }
         }
@@ -1099,12 +1131,28 @@
   DeletePendingDeleteEntries();
 }
 
+void PrerenderManager::RecordFinalStatusWithMatchCompleteStatus(
+    Origin origin,
+    uint8 experiment_id,
+    PrerenderContents::MatchCompleteStatus mc_status,
+    FinalStatus final_status) const {
+  if (mc_status != PrerenderContents::MC_REPLACEMENT)
+    histograms_->RecordFinalStatus(origin, experiment_id, final_status);
+  if (mc_status != PrerenderContents::MC_REPLACED) {
+    histograms_->RecordFinalStatusMatchComplete(origin, experiment_id,
+                                                final_status);
+  }
+}
+
 void PrerenderManager::RecordFinalStatus(Origin origin,
                                          uint8 experiment_id,
                                          FinalStatus final_status) const {
-  histograms_->RecordFinalStatus(origin, experiment_id, final_status);
+  RecordFinalStatusWithMatchCompleteStatus(origin, experiment_id,
+                                           PrerenderContents::MC_DEFAULT,
+                                           final_status);
 }
 
+
 PrerenderManager* FindPrerenderManagerUsingRenderProcessId(
     int render_process_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));