Prerender: Remove MatchComplete

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
diff --git a/chrome/browser/prerender/prerender_manager.cc b/chrome/browser/prerender/prerender_manager.cc
index f3dedfe3ca6a331ec3c74368df1ee8fef6dfc40a..a65cf4ba52fafca4f2ff597efc3880a95602957b 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -86,39 +86,6 @@ const char* const kValidHttpMethods[] = {
 // 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 &
-// 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.
-
-bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) {
-  return final_status != FINAL_STATUS_USED &&
-      final_status != FINAL_STATUS_TIMED_OUT &&
-      final_status != FINAL_STATUS_MANAGER_SHUTDOWN &&
-      final_status != FINAL_STATUS_PROFILE_DESTROYED &&
-      final_status != FINAL_STATUS_APP_TERMINATING &&
-      final_status != FINAL_STATUS_WINDOW_OPENER &&
-      final_status != FINAL_STATUS_CACHE_OR_HISTORY_CLEARED &&
-      final_status != FINAL_STATUS_CANCELLED &&
-      final_status != FINAL_STATUS_DEVTOOLS_ATTACHED &&
-      final_status != FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING &&
-      final_status != FINAL_STATUS_PAGE_BEING_CAPTURED &&
-      final_status != FINAL_STATUS_NAVIGATION_UNCOMMITTED &&
-      final_status != FINAL_STATUS_NON_EMPTY_BROWSING_INSTANCE;
-}
-
 }  // namespace
 
 class PrerenderManager::OnCloseWebContentsDeleter
@@ -550,29 +517,8 @@ void PrerenderManager::MoveEntryToPendingDelete(PrerenderContents* entry,
   ScopedVector<PrerenderData>::iterator it =
       FindIteratorForPrerenderContents(entry);
   DCHECK(it != active_prerenders_.end());
-
-  // If this PrerenderContents is being deleted due to a cancellation any time
-  // after the prerender has started then 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 (entry->prerendering_has_started() &&
-      entry->match_complete_status() ==
-          PrerenderContents::MATCH_COMPLETE_DEFAULT &&
-      NeedMatchCompleteDummyForFinalStatus(final_status) &&
-      ActuallyPrerendering() &&
-      GetMode() == PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP) {
-    // 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.
-    (*it)->MakeIntoMatchCompleteReplacement();
-  } else {
-    to_delete_prerenders_.push_back(*it);
-    active_prerenders_.weak_erase(it);
-  }
-
+  to_delete_prerenders_.push_back(*it);
+  active_prerenders_.weak_erase(it);
   // Destroy the old WebContents relatively promptly to reduce resource usage.
   PostCleanupTask();
 }
@@ -632,8 +578,6 @@ const char* PrerenderManager::GetModeString() {
       return "_15MinTTL";
     case PRERENDER_MODE_EXPERIMENT_NO_USE_GROUP:
       return "_NoUse";
-    case PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP:
-      return "_MatchComplete";
     case PRERENDER_MODE_MAX:

[mmenke, 2016/05/04 19:21:52]

Should update histograms.xml to account for removing this group.

[pasko, 2016/05/09 16:07:51]

I cannot find these suffixes with leading underscore in histograms.xml, were
they renamed or never put there?

I see stuff like _PrerenderMulti, _PrerenderNoUse, _PrerenderMatchComplete via
affected-histogram, but those are already marked as obsolete.

Histogram cleanup looks like a separate pass/CL for me.

[mmenke, 2016/05/09 20:04:39]

The histograms team seem to prefer histograms to be cleaned up in the same CL
that removes code to record the histogram.  That having been said...Yea, I don't
see any corresponding histograms in the XML file.  I have no idea why that is.

[pasko, 2016/05/10 13:52:26]

What I tried to say about a separate CL is: "My preference is also to do
histograms.xml change in the same change that modifies histogram collection, but
in this case I did not find the corresponding histograms.xml parts for this
code. Also, since this is dead code, it's somewhat safe to leave it this way
even if my reading of histograms.xml is wrong".

A separate CL for histograms I envision is about:
1. removing dead code that does not record anything
2. remove code that records useless histograms

So more like 2 CLs, or even more than that. But I did not plan them for short
term.

     default:
       NOTREACHED() << "Invalid PrerenderManager mode.";
@@ -872,16 +816,6 @@ PrerenderManager::PrerenderData::PrerenderData(PrerenderManager* manager,
 PrerenderManager::PrerenderData::~PrerenderData() {
 }
 
-void PrerenderManager::PrerenderData::MakeIntoMatchCompleteReplacement() {
-  DCHECK(contents_);
-  contents_->set_match_complete_status(
-      PrerenderContents::MATCH_COMPLETE_REPLACED);
-  PrerenderData* to_delete = new PrerenderData(manager_, contents_.release(),
-                                               expiry_time_);
-  contents_.reset(to_delete->contents_->CreateMatchCompleteReplacement());
-  manager_->to_delete_prerenders_.push_back(to_delete);
-}
-
 void PrerenderManager::PrerenderData::OnHandleCreated(PrerenderHandle* handle) {
   DCHECK(contents_);
   ++handle_count_;