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

chrome/renderer/page_load_histograms.cc

 // Copyright (c) 2010 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/renderer/page_load_histograms.h"
 
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/time.h"
@@ skipping 88 matching lines @@
   missing_start_type |= commit.is_null() ? COMMIT_MISSING : 0;
   missing_start_type |= navigation_start.is_null() ? NAV_START_MISSING : 0;
   UMA_HISTOGRAM_ENUMERATION("PLT.MissingStart", missing_start_type,
                             MISSING_START_TYPE_MAX);
   if (missing_start_type)
     return;
 
   // 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();
   Time first_paint = navigation_state->first_paint_time();
   Time first_paint_after_load = navigation_state->first_paint_after_load_time();
   Time finish_doc = navigation_state->finish_document_load_time();
   Time finish_all_loads = navigation_state->finish_load_time();
 
   // TODO(tonyg, jar): We suspect a bug in abandonment counting, this temporary
   // historgram should help us to troubleshoot.
@@ skipping 614 matching lines @@
 
   DCHECK(state);
   DCHECK(ds);
   GURL url(ds->request().url());
   Time start = state->start_load_time();
   Time finish = state->finish_load_time();
   // TODO(mbelshe): should we log more stats?
   VLOG(1) << "PLT: " << (finish - start).InMilliseconds() << "ms "
           << url.spec();
 }