Fix prerender histograms for multiple prerender case.

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 2282f21a288c64f61f39568e07796d8d169d78f2..e932868887e4fabc36f8ba0119b0c4371e3a97b5 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -165,14 +165,19 @@ PrerenderManager::PrerenderManagerMode PrerenderManager::mode_ =
     PRERENDER_MODE_ENABLED;
 
 struct PrerenderManager::NavigationRecord {
-  GURL url_;
-  base::TimeTicks time_;
+  GURL url;
+  base::TimeTicks time;
+
   NavigationRecord(const GURL& url, base::TimeTicks time)
-      : url_(url),
-        time_(time) {
+      : url(url),
+        time(time) {
   }
 };
 
+PrerenderManager::PrerenderedWebContentsData::
+PrerenderedWebContentsData(Origin origin) : origin(origin) {
+}
+
 PrerenderManager::PrerenderManager(Profile* profile,
                                    PrerenderTracker* prerender_tracker)
     : enabled_(true),
@@ -182,8 +187,7 @@ PrerenderManager::PrerenderManager(Profile* profile,
       last_prerender_start_time_(GetCurrentTimeTicks() -
           base::TimeDelta::FromMilliseconds(kMinTimeBetweenPrerendersMs)),
       weak_factory_(this),
-      prerender_history_(new PrerenderHistory(kHistoryLength)),
-      histograms_(new PrerenderHistograms()) {
+      prerender_history_(new PrerenderHistory(kHistoryLength)) {
   // There are some assumptions that the PrerenderManager is on the UI thread.
   // Any other checks simply make sure that the PrerenderManager is accessed on
   // the same thread that it was created on.
@@ -200,6 +204,8 @@ PrerenderManager::PrerenderManager(Profile* profile,
     default:
       break;
   }
+
+  histograms_.reset(new PrerenderHistograms(config_.time_to_live));
 }
 
 PrerenderManager::~PrerenderManager() {
@@ -232,7 +238,7 @@ PrerenderHandle* PrerenderManager::AddPrerenderFromLinkRelPrerender(
           new PrerenderHandle(pending_prerender_list_.back().get());
       contents->AddPendingPrerender(
           prerender_handle->weak_ptr_factory_.GetWeakPtr(),
-          url, referrer, size);
+          ORIGIN_LINK_REL_PRERENDER, url, referrer, size);
       return prerender_handle;
     }
   }
@@ -372,11 +378,13 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
 
   if (!prerender_contents->load_start_time().is_null()) {
     histograms_->RecordTimeUntilUsed(
+        prerender_contents->origin(),
         GetCurrentTimeTicks() - prerender_contents->load_start_time(),
         config_.time_to_live);
   }
 
-  histograms_->RecordPerSessionCount(++prerenders_per_session_count_);
+  histograms_->RecordPerSessionCount(prerender_contents->origin(),
+                                     ++prerenders_per_session_count_);
   histograms_->RecordUsedPrerender(prerender_contents->origin());
   prerender_contents->set_final_status(FINAL_STATUS_USED);
 
@@ -397,7 +405,8 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   DCHECK(new_tab_contents);
   DCHECK(old_tab_contents);
 
-  MarkWebContentsAsPrerendered(new_tab_contents->web_contents());
+  MarkWebContentsAsPrerendered(new_tab_contents->web_contents(),
+                               prerender_contents->origin());
 
   // Merge the browsing history.
   new_tab_contents->web_contents()->GetController().CopyStateFromAndPrune(
@@ -511,17 +520,22 @@ void PrerenderManager::RecordPerceivedPageLoadTime(
     return;
   if (!prerender_manager->IsEnabled())
     return;
+  const Origin* prerender_origin = NULL;

[mmenke, 2012/10/05 17:01:15]

Using a pointer for this seems a little weird and overly complicated.  Why not
just use an Origin, initialize it to ORIGIN_NONE, and have
IsWebContentsPrerendered guaranteed to leave it alone when it returns false (Or
set it to ORIGIN_NONE).

[gavinp, 2012/10/05 20:43:35]

Done.

[gavinp, 2012/10/05 20:43:35]

Done.

   bool was_prerender =
-      prerender_manager->IsWebContentsPrerendered(web_contents);
+      prerender_manager->IsWebContentsPrerendered(web_contents,
+                                                  &prerender_origin);
   bool was_complete_prerender = was_prerender ||
       prerender_manager->WouldWebContentsBePrerendered(web_contents);

[mmenke, 2012/10/05 17:01:15]

So only real prerenders get an origin?  Non-prerenders don't get one, and
neither do "WouldBePrerendered" prerenders?  We could get the origin for the
latter set now, right?

[gavinp, 2012/10/05 20:43:35]

Non prerenders would be tricky, yeah. And it was always weird that in histograms
they were all considered link prerenders.

We could track it for "WouldBePrerendered", yes. But right now we don't in that
case, so in control group, all of these perf histograms will be ORIGIN_NONE,
which is fine by me.

[mmenke, 2012/10/08 18:20:07]

Sorry, this is fine.  I was getting PrerenderedWebContentsData and
WouldBePrerenderedMap confused, so was thinking we were already tracking it. 
Since we're not tracking it, I strongly agree that it's not worth adding.

[gavinp, 2012/10/10 18:14:56]

Timo asked for it in his review, so I've added it. Previously this information
was sort-of-tracked via the global variable in PrerenderHistograms, so adding it
was fair. I would like to talk about how much reporting we do for prerendering
though: please wait for my upcoming mailing list post to be followed by a
contentious team meeting.

   if (prerender_manager->IsWebContentsPrerendering(web_contents)) {
     prerender_manager->histograms_->RecordPageLoadTimeNotSwappedIn(
+        was_prerender ? *prerender_origin : ORIGIN_NONE,
         perceived_page_load_time, url);
   } else {
     prerender_manager->histograms_->RecordPerceivedPageLoadTime(
+        was_prerender ? *prerender_origin : ORIGIN_NONE,
         perceived_page_load_time, was_prerender, was_complete_prerender, url);
     prerender_manager->histograms_->RecordPercentLoadDoneAtSwapin(
+        was_prerender ? *prerender_origin : ORIGIN_NONE,
         fraction_plt_elapsed_at_swap_in);

[mmenke, 2012/10/05 17:01:15]

If was_prerender is false, should we even bother calling this function?  While
it's not exactly obvious what's going on, since the code is more than a little
confusing, it looks like |actual_load_start_| is only set for real prerenders,
so it's NULL exactly when was_prerender is false...Right?

[gavinp, 2012/10/05 20:43:35]

Ah, but in PrerenderTabHelper::DidStopLoading(), the test that controls is on
pplt_load_start_, which is usually not null. Now, in this case,
actual_load_start_ will be null, so the fraction_plt_elapsed_at_swap_in is set
to -1.0, which is a proxy here for NaN, and the first test in that function
bails out.

It's easier to read with the test here, so I'm adding it. Thanks.

     if (prerender_manager->local_predictor_.get()) {
       prerender_manager->local_predictor_->
@@ -530,6 +544,15 @@ void PrerenderManager::RecordPerceivedPageLoadTime(
   }
 }
 
+void PrerenderManager::RecordFractionPixelsFinalAtSwapin(
+    content::WebContents* web_contents,
+    double fraction) {
+  const Origin* origin = NULL;
+  IsWebContentsPrerendered(web_contents, &origin);
+  histograms_->RecordFractionPixelsFinalAtSwapin(
+      origin ? *origin : ORIGIN_NONE, fraction);
+}
+
 void PrerenderManager::set_enabled(bool enabled) {
   DCHECK(CalledOnValidThread());
   enabled_ = enabled;
@@ -636,9 +659,13 @@ PrerenderContents* PrerenderManager::GetPrerenderContents(
   return NULL;
 }
 
-void PrerenderManager::MarkWebContentsAsPrerendered(WebContents* web_contents) {
+void PrerenderManager::MarkWebContentsAsPrerendered(WebContents* web_contents,
+                                                    Origin origin) {
   DCHECK(CalledOnValidThread());
-  prerendered_tab_contents_set_.insert(web_contents);
+  prerendered_web_contents_data_.insert(
+      base::hash_map<content::WebContents*,
+                     PrerenderedWebContentsData>::value_type(
+                         web_contents, PrerenderedWebContentsData(origin)));
 }
 
 void PrerenderManager::MarkWebContentsAsWouldBePrerendered(
@@ -650,7 +677,7 @@ void PrerenderManager::MarkWebContentsAsWouldBePrerendered(
 void PrerenderManager::MarkWebContentsAsNotPrerendered(
     WebContents* web_contents) {
   DCHECK(CalledOnValidThread());
-  prerendered_tab_contents_set_.erase(web_contents);
+  prerendered_web_contents_data_.erase(web_contents);
   WouldBePrerenderedMap::iterator it =
       would_be_prerendered_map_.find(web_contents);
   if (it != would_be_prerendered_map_.end()) {
@@ -664,9 +691,19 @@ void PrerenderManager::MarkWebContentsAsNotPrerendered(
 }
 
 bool PrerenderManager::IsWebContentsPrerendered(
-    content::WebContents* web_contents) const {
+    content::WebContents* web_contents,
+    const Origin** origin) const {
   DCHECK(CalledOnValidThread());
-  return prerendered_tab_contents_set_.count(web_contents) > 0;
+  base::hash_map<content::WebContents*, PrerenderedWebContentsData>::
+      const_iterator it = prerendered_web_contents_data_.find(web_contents);
+  if (it == prerendered_web_contents_data_.end()) {
+    if (origin)
+      *origin = NULL;
+    return false;
+  }
+  if (origin)
+    *origin = &it->second.origin;
+  return true;
 }
 
 bool PrerenderManager::WouldWebContentsBePrerendered(
@@ -675,7 +712,8 @@ bool PrerenderManager::WouldWebContentsBePrerendered(
   return would_be_prerendered_map_.count(web_contents) > 0;
 }
 
-bool PrerenderManager::HasRecentlyBeenNavigatedTo(const GURL& url) {
+bool PrerenderManager::HasRecentlyBeenNavigatedTo(Origin origin,
+                                                  const GURL& url) {
   DCHECK(CalledOnValidThread());
 
   CleanUpOldNavigations();
@@ -684,9 +722,9 @@ bool PrerenderManager::HasRecentlyBeenNavigatedTo(const GURL& url) {
            navigations_.rbegin();
        it != end;
        ++it) {
-    if (it->url_ == url) {
-      base::TimeDelta delta = GetCurrentTimeTicks() - it->time_;
-      histograms_->RecordTimeSinceLastRecentVisit(delta);
+    if (it->url == url) {
+      base::TimeDelta delta = GetCurrentTimeTicks() - it->time;
+      histograms_->RecordTimeSinceLastRecentVisit(origin, delta);
       return true;
     }
   }
@@ -968,7 +1006,7 @@ PrerenderHandle* PrerenderManager::AddPrerender(
 #endif
 
   // Check if enough time has passed since the last prerender.
-  if (!DoesRateLimitAllowPrerender()) {
+  if (!DoesRateLimitAllowPrerender(origin)) {
     // Cancel the prerender. We could add it to the pending prerender list but
     // this doesn't make sense as the next prerender request will be triggered
     // by a navigation and is unlikely to be the same site.
@@ -1171,11 +1209,11 @@ PrerenderManager::FindIteratorForPrerenderContents(
   return active_prerender_list_.end();
 }
 
-bool PrerenderManager::DoesRateLimitAllowPrerender() const {
+bool PrerenderManager::DoesRateLimitAllowPrerender(Origin origin) const {
   DCHECK(CalledOnValidThread());
   base::TimeDelta elapsed_time =
       GetCurrentTimeTicks() - last_prerender_start_time_;
-  histograms_->RecordTimeBetweenPrerenderRequests(elapsed_time);
+  histograms_->RecordTimeBetweenPrerenderRequests(origin, elapsed_time);
   if (!config_.rate_limit_enabled)
     return true;
   return elapsed_time >
@@ -1198,7 +1236,7 @@ void PrerenderManager::CleanUpOldNavigations() {
   base::TimeTicks cutoff = GetCurrentTimeTicks() -
       base::TimeDelta::FromMilliseconds(kNavigationRecordWindowMs);
   while (!navigations_.empty()) {
-    if (navigations_.front().time_ > cutoff)
+    if (navigations_.front().time > cutoff)
       break;
     navigations_.pop_front();
   }