Clean up PLT histograms for a move to navigator_state.cc

Clean up PLT gathering in preparation for moving it
to take place in navigation_state.cc, during the
instance destructor.  This CL should have almost
no semantic impact, and I'm submitting it separately
so that when I move large blocks, I can mostly move
them unchanged.

Add a bunch of DCHECKs to be sure that finish and finish_doc
times are only set once, and that they are set only in
the correct order.

In a future CL, I expect to push a lot of the time-gathering
code into navigation_state.cc, along with the generation
of the final histograms.  The challenge is to mark the
NavigationState instances that are worthy of recording
(which looks like it MAY be done by marking the state
with the boolean to indicate it is part of an HTTP or
HTTPS connection), and make sure we have good numbers 
throughout.

bug=48970
r=mbelshe
Review URL: http://codereview.chromium.org/2993004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56164 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 166 insertions, 107 deletions

diff --git a/chrome/renderer/navigation_state.h b/chrome/renderer/navigation_state.h
index afdebb8..7b3d8d4 100644
--- a/chrome/renderer/navigation_state.h
+++ b/chrome/renderer/navigation_state.h
@@ -6,8 +6,11 @@
 #define CHROME_RENDERER_NAVIGATION_STATE_H_
 #pragma once
 
+#include <string>
+
 #include "base/scoped_ptr.h"
 #include "base/time.h"
+#include "chrome/common/extensions/url_pattern.h"
 #include "chrome/common/page_transition_types.h"
 #include "chrome/renderer/user_script_idle_scheduler.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebDataSource.h"
@@ -32,6 +35,9 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     kLoadTypeMax               // Bounding value for this enum.
   };
 
+  virtual ~NavigationState() {
+  }
+
   static NavigationState* CreateBrowserInitiated(
       int32 pending_page_id,
       int pending_history_list_offset,
@@ -84,6 +90,7 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     return request_time_;
   }
   void set_request_time(const base::Time& value) {
+    DCHECK(start_load_time_.is_null());
     request_time_ = value;
   }
 
@@ -92,6 +99,9 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     return start_load_time_;
   }
   void set_start_load_time(const base::Time& value) {
+    // TODO(jar): This should not be set twice.
+    // DCHECK(!start_load_time_.is_null());
+    DCHECK(finish_document_load_time_.is_null());
     start_load_time_ = value;
   }
 
@@ -108,21 +118,28 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     return finish_document_load_time_;
   }
   void set_finish_document_load_time(const base::Time& value) {
+    // TODO(jar): Some unittests break the following DCHECK, and don't have
+    // DCHECK(!start_load_time_.is_null());
+    DCHECK(!value.is_null());
+    // TODO(jar): Double setting does happen, but probably shouldn't.
+    // DCHECK(finish_document_load_time_.is_null());
+    // TODO(jar): We should guarantee this order :-(.
+    // DCHECK(finish_load_time_.is_null());
     finish_document_load_time_ = value;
   }
 
   // The time that the document and all subresources finished loading.
-  const base::Time& finish_load_time() const {
-    return finish_load_time_;
-  }
+  const base::Time& finish_load_time() const { return finish_load_time_; }
   void set_finish_load_time(const base::Time& value) {
+    DCHECK(!value.is_null());
+    DCHECK(finish_load_time_.is_null());
+    // The following is not already set in all cases :-(
+    // DCHECK(!finish_document_load_time_.is_null());
     finish_load_time_ = value;
   }
 
   // The time that painting first happened after a new navigation.
-  const base::Time& first_paint_time() const {
-    return first_paint_time_;
-  }
+  const base::Time& first_paint_time() const { return first_paint_time_; }
   void set_first_paint_time(const base::Time& value) {
     first_paint_time_ = value;
   }
@@ -135,10 +152,14 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     first_paint_after_load_time_ = value;
   }
 
-  // True iff the histograms for the associated frame have been dumped.
-  bool load_histograms_recorded() const {
-    return load_histograms_recorded_;
+  // Info about the URL used as the target of this navigation.
+  URLPattern::SchemeMasks scheme_type() const { return scheme_type_; }
+  void set_scheme_type(URLPattern::SchemeMasks scheme_type) {
+    scheme_type_ = scheme_type;
   }
+
+  // True iff the histograms for the associated frame have been dumped.
+  bool load_histograms_recorded() const { return load_histograms_recorded_; }
   void set_load_histograms_recorded(bool value) {
     load_histograms_recorded_ = value;
   }
@@ -174,29 +195,21 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
     alt_error_page_fetcher_.reset(f);
   }
 
