Don't add to LoadStart and LoadEnd histograms if load event hasn't fired yet.

chrome/renderer/page_load_histograms.cc

Index: chrome/renderer/page_load_histograms.cc
diff --git a/chrome/renderer/page_load_histograms.cc b/chrome/renderer/page_load_histograms.cc
index 0fccbd1ae41c1bf115638c76f1fe4a2e365aeed9..a860783ecfeb378fce4a3522ec339edb958cf136 100644
--- a/chrome/renderer/page_load_histograms.cc
+++ b/chrome/renderer/page_load_histograms.cc
@@ -106,10 +106,12 @@ void PageLoadHistograms::Dump(WebFrame* frame) {
   // Record the new PLT times prior to the faulty abandon check below.
   // TODO(tonyg): There are many new details we can record, add them after the
   // basic metrics are evaluated.
-  TimeDelta nav_start_to_load_start = load_event_start - navigation_start;
-  TimeDelta nav_start_to_load_end = load_event_end - navigation_start;
-  PLT_HISTOGRAM("PLT.NavStartToLoadStart", nav_start_to_load_start);
-  PLT_HISTOGRAM("PLT.NavStartToLoadEnd", nav_start_to_load_end);
+  if (!load_event_start.is_null() && !load_event_end.is_null()) {
+    TimeDelta nav_start_to_load_start = load_event_start - navigation_start;
+    TimeDelta nav_start_to_load_end = load_event_end - navigation_start;
+    PLT_HISTOGRAM("PLT.NavStartToLoadStart", nav_start_to_load_start);
+    PLT_HISTOGRAM("PLT.NavStartToLoadEnd", nav_start_to_load_end);
+  }

[jar (doing other things), 2010/11/29 23:05:40]

I think I'd like TonyG to review this, as care needs to be taken to get data,
and be able to normalize it relative to other count of events.

One basic question: Why don't you test for each condition separately, and record
the histogram for the one(s) that are valid?

Is there a reason to only have both, or neither?

Also, given that we don't ever use these intermediate values (on lines 110 and
111) ever again, does it really add clarity to declare them, or should we just
inline them (and I think it would be pretty readable).

[tonyg, 2010/12/04 01:34:39]

I agree this is the right way to report client side redirects. I'd just like to
verify that after this fix, we record a single histogram that measures from the
initial navigation start to the load of the redirect target. There isn't much of
a precedent for unittesting these metrics, but if you feel so inclined it would
be really nice to verify w/ a test.

One thing to note is that the old code seems to count the redirect as two loads,
but I can't tell from inspection exactly how it would behave. The AbandonType
histogram below should give us the information to have insight into this case.
Seems slightly more correct to test for both separately.

 
   // We properly handle null values for the next 3 variables.
   Time request = navigation_state->request_time();