Prerender: Remove MatchComplete

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 <stddef.h>
 
 #include <algorithm>
 #include <functional>
@@ skipping 68 matching lines @@
   "GET",
   "HEAD",
   "OPTIONS",
   "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 &
-// 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
     : public content::WebContentsDelegate,
       public base::SupportsWeakPtr<
           PrerenderManager::OnCloseWebContentsDeleter> {
  public:
   OnCloseWebContentsDeleter(PrerenderManager* manager,
                             WebContents* tab)
       : manager_(manager),
@@ skipping 411 matching lines @@
 }
 
 void PrerenderManager::MoveEntryToPendingDelete(PrerenderContents* entry,
                                                 FinalStatus final_status) {
   DCHECK(CalledOnValidThread());
   DCHECK(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();
 }
 
 void PrerenderManager::RecordPageLoadTimeNotSwappedIn(
     Origin origin,
     base::TimeDelta page_load_time,
     const GURL& url) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   histograms_->RecordPageLoadTimeNotSwappedIn(origin, page_load_time, url);
@@ skipping 39 matching lines @@
     case PRERENDER_MODE_EXPERIMENT_PRERENDER_GROUP:
       return "_Enabled";
     case PRERENDER_MODE_EXPERIMENT_CONTROL_GROUP:
       return "_Control";
     case PRERENDER_MODE_EXPERIMENT_MULTI_PRERENDER_GROUP:
       return "_Multi";
     case PRERENDER_MODE_EXPERIMENT_15MIN_TTL_GROUP:
       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.";
       break;
   }
   return "";
 }
 
 // static
 bool PrerenderManager::IsPrerenderingPossible() {
   return GetMode() != PRERENDER_MODE_DISABLED;
@@ skipping 217 matching lines @@
     : manager_(manager),
       contents_(contents),
       handle_count_(0),
       expiry_time_(expiry_time) {
   DCHECK(contents_);
 }
 
 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_;
   contents_->AddObserver(handle);
 }
 
 void PrerenderManager::PrerenderData::OnHandleNavigatedAway(
     PrerenderHandle* handle) {
   DCHECK_LT(0, handle_count_);
   DCHECK(contents_);
@@ skipping 486 matching lines @@
 }
 
 void PrerenderManager::RenderProcessHostDestroyed(
     content::RenderProcessHost* host) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   size_t erased = prerender_process_hosts_.erase(host);
   DCHECK_EQ(1u, erased);
 }
 
 }  // namespace prerender