-  const std::string& security_info() const {
-    return security_info_;
-  }
+  const std::string& security_info() const { return security_info_; }
   void set_security_info(const std::string& security_info) {
     security_info_ = security_info;
   }
 
-  bool postpone_loading_data() const {
-    return postpone_loading_data_;
-  }
+  bool postpone_loading_data() const { return postpone_loading_data_; }
   void set_postpone_loading_data(bool postpone_loading_data) {
     postpone_loading_data_ = postpone_loading_data;
   }
 
-  void clear_postponed_data() {
-    postponed_data_.clear();
-  }
+  const std::string& postponed_data() const { return postponed_data_; }
+  void clear_postponed_data() { postponed_data_.clear(); }
   void append_postponed_data(const char* data, size_t data_len) {
     postponed_data_.append(data, data_len);
   }
-  const std::string& postponed_data() const {
-    return postponed_data_;
-  }
 
   // Sets the cache policy. The cache policy is only used if explicitly set and
   // by default is not set. You can mark a NavigationState as not having a cache
@@ -259,6 +272,7 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
       : transition_type_(transition_type),
         load_type_(UNDEFINED_LOAD),
         request_time_(request_time),
+        scheme_type_(static_cast<URLPattern::SchemeMasks>(0)),
         load_histograms_recorded_(false),
         request_committed_(false),
         is_content_initiated_(is_content_initiated),
