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

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

BUG=64321
TEST=adhoc (visit http://www/~eisinger/redirect.html)

[James Simonsen, 2010-11-29 22:47:44]

I'm not a committer yet. Can you please land this if you're happy with it?

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

Please get input/agreement from TonyG.

thx!

http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
File chrome/renderer/page_load_histograms.cc (right):

http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
chrome/renderer/page_load_histograms.cc:114: }
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).

[James Simonsen, 2010-11-29 23:22:00]

On Mon, Nov 29, 2010 at 3:05 PM, <jar@chromium.org> wrote:

> Please get input/agreement from TonyG.
>

Alright, he'll be back sometime next week.


>
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> File chrome/renderer/page_load_histograms.cc (right):
>
>
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> chrome/renderer/page_load_histograms.cc:114: }
> 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).


No particular reason for any of these. I'll wait and see what Tony says
before uploading a new version.

James

[tonyg, 2010-12-04 01:34:38]

http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
File chrome/renderer/page_load_histograms.cc (right):

http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
chrome/renderer/page_load_histograms.cc:114: }
On 2010/11/29 23:05:40, jar wrote:
> 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.

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.

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

Seems slightly more correct to test for both separately.

[James Simonsen, 2010-12-08 04:00:44]

On 2010/12/04 01:34:38, tonyg wrote:
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> File chrome/renderer/page_load_histograms.cc (right):
> 
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> chrome/renderer/page_load_histograms.cc:114: }
> On 2010/11/29 23:05:40, jar wrote:
> > 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.
> 
> 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.

I tried to add a test case, but I couldn't find a way to make it stable, since
it's hard to synchronize all of the relevant events.

I manually checked the behavior. If a redirection happens before load fires,
then we only get one histogram entry. That entry starts from the redirect, not
from the initial navigation.

> 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.

navigation_state->request_time is null after the redirect, so the RequestTo...
timers will only count once. The other timers fall back to starting from
navigation_state->start_load_time in the redirect.

> > One basic question: Why don't you test for each condition separately, and
> record
> > the histogram for the one(s) that are valid?
> > 
> 
> Seems slightly more correct to test for both separately.

Done.

[tonyg, 2010-12-08 04:55:19]

LGTM

On Tue, Dec 7, 2010 at 8:00 PM,  <simonjam@chromium.org> wrote:
> On 2010/12/04 01:34:38, tonyg wrote:
>
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
>>
>> File chrome/renderer/page_load_histograms.cc (right):
>
>
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
>>
>> chrome/renderer/page_load_histograms.cc:114: }
>> On 2010/11/29 23:05:40, jar wrote:
>> > 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.
>
>> 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.
>
> I tried to add a test case, but I couldn't find a way to make it stable,
> since
> it's hard to synchronize all of the relevant events.
>
> I manually checked the behavior. If a redirection happens before load fires,
> then we only get one histogram entry. That entry starts from the redirect,
> not
> from the initial navigation.
>
>> 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.
>
> navigation_state->request_time is null after the redirect, so the
> RequestTo...
> timers will only count once. The other timers fall back to starting from
> navigation_state->start_load_time in the redirect.
>
>> > One basic question: Why don't you test for each condition separately,
>> > and
>> record
>> > the histogram for the one(s) that are valid?
>> >
>
>> Seems slightly more correct to test for both separately.
>
> Done.
>
> http://codereview.chromium.org/5382006/
>

[tonyg, 2010-12-14 01:19:34]

Landing this shortly.

On 2010/12/08 04:55:19, tonyg wrote:
> LGTM
> 
> On Tue, Dec 7, 2010 at 8:00 PM,  <mailto:simonjam@chromium.org> wrote:
> > On 2010/12/04 01:34:38, tonyg wrote:
> >
> >
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> >>
> >> File chrome/renderer/page_load_histograms.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/5382006/diff/1/chrome/renderer/page_load_histo...
> >>
> >> chrome/renderer/page_load_histograms.cc:114: }
> >> On 2010/11/29 23:05:40, jar wrote:
> >> > 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.
> >
> >> 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.
> >
> > I tried to add a test case, but I couldn't find a way to make it stable,
> > since
> > it's hard to synchronize all of the relevant events.
> >
> > I manually checked the behavior. If a redirection happens before load fires,
> > then we only get one histogram entry. That entry starts from the redirect,
> > not
> > from the initial navigation.
> >
> >> 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.
> >
> > navigation_state->request_time is null after the redirect, so the
> > RequestTo...
> > timers will only count once. The other timers fall back to starting from
> > navigation_state->start_load_time in the redirect.
> >
> >> > One basic question: Why don't you test for each condition separately,
> >> > and
> >> record
> >> > the histogram for the one(s) that are valid?
> >> >
> >
> >> Seems slightly more correct to test for both separately.
> >
> > Done.
> >
> > http://codereview.chromium.org/5382006/
> >

[commit-bot: I haz the power, 2011-01-03 02:34:53]

Patch failed to apply against 70108.