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,20 @@
 // 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 &

[dominich, 2012/01/20 22:23:37]

Match? or MatchComplete?

[tburkard, 2012/01/20 23:23:00]

What I wrote is correct, I think.
On 2012/01/20 22:23:37, dominich wrote:

+// 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, by comparing a larger Match in Control vs. a smaller Match

[dominich, 2012/01/20 22:23:37]

Match or MatchComplete?

[tburkard, 2012/01/20 23:23:00]

Done.

+// in the Prerender group, there is no bias in terms of the page load times
+// of the pages forming the difference between the two sets.
 
 bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) {
   return final_status != FINAL_STATUS_USED &&

[dominich, 2012/01/20 22:23:37]

Might this be easier as an inclusive check rather than exclusive?

[tburkard, 2012/01/20 23:23:00]

No, because as new status'es are added, the default should be to generate a MC
dummy.
On 2012/01/20 22:23:37, dominich wrote:

@@ -96,6 +110,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 +454,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 +549,17 @@
   return GetEntryButNotSpecifiedWC(url, NULL);
 }
 
+void PrerenderManager::DestroyAndMarkMatchCompleteAsUsed(
+    PrerenderContents* prerender_contents,
+    FinalStatus final_status) {
+  prerender_contents->set_mc_status(PrerenderContents::MC_REPLACED);
+  histograms_->RecordMatchCompleteFinalStatus(
+      prerender_contents->origin(),
+      prerender_contents->experiment_id(),
+      FINAL_STATUS_USED);
+  prerender_contents->Destroy(FINAL_STATUS_DEVTOOLS_ATTACHED);

[dominich, 2012/01/20 22:23:37]

This should be |final_status|. You might need a test for it.

[tburkard, 2012/01/20 23:23:00]

Great catch.  Haven't even run/written tests yet while you are still providing
comments and the code is changing so much.

On 2012/01/20 22:23:37, dominich wrote:

+}
+
 bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
                                                const GURL& url,
                                                const GURL& opener_url) {
@@ -569,20 +595,23 @@
   // would be showing a prerendered contents, but otherwise, don't do anything.
   if (!prerender_contents->prerendering_has_started()) {

[dominich, 2012/01/20 22:23:37]

Why is this different to IsControlGroup?

[tburkard, 2012/01/20 23:23:00]

I already answered this before, I said:
This is necessary to also cover e.g. the case of a MatchComplete dummy.
prerendering_has_started() is the easiest check to cover all these
cases.  It is also more robust in case one day, the field trial could
change on the fly.
On 2012/01/20 22:23:37, dominich wrote:

     MarkWebContentsAsWouldBePrerendered(web_contents);
+    prerender_contents.release()->Destroy(FINAL_STATUS_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 +625,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 +735,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 &&

[dominich, 2012/01/20 22:23:37]

if mc_status is only set from this method, this condition should always be true.
Could it be a DCHECK?

[tburkard, 2012/01/20 23:23:00]

huh?  why?  Can you give an example?

A prerender may be destroyed, replaced with a matchcomplete dummy, which in turn
may be destroyed later for some other reason, and you want to make sure to not
create a matchcomplete dummy for another matchcomplete dummy currently being
destroyed.  that's why this check is needed.

On 2012/01/20 22:23:37, dominich wrote:

+          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 +754,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 +1132,28 @@
   DeletePendingDeleteEntries();
 }
 
+void PrerenderManager::RecordFinalStatusWithMatchCompleteStatus(
+    Origin origin,
+    uint8 experiment_id,
+    PrerenderContents::MatchCompleteStatus mc_status,
+    FinalStatus final_status) const {

[dominich, 2012/01/20 22:23:37]

MC_DEFAULT will be in both histograms. Is this expected?

[tburkard, 2012/01/20 23:23:00]

not sure why stuff keeps being repeated.  i said earlier:
Yes.  There will be 2 histograms for the Prerender gorups only: The
actual FinalStatus'es as perceived by the user, and the MatchComplete
FinalStatus'es, that should line up with the control group.  Notice that
the matchComplete final status histograms will not be generated for the
control groups or the no use groups, so this is only adding a handful of
new histograms.
On 2012/01/20 22:23:37, dominich wrote:

+  if (mc_status != PrerenderContents::MC_REPLACEMENT)
+    histograms_->RecordFinalStatus(origin, experiment_id, final_status);
+  if (mc_status != PrerenderContents::MC_REPLACED) {
+    histograms_->RecordMatchCompleteFinalStatus(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));