@@ -286,6 +300,7 @@ class NavigationState : public WebKit::WebDataSource::ExtraData {
   base::Time finish_load_time_;
   base::Time first_paint_time_;
   base::Time first_paint_after_load_time_;
+  URLPattern::SchemeMasks scheme_type_;
   bool load_histograms_recorded_;
   bool request_committed_;
   bool is_content_initiated_;
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 8ef8adb..5ff171d 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -2418,14 +2418,21 @@ void RenderView::frameDetached(WebFrame* frame) {
 }
 
 void RenderView::willClose(WebFrame* frame) {
+  WebDataSource* ds = frame->dataSource();
+  NavigationState* navigation_state = NavigationState::FromDataSource(ds);
+
   if (!frame->parent()) {
     const GURL& url = frame->url();
-    if (url.SchemeIs("http") || url.SchemeIs("https"))
-      DumpLoadHistograms();
+    // Ensure state contains scheme.
+    if (url.SchemeIs("http"))
+      navigation_state->set_scheme_type(URLPattern::SCHEME_HTTP);
+    else if (url.SchemeIs("https"))
+      navigation_state->set_scheme_type(URLPattern::SCHEME_HTTPS);
+
+    // Dump will only be provided when scheme is http or https.
+    DumpLoadHistograms();
   }
 
-  WebDataSource* ds = frame->dataSource();
-  NavigationState* navigation_state = NavigationState::FromDataSource(ds);
   navigation_state->user_script_idle_scheduler()->Cancel();
 
   // TODO(jhawkins): Remove once frameDetached is called by WebKit.
@@ -2685,9 +2692,6 @@ void RenderView::didCreateDataSource(WebFrame* frame, WebDataSource* ds) {
       NavigationState::CreateContentInitiated() :
       pending_navigation_state_.release();
 
-  const WebURLRequest& original_request = ds->originalRequest();
-  const WebURLRequest& request = ds->request();
-
   // NavigationState::referred_by_prefetcher_ is true if we are
   // navigating from a page that used prefetching using a link on that
   // page.  We are early enough in the request process here that we
@@ -2697,6 +2701,7 @@ void RenderView::didCreateDataSource(WebFrame* frame, WebDataSource* ds) {
   // renderer process.
   if (webview()) {
     if (WebFrame* old_frame = webview()->mainFrame()) {
+      const WebURLRequest& original_request = ds->originalRequest();
       const GURL referrer(
           original_request.httpHeaderField(WebString::fromUTF8("Referer")));
       if (!referrer.is_empty() &&
@@ -2714,6 +2719,7 @@ void RenderView::didCreateDataSource(WebFrame* frame, WebDataSource* ds) {
   }
 
   if (content_initiated) {
+    const WebURLRequest& request = ds->request();
     switch (request.cachePolicy()) {
       case WebURLRequest::UseProtocolCachePolicy:  // normal load.
         state->set_load_type(NavigationState::LINK_LOAD_NORMAL);
@@ -2739,8 +2745,6 @@ void RenderView::didStartProvisionalLoad(WebFrame* frame) {
   WebDataSource* ds = frame->provisionalDataSource();
   NavigationState* navigation_state = NavigationState::FromDataSource(ds);
 
-  navigation_state->set_start_load_time(Time::Now());
-
   // Update the request time if WebKit has better knowledge of it.
   if (navigation_state->request_time().is_null()) {
     double event_time = ds->triggeringEventTime();
@@ -2748,6 +2752,9 @@ void RenderView::didStartProvisionalLoad(WebFrame* frame) {
       navigation_state->set_request_time(Time::FromDoubleT(event_time));
   }
 
+  // Start time is only set after request time.
+  navigation_state->set_start_load_time(Time::Now());
+
   bool is_top_most = !frame->parent();
   if (is_top_most) {
     navigation_gesture_ = frame->isProcessingUserGesture() ?
@@ -4300,9 +4307,17 @@ void RenderView::OnClosePage(const ViewMsg_ClosePage_Params& params) {
     // TODO(davemoore) this code should be removed once willClose() gets
     // called when a page is destroyed. DumpLoadHistograms() is safe to call
     // multiple times for the same frame, but it will simplify things.
-    if (url.SchemeIs(chrome::kHttpScheme) ||
-        url.SchemeIs(chrome::kHttpsScheme))
-      DumpLoadHistograms();
+
+    // Ensure state contains scheme.
+    NavigationState* navigation_state =
+        NavigationState::FromDataSource(main_frame->dataSource());
+    if (url.SchemeIs("http"))
+      navigation_state->set_scheme_type(URLPattern::SCHEME_HTTP);
+    else if (url.SchemeIs("https"))
+      navigation_state->set_scheme_type(URLPattern::SCHEME_HTTPS);
+
+    // Dump will only be provided when scheme is http or https.
+    DumpLoadHistograms();
   }
   webview()->dispatchUnloadEvent();
 
@@ -4488,6 +4503,8 @@ void RenderView::DidFlushPaint() {
     NavigationState* navigation_state = NavigationState::FromDataSource(ds);
     DCHECK(navigation_state);
 
+    // TODO(jar): The following code should all be inside a method, probably in
+    // NavigatorState.
     Time now = Time::Now();
     if (navigation_state->first_paint_time().is_null()) {
       navigation_state->set_first_paint_time(now);
@@ -4581,7 +4598,7 @@ void RenderView::OnExtensionMessageInvoke(const std::string& function_name,
 //    start: time load of document started
 //    commit: time load of document started
 //    finish_document: main document loaded, before onload()
-//    finish: after onload() and all resources are loaded
+//    finish_all_loads: after onload() and all resources are loaded
 //    first_paint: first paint performed
 //    first_paint_after_load: first paint performed after load is finished
 //    begin: request if it was user requested, start otherwise
@@ -4602,6 +4619,12 @@ void RenderView::DumpLoadHistograms() const {
   NavigationState* navigation_state =
       NavigationState::FromDataSource(main_frame->dataSource());
 
+  URLPattern::SchemeMasks scheme_type = navigation_state->scheme_type();
+  if (scheme_type == 0)
+     return;
+  DCHECK(scheme_type == URLPattern::SCHEME_HTTP ||
+         scheme_type == URLPattern::SCHEME_HTTPS);
+
   // If we've already dumped, do nothing.
   if (navigation_state->load_histograms_recorded())
     return;
@@ -4618,47 +4641,57 @@ void RenderView::DumpLoadHistograms() const {
   Time request = navigation_state->request_time();
   Time first_paint = navigation_state->first_paint_time();
   Time first_paint_after_load = navigation_state->first_paint_after_load_time();
-  Time finish = navigation_state->finish_load_time();
   Time finish_doc = navigation_state->finish_document_load_time();
+  Time finish_all_loads = navigation_state->finish_load_time();
 
   // Handle case where user hits "stop" or "back" before loading completely.
-  bool abandoned_page = finish.is_null();
+  bool abandoned_page = finish_doc.is_null();
   if (abandoned_page) {
-    finish = Time::Now();
-    navigation_state->set_finish_load_time(finish);
+    finish_doc = Time::Now();
+    navigation_state->set_finish_document_load_time(finish_doc);
   }
 
-  if (finish_doc.is_null()) {
-    DCHECK(abandoned_page);  // how can the doc have finished but not the page?
+  // TODO(jar): We should really discriminate the definition of "abandon" more
+  // finely.  We should have:
+  // abandon_before_document_loaded
+  // abandon_before_onload_fired
+
+  if (finish_all_loads.is_null()) {
+    finish_all_loads = Time::Now();
+    navigation_state->set_finish_load_time(finish_all_loads);
+  } else {
+    DCHECK(!abandoned_page);  // How can the doc have finished but not the page?
     if (!abandoned_page)
       return;  // Don't try to record a stat which is broken.
-    finish_doc = finish;
-    navigation_state->set_finish_document_load_time(finish_doc);
   }
 
   // Note: Client side redirects will have no request time.
   Time begin = request.is_null() ? start : request;
   TimeDelta begin_to_finish_doc = finish_doc - begin;
-  TimeDelta begin_to_finish = finish - begin;
-  TimeDelta start_to_finish = finish - start;
+  TimeDelta begin_to_finish_all_loads = finish_all_loads - begin;
+  TimeDelta start_to_finish_all_loads = finish_all_loads - start;
   TimeDelta start_to_commit = commit - start;
 
   NavigationState::LoadType load_type = navigation_state->load_type();
 
+  // The above code sanitized all values of times, in preparation for creating
+  // actual histograms.  The remainder of this code could be run at destructor
+  // time for the navigation_state, since all data is intact.
+
   // Aggregate PLT data across all link types.
   UMA_HISTOGRAM_ENUMERATION("PLT.Abandoned", abandoned_page ? 1 : 0, 2);
   UMA_HISTOGRAM_ENUMERATION("PLT.LoadType", load_type,
       NavigationState::kLoadTypeMax);
   PLT_HISTOGRAM("PLT.StartToCommit", start_to_commit);
   PLT_HISTOGRAM("PLT.CommitToFinishDoc", finish_doc - commit);
-  PLT_HISTOGRAM("PLT.FinishDocToFinish", finish - finish_doc);
+  PLT_HISTOGRAM("PLT.FinishDocToFinish", finish_all_loads - finish_doc);
   PLT_HISTOGRAM("PLT.BeginToCommit", commit - begin);
-  PLT_HISTOGRAM("PLT.StartToFinish", start_to_finish);
+  PLT_HISTOGRAM("PLT.StartToFinish", start_to_finish_all_loads);
   if (!request.is_null()) {
     PLT_HISTOGRAM("PLT.RequestToStart", start - request);
-    PLT_HISTOGRAM("PLT.RequestToFinish", finish - request);
+    PLT_HISTOGRAM("PLT.RequestToFinish", finish_all_loads - request);
   }
-  PLT_HISTOGRAM("PLT.CommitToFinish", finish - commit);
+  PLT_HISTOGRAM("PLT.CommitToFinish", finish_all_loads - commit);
   if (!first_paint.is_null()) {
     DCHECK(begin <= first_paint);
     PLT_HISTOGRAM("PLT.BeginToFirstPaint", first_paint - begin);
@@ -4672,50 +4705,54 @@ void RenderView::DumpLoadHistograms() const {
     DCHECK(commit <= first_paint_after_load);
     PLT_HISTOGRAM("PLT.CommitToFirstPaintAfterLoad",
         first_paint_after_load - commit);
-    DCHECK(finish <= first_paint_after_load);
+    DCHECK(finish_all_loads <= first_paint_after_load);
     PLT_HISTOGRAM("PLT.FinishToFirstPaintAfterLoad",
-        first_paint_after_load - finish);
+        first_paint_after_load - finish_all_loads);
   }
   PLT_HISTOGRAM("PLT.BeginToFinishDoc", begin_to_finish_doc);
-  PLT_HISTOGRAM("PLT.BeginToFinish", begin_to_finish);
+  PLT_HISTOGRAM("PLT.BeginToFinish", begin_to_finish_all_loads);
 
   // Load type related histograms.
   switch (load_type) {
     case NavigationState::UNDEFINED_LOAD:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_UndefLoad", begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_UndefLoad", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_UndefLoad", begin_to_finish_all_loads);
       break;
     case NavigationState::RELOAD:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_Reload", begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_Reload", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_Reload", begin_to_finish_all_loads);
       break;
     case NavigationState::HISTORY_LOAD:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_HistoryLoad", begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_HistoryLoad", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_HistoryLoad", begin_to_finish_all_loads);
       break;
     case NavigationState::NORMAL_LOAD:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_NormalLoad", begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_NormalLoad", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_NormalLoad", begin_to_finish_all_loads);
       break;
     case NavigationState::LINK_LOAD_NORMAL:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_LinkLoadNormal",
           begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadNormal", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadNormal",
+                    begin_to_finish_all_loads);
       break;
     case NavigationState::LINK_LOAD_RELOAD:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_LinkLoadReload",
            begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadReload", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadReload",
+                    begin_to_finish_all_loads);
       break;
     case NavigationState::LINK_LOAD_CACHE_STALE_OK:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_LinkLoadStaleOk",
            begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadStaleOk", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadStaleOk",
+                    begin_to_finish_all_loads);
       break;
     case NavigationState::LINK_LOAD_CACHE_ONLY:
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_LinkLoadCacheOnly",
            begin_to_finish_doc);
-      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadCacheOnly", begin_to_finish);
+      PLT_HISTOGRAM("PLT.BeginToFinish_LinkLoadCacheOnly",
+                    begin_to_finish_all_loads);
       break;
     default:
       break;
@@ -4734,22 +4771,23 @@ void RenderView::DumpLoadHistograms() const {
     switch (load_type) {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
-            "PLT.BeginToFinish_NormalLoad", "DnsImpact"), begin_to_finish);
+            "PLT.BeginToFinish_NormalLoad", "DnsImpact"),
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal", "DnsImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_RELOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadReload", "DnsImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_STALE_OK:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadStaleOk", "DnsImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       default:
         break;
@@ -4768,7 +4806,7 @@ void RenderView::DumpLoadHistograms() const {
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_ContentPrefetcher",
                     begin_to_finish_doc);
       PLT_HISTOGRAM("PLT.BeginToFinish_ContentPrefetcher",
-                    begin_to_finish);
+                    begin_to_finish_all_loads);
     }
     if (prefetching_fieldtrial) {
       PLT_HISTOGRAM(
@@ -4778,7 +4816,7 @@ void RenderView::DumpLoadHistograms() const {
       PLT_HISTOGRAM(
           FieldTrial::MakeName("PLT.BeginToFinish_ContentPrefetcher",
                                "Prefetch"),
-          begin_to_finish);
+          begin_to_finish_all_loads);
     }
   }
   if (navigation_state->was_referred_by_prefetcher()) {
@@ -4786,7 +4824,7 @@ void RenderView::DumpLoadHistograms() const {
       PLT_HISTOGRAM("PLT.BeginToFinishDoc_ContentPrefetcherReferrer",
                     begin_to_finish_doc);
       PLT_HISTOGRAM("PLT.BeginToFinish_ContentPrefetcherReferrer",
-                    begin_to_finish);
+                    begin_to_finish_all_loads);
     }
     if (prefetching_fieldtrial) {
       PLT_HISTOGRAM(
@@ -4796,7 +4834,7 @@ void RenderView::DumpLoadHistograms() const {
       PLT_HISTOGRAM(
           FieldTrial::MakeName("PLT.BeginToFinish_ContentPrefetcherReferrer",
                                "Prefetch"),
-          begin_to_finish);
+          begin_to_finish_all_loads);
     }
   }
   if (prefetching_fieldtrial) {
@@ -4805,7 +4843,7 @@ void RenderView::DumpLoadHistograms() const {
     PLT_HISTOGRAM(FieldTrial::MakeName("PLT.BeginToFinishDoc", "Prefetch"),
                   begin_to_finish_doc);
     PLT_HISTOGRAM(FieldTrial::MakeName("PLT.BeginToFinish", "Prefetch"),
-                  begin_to_finish);
+                  begin_to_finish_all_loads);
   }
 
   // Histograms to determine if the number of connections has an
@@ -4823,22 +4861,22 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_NormalLoad", "ConnCountImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal", "ConnCountImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_RELOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadReload", "ConnCountImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_STALE_OK:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadStaleOk", "ConnCountImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       default:
         break;
@@ -4857,22 +4895,22 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_NormalLoad", "IdleSktToImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal", "IdleSktToImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_RELOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadReload", "IdleSktToImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_STALE_OK:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadStaleOk", "IdleSktToImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       default:
         break;
@@ -4891,22 +4929,22 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_NormalLoad", "ProxyConnectionImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal", "ProxyConnectionImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_RELOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadReload", "ProxyConnectionImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_STALE_OK:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadStaleOk", "ProxyConnectionImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       default:
         break;
@@ -4925,27 +4963,27 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_NormalLoad", "GlobalSdch"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal", "GlobalSdch"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_RELOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadReload", "GlobalSdch"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_STALE_OK:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadStaleOk", "GlobalSdch"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       case NavigationState::LINK_LOAD_CACHE_ONLY:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadCacheOnly", "GlobalSdch"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         break;
       default:
         break;
@@ -4964,14 +5002,13 @@ void RenderView::DumpLoadHistograms() const {
   }
 
 
-  GURL url = GURL(main_frame->url());
-
   // Histograms to determine if SPDY has an impact for https traffic.
   // TODO(mbelshe): After we've seen the difference between BeginToFinish
   //                and StartToFinish, consider removing one or the other.
   static bool use_spdy_histogram(FieldTrialList::Find("SpdyImpact") &&
       !FieldTrialList::Find("SpdyImpact")->group_name().empty());
-  if (use_spdy_histogram && url.SchemeIs("https") &&
+  if (use_spdy_histogram &&
+      scheme_type == URLPattern::SCHEME_HTTPS &&
       navigation_state->was_npn_negotiated()) {
     UMA_HISTOGRAM_ENUMERATION(
         FieldTrial::MakeName("PLT.Abandoned", "SpdyImpact"),
@@ -4980,10 +5017,10 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::LINK_LOAD_NORMAL:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_LinkLoadNormal_SpdyTrial", "SpdyImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.StartToFinish_LinkLoadNormal_SpdyTrial", "SpdyImpact"),
-            start_to_finish);
+            start_to_finish_all_loads);
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.StartToCommit_LinkLoadNormal_SpdyTrial", "SpdyImpact"),
             start_to_commit);
@@ -4991,10 +5028,10 @@ void RenderView::DumpLoadHistograms() const {
       case NavigationState::NORMAL_LOAD:
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.BeginToFinish_NormalLoad_SpdyTrial", "SpdyImpact"),
-            begin_to_finish);
+            begin_to_finish_all_loads);
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.StartToFinish_NormalLoad_SpdyTrial", "SpdyImpact"),
-            start_to_finish);
+            start_to_finish_all_loads);
         PLT_HISTOGRAM(FieldTrial::MakeName(
             "PLT.StartToCommit_NormalLoad_SpdyTrial", "SpdyImpact"),
             start_to_commit);
@@ -5006,7 +5043,7 @@ void RenderView::DumpLoadHistograms() const {
 
   // Histograms to compare the impact of alternate protocol over http traffic:
   // when spdy is used vs. when http is used.
-  if (url.SchemeIs("http") &&
+  if (scheme_type == URLPattern::SCHEME_HTTP &&
       navigation_state->was_alternate_protocol_available()) {
     if (!navigation_state->was_npn_negotiated()) {
       // This means that even there is alternate protocols for npn_http or
@@ -5015,7 +5052,7 @@ void RenderView::DumpLoadHistograms() const {
         case NavigationState::LINK_LOAD_NORMAL:
           PLT_HISTOGRAM(
               "PLT.StartToFinish_LinkLoadNormal_AlternateProtocol_http",
-              start_to_finish);
+              start_to_finish_all_loads);
           PLT_HISTOGRAM(
               "PLT.StartToCommit_LinkLoadNormal_AlternateProtocol_http",
               start_to_commit);
@@ -5023,7 +5060,7 @@ void RenderView::DumpLoadHistograms() const {
         case NavigationState::NORMAL_LOAD:
           PLT_HISTOGRAM(
               "PLT.StartToFinish_NormalLoad_AlternateProtocol_http",
-              start_to_finish);
+              start_to_finish_all_loads);
           PLT_HISTOGRAM(
               "PLT.StartToCommit_NormalLoad_AlternateProtocol_http",
               start_to_commit);
@@ -5036,7 +5073,7 @@ void RenderView::DumpLoadHistograms() const {
         case NavigationState::LINK_LOAD_NORMAL:
           PLT_HISTOGRAM(
               "PLT.StartToFinish_LinkLoadNormal_AlternateProtocol_spdy",
-              start_to_finish);
+              start_to_finish_all_loads);
           PLT_HISTOGRAM(
               "PLT.StartToCommit_LinkLoadNormal_AlternateProtocol_spdy",
               start_to_commit);
@@ -5044,7 +5081,7 @@ void RenderView::DumpLoadHistograms() const {
         case NavigationState::NORMAL_LOAD:
           PLT_HISTOGRAM(
               "PLT.StartToFinish_NormalLoad_AlternateProtocol_spdy",
-              start_to_finish);
+              start_to_finish_all_loads);
           PLT_HISTOGRAM(
               "PLT.StartToCommit_NormalLoad_AlternateProtocol_spdy",
               start_to_commit);
@@ -5057,24 +5094,28 @@ void RenderView::DumpLoadHistograms() const {
 
   // Record page load time and abandonment rates for proxy cases.
   if (navigation_state->was_fetched_via_proxy()) {
-    if (url.SchemeIs("https")) {
-      PLT_HISTOGRAM("PLT.StartToFinish.Proxy.https", start_to_finish);
+    if (scheme_type == URLPattern::SCHEME_HTTPS) {
+      PLT_HISTOGRAM("PLT.StartToFinish.Proxy.https", start_to_finish_all_loads);
       UMA_HISTOGRAM_ENUMERATION("PLT.Abandoned.Proxy.https",
-          abandoned_page ? 1 : 0, 2);
+                                abandoned_page ? 1 : 0, 2);
     } else {
-      PLT_HISTOGRAM("PLT.StartToFinish.Proxy.http", start_to_finish);
+      DCHECK(scheme_type == URLPattern::SCHEME_HTTP);
+      PLT_HISTOGRAM("PLT.StartToFinish.Proxy.http", start_to_finish_all_loads);
       UMA_HISTOGRAM_ENUMERATION("PLT.Abandoned.Proxy.http",
-          abandoned_page ? 1 : 0, 2);
+                                abandoned_page ? 1 : 0, 2);
     }
   } else {
-    if (url.SchemeIs("https")) {
-      PLT_HISTOGRAM("PLT.StartToFinish.NoProxy.https", start_to_finish);
+    if (scheme_type == URLPattern::SCHEME_HTTPS) {
+      PLT_HISTOGRAM("PLT.StartToFinish.NoProxy.https",
+                    start_to_finish_all_loads);
       UMA_HISTOGRAM_ENUMERATION("PLT.Abandoned.NoProxy.https",
-          abandoned_page ? 1 : 0, 2);
+                                abandoned_page ? 1 : 0, 2);
     } else {
-      PLT_HISTOGRAM("PLT.StartToFinish.NoProxy.http", start_to_finish);
+      DCHECK(scheme_type == URLPattern::SCHEME_HTTP);
+      PLT_HISTOGRAM("PLT.StartToFinish.NoProxy.http",
+                    start_to_finish_all_loads);
       UMA_HISTOGRAM_ENUMERATION("PLT.Abandoned.NoProxy.http",
-          abandoned_page ? 1 : 0, 2);
+                                abandoned_page ? 1 : 0, 2);
     }
   }
 
@@ -5084,6 +5125,9 @@ void RenderView::DumpLoadHistograms() const {
   UMA_HISTOGRAM_COUNTS("SiteIsolation.PageLoadsWithSameSiteFrameAccess",
                        same_origin_access_count_);
 
+  // TODO(jar): This is the ONLY call in this method that needed more than
+  // navigation_state.  This should be relocated, or changed, so that this
+  // body can be made a method on NavigationStates, and run via its destructor.
   // Log some PLT data to the error log.
   LogNavigationState(navigation_state, main_frame->dataSource());