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

1 files changed, 36 insertions, 21 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_;