chrome.loadTimes() shouldn't be affected by in-document navigation.

chrome.loadTimes() shouldn't be affected by in-document navigation.

Data that's only pertinent to initially loading the document is stored in LoadTimes. These are carried over when the NavigationState changes due to in-document navigation.

I'd be interested in feedback on the position of LoadTimes. I'm not happy with it. I'd prefer to have it on WebDataSource, but I expect NavigationState was hidden in ExtraData for a reason.

This also impacts PLT histograms. In-document navigation is the cause of some of the missing start types. So, by fixing this, we should get more complete page load data. There are probably whole classes of sites where we have no PLT data, such as Google Docs.

BUG=79078
TEST=ui_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110392

[James Simonsen, 2011-10-27 19:06:58]

John, please review NavigationState and RenderViewImpl.

Jim, please review the PLT histogram changes.

[jam, 2011-10-27 20:39:53]

I really am not familiar with the details of that code, so all I can give you is
a rubberstamp lgtm for these files. I assuem that in navigation_state you just
moved the member variables only, plus the additional methods.

http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:2465: if
(load_times->commit_load_time().is_null()) {
nit: this file doesn't use brace brackets for one line statements

http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:2575: DCHECK(load_times);
nit: these dchecks, at least for load_times, aren't necessary. we know this will
never happen. if it ever does, a dcheck is not useful because it'll be compiled
away in release builds. in debug build, the crash in the next line is just as
good of a signal

[James Simonsen, 2011-10-27 21:18:40]

On 2011/10/27 20:39:53, John Abd-El-Malek wrote:
> I really am not familiar with the details of that code, so all I can give you
is
> a rubberstamp lgtm for these files.

Okay, let me rope in Darin. I saw his name throughout the code too.

> I assuem that in navigation_state you just
> moved the member variables only, plus the additional methods.

Yeah, it's just moving them around. I also got rid of was_translated() since
that was unused.

>
http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
> content/renderer/render_view_impl.cc:2465: if
> (load_times->commit_load_time().is_null()) {
> nit: this file doesn't use brace brackets for one line statements

Done.

http://codereview.chromium.org/8404018/diff/1/content/renderer/render_view_im...
> content/renderer/render_view_impl.cc:2575: DCHECK(load_times);
> nit: these dchecks, at least for load_times, aren't necessary. we know this
will
> never happen. if it ever does, a dcheck is not useful because it'll be
compiled
> away in release builds. in debug build, the crash in the next line is just as
> good of a signal

Done.

[darin (slow to review), 2011-10-27 23:31:13]

As discussed in person, I think we should probably introduce a DocumentState
object that is 1-1 with WebDataSource (i.e., only created in
didCreateDataSource).  DocumentState can have a NavigationState that gets
updated when we only perform in-document navigations.

Long-term, we should probably store NavigationState on the WebCore::HistoryItem.

[jar (doing other things), 2011-10-27 23:58:38]

PLT have been a nightmare.  If you have nailed this (finally), major kudos.  If
you have merely helped things, then thanks ;-).

In general, I think we can't figure out (for all sites) when a page has loaded
:-(.  When you get one class of site right, another is wrong.  Please carefully
run about:histograms and see if you are getting the stats you'd expect.  Be
warned, the requests are usually "one behind," as we don't aggregate page load
stats for one page, until that page is unloaded (and we're sure we're done with
it!).  As a result, you need to do 2 page loads, then update the about:histogram
page, and you'll see the result of the first page load.

LGTM for the PLT changes... but please validate that you are getting what you
expected, by hand.

[Paweł Hajdan Jr., 2011-10-28 08:29:34]

Drive-by, no need to wait for me.

http://codereview.chromium.org/8404018/diff/2003/chrome/renderer/loadtimes_ex...
File chrome/renderer/loadtimes_extension_bindings_uitest.cc (right):

http://codereview.chromium.org/8404018/diff/2003/chrome/renderer/loadtimes_ex...
chrome/renderer/loadtimes_extension_bindings_uitest.cc:39:
scoped_refptr<TabProxy> tab_proxy = GetActiveTab();
ASSERT_TRUE on this or you'll crash randomly.

http://codereview.chromium.org/8404018/diff/2003/chrome/renderer/loadtimes_ex...
chrome/renderer/loadtimes_extension_bindings_uitest.cc:55:
scoped_refptr<TabProxy> tab_proxy = GetActiveTab();
Same here.

[James Simonsen, 2011-11-01 18:40:23]

On 2011/10/27 23:31:13, darin wrote:
> As discussed in person, I think we should probably introduce a DocumentState
> object that is 1-1 with WebDataSource (i.e., only created in
> didCreateDataSource).  DocumentState can have a NavigationState that gets
> updated when we only perform in-document navigations.

Yep. Good idea. That looks much nicer. PTAL.

I still need to un-flake the test (there's a race on first paint), but otherwise
the code is ready to go.

> Long-term, we should probably store NavigationState on the
WebCore::HistoryItem.

I added a TODO.

[James Simonsen, 2011-11-08 01:08:00]

On 2011/11/01 18:40:23, James Simonsen wrote:
> I still need to un-flake the test (there's a race on first paint), but
otherwise
> the code is ready to go.

I've done this too now.

Darin, can you please take another look?

[James Simonsen, 2011-11-15 22:40:06]

On 2011/11/08 01:08:00, James Simonsen wrote:
> Darin, can you please take another look?

darin: ping

[darin (slow to review), 2011-11-16 04:56:22]

LGTM

[commit-bot: I haz the power, 2011-11-16 21:16:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8404018/19001

[commit-bot: I haz the power, 2011-11-17 00:34:08]

Change committed as 110392

[Aaron Boodman, 2012-01-09 18:53:08]

Hi there.

Dominic Battre and I were looking at some data and we noticed a large spike in
the number of samples for PLT.BeginToFinish_NormalLoad (and related).

Also, the timings got a bit faster (~10%). Not sure if this was expected. Ping
us privately if this concerns you. Otherwise we'll assume it was by design.