Make a separate histogram for MatchComplete Final Status'es and

chrome/browser/prerender/prerender_manager.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/prerender/prerender_manager.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 66 matching lines @@
   "POST",
   "TRACE",
 };
 
 // Length of prerender history, for display in chrome://net-internals
 const int kHistoryLength = 100;
 
 // 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:

       final_status != FINAL_STATUS_TIMED_OUT &&
       final_status != FINAL_STATUS_EVICTED &&
       final_status != FINAL_STATUS_MANAGER_SHUTDOWN &&
       final_status != FINAL_STATUS_APP_TERMINATING &&
       final_status != FINAL_STATUS_RENDERER_CRASHED &&
       final_status != FINAL_STATUS_WINDOW_OPENER &&
       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;
 }
 
 }  // namespace
 
 class PrerenderManager::OnCloseTabContentsDeleter
     : public content::WebContentsDelegate,
       public base::SupportsWeakPtr<
           PrerenderManager::OnCloseTabContentsDeleter> {
  public:
@@ skipping 321 matching lines @@
   if (!prerender_contents || !prerender_contents->Init())
     return false;
 
   histograms_->RecordPrerenderStarted(origin);
 
   // TODO(cbentzel): Move invalid checks here instead of PrerenderContents?
   PrerenderContentsData data(prerender_contents, GetCurrentTime());
 
   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);
   }
   while (prerender_list_.size() > config_.max_elements) {
     data = prerender_list_.front();
     prerender_list_.pop_front();
     data.contents_->Destroy(FINAL_STATUS_EVICTED);
   }
   StartSchedulingPeriodicCleanups();
@@ skipping 74 matching lines @@
     }
   }
   // Entry not found.
   return NULL;
 }
 
 PrerenderContents* PrerenderManager::GetEntry(const GURL& url) {
   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) {
   DCHECK(CalledOnValidThread());
   RecordNavigation(url);
 
   scoped_ptr<PrerenderContents> prerender_contents(
       GetEntryButNotSpecifiedWC(url, web_contents));
   if (prerender_contents.get() == NULL)
     return false;
@@ skipping 13 matching lines @@
   bool url_matches = prerender_contents->MatchesURL(url, &matching_url);
   DCHECK(url_matches);
   if (url_matches && url.ref() != matching_url.ref()) {
     prerender_contents.release()->Destroy(FINAL_STATUS_FRAGMENT_MISMATCH);
     return false;
   }
 
   // If we are just in the control group (which can be detected by noticing
   // that prerendering hasn't even started yet), record that |web_contents| now
   // 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;
   }
 
   // If the session storage namespaces don't match, cancel the prerender.
   RenderViewHost* old_render_view_host = web_contents->GetRenderViewHost();
   RenderViewHost* new_render_view_host =
       prerender_contents->prerender_contents()->web_contents()->
           GetRenderViewHost();
   DCHECK(old_render_view_host);
   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;
   }
 
   // If we don't want to use prerenders at all, we are done.
   // For bookkeeping purposes, we need to mark this TabContents to
   // reflect that it would have been prerendered.
   if (GetMode() == PRERENDER_MODE_EXPERIMENT_NO_USE_GROUP) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
     prerender_contents.release()->Destroy(FINAL_STATUS_NO_USE_GROUP);
@@ skipping 88 matching lines @@
        it != prerender_list_.end();
        ++it) {
     if (it->contents_ == entry) {
       bool swapped_in_dummy_replacement = false;
 
       // If this PrerenderContents is being deleted due to a cancellation,
       // we need to create a dummy replacement for PPLT accounting purposes
       // 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(),
                   entry->referrer(),
                   entry->origin(),
                   entry->experiment_id());
           if (dummy_replacement_prerender_contents &&
               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;
           }
         }
       }
       if (!swapped_in_dummy_replacement)
         prerender_list_.erase(it);
       break;
     }
   }
   AddToHistory(entry);
@@ skipping 355 matching lines @@
 void PrerenderManager::DestroyAllContents(FinalStatus final_status) {
   DeleteOldTabContents();
   while (!prerender_list_.empty()) {
     PrerenderContentsData data = prerender_list_.front();
     prerender_list_.pop_front();
     data.contents_->Destroy(final_status);
   }
   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));
   content::RenderProcessHost* render_process_host =
       content::RenderProcessHost::FromID(render_process_id);
   // Each render process is guaranteed to only hold RenderViews owned by the
   // same BrowserContext. This is enforced by
   // RenderProcessHost::GetExistingProcessHost.
   if (!render_process_host || !render_process_host->GetBrowserContext())
     return NULL;
   Profile* profile = Profile::FromBrowserContext(
       render_process_host->GetBrowserContext());
   if (!profile)
     return NULL;
   return PrerenderManagerFactory::GetInstance()->GetForProfile(profile);
 }
 
 }  // namespace prerender