- Added support for keeping track of load times....

- Added support for keeping track of load times.
For each document loaded we record the time the page was requested by the user
(or as close as we can get to that), the time the load process started, the time
the
document and it's dependent resources (scripts) have been loaded (before
onload())
and the time all the document's resources have been loaded.
We use this data for two things:

1) We histogram the deltas between the time marks
2) We expose the times to javascript running on the page which was loaded


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

[darin (slow to review), 2009-03-25 16:07:34]

I think a better solution is to add some attributes to WebDataSource to record
the navigation type and navigation state change times.  I would then move all of
the histogramming code into renderer/.

http://codereview.chromium.org/42527/diff/36/1011
File base/time.h (right):

http://codereview.chromium.org/42527/diff/36/1011#newcode216
Line 216: // Converts time to a double which is the number of seconds since
epoch
nit: to/from

http://codereview.chromium.org/42527/diff/36/1013
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/42527/diff/36/1013#newcode38
Line 38: #include "base/time.h"
nit: headers should be alphabetically listed

http://codereview.chromium.org/42527/diff/36/1014
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/42527/diff/36/1014#newcode85
Line 85: double transition_time;
nit: use base::Time instead of a double here.  chrome code uses base::Time to
store time values.

http://codereview.chromium.org/42527/diff/36/1002
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/42527/diff/36/1002#newcode25
Line 25: class Time;
nit: google style says not to indent the body of a namespace

http://codereview.chromium.org/42527/diff/36/1002#newcode385
Line 385: // All values in seconds
nit: this comment is wrong / misleading / probably out-of-date

http://codereview.chromium.org/42527/diff/36/1002#newcode386
Line 386: virtual base::Time GetRequestTime() = 0;
these may be better placed on WebDataSource since they seem to relate to the
navigation that generated the document held by this frame.  note: a frame can
have several documents, and there is a WebDataSource corresponding to each.

http://codereview.chromium.org/42527/diff/36/1002#newcode390
Line 390: virtual const char* GetNavigationType() = 0;
how should these strings be interpreted?

http://codereview.chromium.org/42527/diff/36/1001
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/42527/diff/36/1001#newcode1972
Line 1972: const char *WebFrameImpl::GetNavigationType() {
it seems like we should expose an enum instead.

http://codereview.chromium.org/42527/diff/36/1007
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/42527/diff/36/1007#newcode423
Line 423: UMA_HISTOGRAM_TIMES("Renderer.All.RequestToStart", requestToStart);
we need to avoid using the histogram APIs directly from glue.  soon this file
needs to be upstreamed to webkit.

[DaveMoore, 2009-03-26 00:56:02]

http://codereview.chromium.org/42527/diff/36/1011
File base/time.h (right):

http://codereview.chromium.org/42527/diff/36/1011#newcode216
Line 216: // Converts time to a double which is the number of seconds since
epoch
On 2009/03/25 16:07:34, darin wrote:
> nit: to/from

Done.

http://codereview.chromium.org/42527/diff/36/1013
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/42527/diff/36/1013#newcode38
Line 38: #include "base/time.h"
On 2009/03/25 16:07:34, darin wrote:
> nit: headers should be alphabetically listed

Done.

http://codereview.chromium.org/42527/diff/36/1014
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/42527/diff/36/1014#newcode85
Line 85: double transition_time;
On 2009/03/25 16:07:34, darin wrote:
> nit: use base::Time instead of a double here.  chrome code uses base::Time to
> store time values.

Done.

http://codereview.chromium.org/42527/diff/36/1002
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/42527/diff/36/1002#newcode25
Line 25: class Time;
On 2009/03/25 16:07:34, darin wrote:
> nit: google style says not to indent the body of a namespace

Done.

http://codereview.chromium.org/42527/diff/36/1002#newcode385
Line 385: // All values in seconds
On 2009/03/25 16:07:34, darin wrote:
> nit: this comment is wrong / misleading / probably out-of-date

Done.

http://codereview.chromium.org/42527/diff/36/1002#newcode386
Line 386: virtual base::Time GetRequestTime() = 0;
On 2009/03/25 16:07:34, darin wrote:
> these may be better placed on WebDataSource since they seem to relate to the
> navigation that generated the document held by this frame.  note: a frame can
> have several documents, and there is a WebDataSource corresponding to each.
I left the methods here, but they use the GetDataSourceImpl() method to find the
data source, and the values. They are called from the V8 extension which needs
the ability to get the right data for the current context. Is there a better way
to do this?

http://codereview.chromium.org/42527/diff/36/1002#newcode390
Line 390: virtual const char* GetNavigationType() = 0;
On 2009/03/25 16:07:34, darin wrote:
> how should these strings be interpreted?
These strings are meant for the v8 extension and will only matter to the
javascript code.

http://codereview.chromium.org/42527/diff/36/1001
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/42527/diff/36/1001#newcode1972
Line 1972: const char *WebFrameImpl::GetNavigationType() {
On 2009/03/25 16:07:34, darin wrote:
> it seems like we should expose an enum instead.

This is called from the JS extension...an enum would just push the conversion to
strings into that implementation.

http://codereview.chromium.org/42527/diff/36/1007
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/42527/diff/36/1007#newcode423
Line 423: UMA_HISTOGRAM_TIMES("Renderer.All.RequestToStart", requestToStart);
On 2009/03/25 16:07:34, darin wrote:
> we need to avoid using the histogram APIs directly from glue.  soon this file
> needs to be upstreamed to webkit.

I moved it to webdatasource_impl, but that's glue too. Where would you suggest?

[DaveMoore, 2009-03-27 01:28:27]

I moved the UMA stuff into render_view.cc and exposed the WebNavigationType the
way we discussed

[darin (slow to review), 2009-03-27 16:43:29]

http://codereview.chromium.org/42527/diff/1054/108
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/42527/diff/1054/108#newcode618
Line 618: params->transition_time = base::Time::Now();
i see... this is the time the request was initiated.  what about frame load
requests that do not originate in the browser (e.g., link clicks)?

http://codereview.chromium.org/42527/diff/1054/106
File webkit/glue/webdatasource.h (right):

http://codereview.chromium.org/42527/diff/1054/106#newcode99
Line 99: // LoadTimes
please document what these times mean.

http://codereview.chromium.org/42527/diff/1054/99
File webkit/glue/weburlrequest.h (right):

http://codereview.chromium.org/42527/diff/1054/99#newcode107
Line 107: virtual void SetTransitionTime(double transitionTime) = 0;
why is this a property of a request?  in general, we should not be adding
properties to WebRequest that are not also reflected on ResourceRequest.  (the
history state attribute should not be part of this interface, for example.)

http://codereview.chromium.org/42527/diff/1054/107
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/42527/diff/1054/107#newcode46
Line 46: enum WebNavigationType;
you cannot forward declare an enum like this.  it will fail to compile under
gcc.

[DaveMoore, 2009-03-27 23:03:27]

Moved transition time into extra data in RenderView. Added setter for request
time to WebDataSource

http://codereview.chromium.org/42527/diff/1054/106
File webkit/glue/webdatasource.h (right):

http://codereview.chromium.org/42527/diff/1054/106#newcode99
Line 99: // LoadTimes
On 2009/03/27 16:43:29, darin wrote:
> please document what these times mean.

Done.

http://codereview.chromium.org/42527/diff/1054/99
File webkit/glue/weburlrequest.h (right):

http://codereview.chromium.org/42527/diff/1054/99#newcode107
Line 107: virtual void SetTransitionTime(double transitionTime) = 0;
On 2009/03/27 16:43:29, darin wrote:
> why is this a property of a request?  in general, we should not be adding
> properties to WebRequest that are not also reflected on ResourceRequest.  (the
> history state attribute should not be part of this interface, for example.)

Done.

http://codereview.chromium.org/42527/diff/1054/107
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/42527/diff/1054/107#newcode46
Line 46: enum WebNavigationType;
On 2009/03/27 16:43:29, darin wrote:
> you cannot forward declare an enum like this.  it will fail to compile under
> gcc.

Done.

[darin (slow to review), 2009-03-30 21:27:15]

http://codereview.chromium.org/42527/diff/5003/4012
File webkit/glue/webdatasource.h (right):

http://codereview.chromium.org/42527/diff/5003/4012#newcode99
Line 99: // LoadTimes
nit: please separate method declarations with new lines for consistency with
existing code.

http://codereview.chromium.org/42527/diff/5003/4012#newcode103
Line 103: // the ipc parameters
nit: please don't mention things like "ipc" at this layer.  webkit/glue doesn't
know about multiprocess stuff.

http://codereview.chromium.org/42527/diff/5003/4012#newcode105
Line 105: // The time we started loading the page
nit: please write proper sentences that end with a period.  see above.

http://codereview.chromium.org/42527/diff/5003/4011
File webkit/glue/webdatasource_impl.cc (right):

http://codereview.chromium.org/42527/diff/5003/4011#newcode11
Line 11: #include "FrameLoaderTypes.h"
nit: please alphabetize headers

http://codereview.chromium.org/42527/diff/5003/4011#newcode21
Line 21: #include "webkit/glue/webview_delegate.h"
nit: please alphabetize headers

http://codereview.chromium.org/42527/diff/5003/4011#newcode38
Line 38: request_time_(Time()),
nit: no need to list these here when you are just calling the default
constructors.

http://codereview.chromium.org/42527/diff/5003/4011#newcode134
Line 134: WebNavigationType
WebDataSourceImpl::NavigationTypeToWebNavigationType(
nit: please list the functions in the .cc file in the same order they were
declared in the header.

http://codereview.chromium.org/42527/diff/5003/4010
File webkit/glue/webdatasource_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4010#newcode41
Line 41: // LoadTimes
nit: no need for this comment.  (GetNavigationType is not a load time, so this
divider comment is a bit misleading)

http://codereview.chromium.org/42527/diff/5003/4010#newcode50
Line 50: WebCore::NavigationType t);
nit: when wrapping arguments, please use 4 spaces instead of 2.

http://codereview.chromium.org/42527/diff/5003/4010#newcode130
Line 130: // The time the request to load the page was received
nit: same nit about proper sentences ending with periods.

http://codereview.chromium.org/42527/diff/5003/4010#newcode134
Line 134: // The time the document itself was loaded, before the onload() method
it may be more helpful to talk about this load time in terms of the
DOMContentLoaded event.  "load" doesn't fire until a document and all of its
subresources have loaded, but DOMContentLoaded fires when the DOM is fully
formed.  i like how you mention before/after onload.  that is very helpful to
know what has or hasn't happened when these timestamps were collected.

http://codereview.chromium.org/42527/diff/5003/4007
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/42527/diff/5003/4007#newcode64
Line 64: #include "StringExtras.h"
#include <wtf/StringExtras.h>

please move this into the section above where we list other WebKit headers...
that is, just above the MSVC_POP_WARNING.

http://codereview.chromium.org/42527/diff/5003/4007#newcode67
Line 67: using base::TimeDelta;
nit: please alphabetize

http://codereview.chromium.org/42527/diff/5003/4007#newcode391
Line 391: webframe_->frame()->loader()->activeDocumentLoader();
nit: when wrapping a line, we usually indent by 4 spaces

http://codereview.chromium.org/42527/diff/5003/4007#newcode392
Line 392: WebDataSourceImpl* dataSource =
nit: please use google style to name these variables

http://codereview.chromium.org/42527/diff/5003/4009
File webkit/glue/webframeloaderclient_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4009#newcode21
Line 21: #include "base/time.h"
nit: alphabetize headers

http://codereview.chromium.org/42527/diff/5003/4008
File webkit/glue/weburlrequest_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4008#newcode66
Line 66: double transitionTime_;
transition_time_

... actually, i thought you were going to delete this member.

http://codereview.chromium.org/42527/diff/5003/4013
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/42527/diff/5003/4013#newcode33
Line 33: #include "webkit/glue/webdatasource.h"
nit: alphabetize

[darin (slow to review), 2009-03-30 21:40:26]

sorry, my last batch of comments only applied to webdatasource.h and later. 
here's comments for the former set of files.

http://codereview.chromium.org/42527/diff/5003/4014
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/42527/diff/5003/4014#newcode618
Line 618: params->transition_time = base::Time::Now();
nit: can you call this request_time to be consistent?

http://codereview.chromium.org/42527/diff/5003/4024
File chrome/chrome.gyp (right):

http://codereview.chromium.org/42527/diff/5003/4024#newcode1370
Line 1370: 'renderer/extensions/loadtimes_extension_bindings.h',
nit: please alphabetize

http://codereview.chromium.org/42527/diff/5003/4015
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/42527/diff/5003/4015#newcode48
Line 48: class Time;
nit: do not indent the body of a namespace

http://codereview.chromium.org/42527/diff/5003/4022
File chrome/renderer/extensions/loadtimes_extension_bindings.cc (right):

http://codereview.chromium.org/42527/diff/5003/4022#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/42527/diff/5003/4022#newcode5
Line 5: //#include "config.h"
please remove commented out code

http://codereview.chromium.org/42527/diff/5003/4022#newcode9
Line 9: #include "base/time.h"
nit: alphabetize

http://codereview.chromium.org/42527/diff/5003/4022#newcode11
Line 11: const char* kLoadTimesExtensionName = "v8/LoadTimes";
this should be static or in an anonymous namespace

http://codereview.chromium.org/42527/diff/5003/4022#newcode14
Line 14: public:
nit: please indent by one space

http://codereview.chromium.org/42527/diff/5003/4022#newcode24
Line 24: LoadTimesExtensionWrapper() :
nit please indent by 2 spaces

http://codereview.chromium.org/42527/diff/5003/4023
File chrome/renderer/extensions/loadtimes_extension_bindings.h (right):

http://codereview.chromium.org/42527/diff/5003/4023#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009, not 2006-2008

http://codereview.chromium.org/42527/diff/5003/4023#newcode8
Line 8: #ifndef LOADTIMES_EXTENSION_H__
please use a correct include guard

http://codereview.chromium.org/42527/diff/5003/4023#newcode11
Line 11: #include "v8/include/v8.h"
normally, we avoid headers if we don't need them.  this helps with compile time.
 here, you can just forward declare v8::Extension like so:

namespace v8 {
class Extension;
}

http://codereview.chromium.org/42527/diff/5003/4017
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/42527/diff/5003/4017#newcode34
Line 34: #include "chrome/renderer/extensions/loadtimes_extension_bindings.h"
nit: alphabetize

http://codereview.chromium.org/42527/diff/5003/4017#newcode255
Line 255:
WebKit::registerExtension(extensions_v8::ExtensionProcessBindings::Get());
did you mean to revert the scheme restriction for the ExtenionProcessBindings
extension?

http://codereview.chromium.org/42527/diff/5003/4019
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/42527/diff/5003/4019#newcode1212
Line 1212: RenderViewExtraRequestData* extraData =
extra_data

http://codereview.chromium.org/42527/diff/5003/4019#newcode1423
Line 1423: GURL url = frame->GetURL();
nit: i recommend making this be "const GURL& url = " to avoid the need to copy
the GURL.

http://codereview.chromium.org/42527/diff/5003/4019#newcode1424
Line 1424: if (url.SchemeIs("http") || (url.SchemeIs("https")))
nit: in one case you put parens around the SchemeIs call and in the other you do
not.  please be consistent

http://codereview.chromium.org/42527/diff/5003/4019#newcode3047
Line 3047: TimeDelta requestToStart = startLoadTime - requestTime;
please name these variables properly

http://codereview.chromium.org/42527/diff/5003/4021
File chrome/renderer/renderer.scons (right):

http://codereview.chromium.org/42527/diff/5003/4021#newcode65
Line 65: 'extensions/loadtimes_extension_bindings.h',
nit: please alphabetize

http://codereview.chromium.org/42527/diff/5003/4016
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/42527/diff/5003/4016#newcode13
Line 13: #include "base/process_util.h"
nit: alphabetize

http://codereview.chromium.org/42527/diff/5003/4016#newcode43
Line 43: std::wstring message = L"renderer starting with pid: ";
StringPrintf is another option here.

[DaveMoore, 2009-04-01 00:11:26]

http://codereview.chromium.org/42527/diff/5003/4014
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/42527/diff/5003/4014#newcode618
Line 618: params->transition_time = base::Time::Now();
On 2009/03/30 21:40:26, darin wrote:
> nit: can you call this request_time to be consistent?

Done.

http://codereview.chromium.org/42527/diff/5003/4024
File chrome/chrome.gyp (right):

http://codereview.chromium.org/42527/diff/5003/4024#newcode1370
Line 1370: 'renderer/extensions/loadtimes_extension_bindings.h',
On 2009/03/30 21:40:26, darin wrote:
> nit: please alphabetize

Done.

http://codereview.chromium.org/42527/diff/5003/4015
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/42527/diff/5003/4015#newcode48
Line 48: class Time;
On 2009/03/30 21:40:26, darin wrote:
> nit: do not indent the body of a namespace

Done.

http://codereview.chromium.org/42527/diff/5003/4022
File chrome/renderer/extensions/loadtimes_extension_bindings.cc (right):

http://codereview.chromium.org/42527/diff/5003/4022#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/03/30 21:40:26, darin wrote:
> 2009

Done.

http://codereview.chromium.org/42527/diff/5003/4022#newcode5
Line 5: //#include "config.h"
On 2009/03/30 21:40:26, darin wrote:
> please remove commented out code

Done.

http://codereview.chromium.org/42527/diff/5003/4022#newcode9
Line 9: #include "base/time.h"
On 2009/03/30 21:40:26, darin wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/42527/diff/5003/4022#newcode11
Line 11: const char* kLoadTimesExtensionName = "v8/LoadTimes";
Done. And I changed the other two extensions in the same way.
On 2009/03/30 21:40:26, darin wrote:
> this should be static or in an anonymous namespace

http://codereview.chromium.org/42527/diff/5003/4022#newcode14
Line 14: public:
On 2009/03/30 21:40:26, darin wrote:
> nit: please indent by one space

Done.

http://codereview.chromium.org/42527/diff/5003/4022#newcode24
Line 24: LoadTimesExtensionWrapper() :
On 2009/03/30 21:40:26, darin wrote:
> nit please indent by 2 spaces

Done.

http://codereview.chromium.org/42527/diff/5003/4023
File chrome/renderer/extensions/loadtimes_extension_bindings.h (right):

http://codereview.chromium.org/42527/diff/5003/4023#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/03/30 21:40:26, darin wrote:
> 2009, not 2006-2008

Done.

http://codereview.chromium.org/42527/diff/5003/4023#newcode8
Line 8: #ifndef LOADTIMES_EXTENSION_H__
On 2009/03/30 21:40:26, darin wrote:
> please use a correct include guard

Done.

http://codereview.chromium.org/42527/diff/5003/4023#newcode11
Line 11: #include "v8/include/v8.h"
On 2009/03/30 21:40:26, darin wrote:
> normally, we avoid headers if we don't need them.  this helps with compile
time.
>  here, you can just forward declare v8::Extension like so:
> 
> namespace v8 {
> class Extension;
> }

Done.

http://codereview.chromium.org/42527/diff/5003/4023#newcode11
Line 11: #include "v8/include/v8.h"
Done. I also changed the other two v8 renderer extensions to match.
On 2009/03/30 21:40:26, darin wrote:
> normally, we avoid headers if we don't need them.  this helps with compile
time.
>  here, you can just forward declare v8::Extension like so:
> 
> namespace v8 {
> class Extension;
> }

http://codereview.chromium.org/42527/diff/5003/4019
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/42527/diff/5003/4019#newcode1212
Line 1212: RenderViewExtraRequestData* extraData =
On 2009/03/30 21:40:26, darin wrote:
> extra_data

Done.

http://codereview.chromium.org/42527/diff/5003/4019#newcode1423
Line 1423: GURL url = frame->GetURL();
On 2009/03/30 21:40:26, darin wrote:
> nit: i recommend making this be "const GURL& url = " to avoid the need to copy
> the GURL.

Done.

http://codereview.chromium.org/42527/diff/5003/4019#newcode1424
Line 1424: if (url.SchemeIs("http") || (url.SchemeIs("https")))
On 2009/03/30 21:40:26, darin wrote:
> nit: in one case you put parens around the SchemeIs call and in the other you
do
> not.  please be consistent

Done.

http://codereview.chromium.org/42527/diff/5003/4019#newcode3047
Line 3047: TimeDelta requestToStart = startLoadTime - requestTime;
On 2009/03/30 21:40:26, darin wrote:
> please name these variables properly

Done.

http://codereview.chromium.org/42527/diff/5003/4021
File chrome/renderer/renderer.scons (right):

http://codereview.chromium.org/42527/diff/5003/4021#newcode65
Line 65: 'extensions/loadtimes_extension_bindings.h',
On 2009/03/30 21:40:26, darin wrote:
> nit: please alphabetize

Done.

http://codereview.chromium.org/42527/diff/5003/4012
File webkit/glue/webdatasource.h (right):

http://codereview.chromium.org/42527/diff/5003/4012#newcode99
Line 99: // LoadTimes
On 2009/03/30 21:27:15, darin wrote:
> nit: please separate method declarations with new lines for consistency with
> existing code.

Done.

http://codereview.chromium.org/42527/diff/5003/4012#newcode103
Line 103: // the ipc parameters
On 2009/03/30 21:27:15, darin wrote:
> nit: please don't mention things like "ipc" at this layer.  webkit/glue
doesn't
> know about multiprocess stuff.

Done.

http://codereview.chromium.org/42527/diff/5003/4012#newcode105
Line 105: // The time we started loading the page
On 2009/03/30 21:27:15, darin wrote:
> nit: please write proper sentences that end with a period.  see above.

Done.

http://codereview.chromium.org/42527/diff/5003/4011
File webkit/glue/webdatasource_impl.cc (right):

http://codereview.chromium.org/42527/diff/5003/4011#newcode11
Line 11: #include "FrameLoaderTypes.h"
On 2009/03/30 21:27:15, darin wrote:
> nit: please alphabetize headers

Done.

http://codereview.chromium.org/42527/diff/5003/4011#newcode21
Line 21: #include "webkit/glue/webview_delegate.h"
On 2009/03/30 21:27:15, darin wrote:
> nit: please alphabetize headers

Done.

http://codereview.chromium.org/42527/diff/5003/4011#newcode38
Line 38: request_time_(Time()),
On 2009/03/30 21:27:15, darin wrote:
> nit: no need to list these here when you are just calling the default
> constructors.

Done.

http://codereview.chromium.org/42527/diff/5003/4011#newcode134
Line 134: WebNavigationType
WebDataSourceImpl::NavigationTypeToWebNavigationType(
On 2009/03/30 21:27:15, darin wrote:
> nit: please list the functions in the .cc file in the same order they were
> declared in the header.

Done.

http://codereview.chromium.org/42527/diff/5003/4010
File webkit/glue/webdatasource_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4010#newcode41
Line 41: // LoadTimes
On 2009/03/30 21:27:15, darin wrote:
> nit: no need for this comment.  (GetNavigationType is not a load time, so this
> divider comment is a bit misleading)

Done.

http://codereview.chromium.org/42527/diff/5003/4010#newcode50
Line 50: WebCore::NavigationType t);
On 2009/03/30 21:27:15, darin wrote:
> nit: when wrapping arguments, please use 4 spaces instead of 2.

Done.

http://codereview.chromium.org/42527/diff/5003/4010#newcode130
Line 130: // The time the request to load the page was received
On 2009/03/30 21:27:15, darin wrote:
> nit: same nit about proper sentences ending with periods.

Done.

http://codereview.chromium.org/42527/diff/5003/4010#newcode134
Line 134: // The time the document itself was loaded, before the onload() method
On 2009/03/30 21:27:15, darin wrote:
> it may be more helpful to talk about this load time in terms of the
> DOMContentLoaded event.  "load" doesn't fire until a document and all of its
> subresources have loaded, but DOMContentLoaded fires when the DOM is fully
> formed.  i like how you mention before/after onload.  that is very helpful to
> know what has or hasn't happened when these timestamps were collected.

Done.

http://codereview.chromium.org/42527/diff/5003/4007
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/42527/diff/5003/4007#newcode67
Line 67: using base::TimeDelta;
On 2009/03/30 21:27:15, darin wrote:
> nit: please alphabetize

Done.

http://codereview.chromium.org/42527/diff/5003/4007#newcode391
Line 391: webframe_->frame()->loader()->activeDocumentLoader();
On 2009/03/30 21:27:15, darin wrote:
> nit: when wrapping a line, we usually indent by 4 spaces

Done.

http://codereview.chromium.org/42527/diff/5003/4007#newcode392
Line 392: WebDataSourceImpl* dataSource =
On 2009/03/30 21:27:15, darin wrote:
> nit: please use google style to name these variables

Done.

http://codereview.chromium.org/42527/diff/5003/4009
File webkit/glue/webframeloaderclient_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4009#newcode21
Line 21: #include "base/time.h"
On 2009/03/30 21:27:15, darin wrote:
> nit: alphabetize headers

Done.

http://codereview.chromium.org/42527/diff/5003/4008
File webkit/glue/weburlrequest_impl.h (right):

http://codereview.chromium.org/42527/diff/5003/4008#newcode66
Line 66: double transitionTime_;
On 2009/03/30 21:27:15, darin wrote:
> transition_time_
> 
> ... actually, i thought you were going to delete this member.

Done.

http://codereview.chromium.org/42527/diff/5003/4013
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/42527/diff/5003/4013#newcode33
Line 33: #include "webkit/glue/webdatasource.h"
On 2009/03/30 21:27:15, darin wrote:
> nit: alphabetize

Done.

[darin (slow to review), 2009-04-02 17:59:18]

more lint errors.. not a complete list. please click the lint links.

http://codereview.chromium.org/42527/diff/6006/6025
File chrome/renderer/extensions/loadtimes_extension_bindings.cc (right):

http://codereview.chromium.org/42527/diff/6006/6025#newcode6
Line 6: #include "loadtimes_extension_bindings.h"
nit: style guide says to use a full include path for this, and the order of the
includes is wrong.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

http://codereview.chromium.org/42527/diff/6006/6025#newcode34
Line 34: "}") {};
nit: semi-colon after "}" is extraneous and may cause gcc to complain??

http://codereview.chromium.org/42527/diff/6006/6025#newcode58
Line 58: if (winFrame) {
nit: please use google style for your variable names:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_L...

winFrame, dataSource, loadTimes are all wrong.

http://codereview.chromium.org/42527/diff/6006/6025#newcode70
Line 70: v8::Number::New(dataSource->GetFinishDocumentLoadTime().ToDoubleT()));
nit: line lengths should not exceed 80 chars:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_L...

http://codereview.chromium.org/42527/diff/6006/6026
File chrome/renderer/extensions/loadtimes_extension_bindings.h (right):

http://codereview.chromium.org/42527/diff/6006/6026#newcode8
Line 8: #ifndef LOADTIMES_EXTENSION_BINDINGS_H__
nit: style guide says that include guards should end with "_" instead of "__"

the style guide changed about a year ago and it previously said to use "__". 
the guidance from c-style was to make the conversion to "_" as files are
modified or added.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=The__d...

[DaveMoore, 2009-04-02 23:36:59]

I think I got them all. Lint complains about some files not ending with
newlines, but as best I can tell they all do.

[darin (slow to review), 2009-04-03 04:51:09]

LGTM for submit w/ these nits picked.

http://codereview.chromium.org/42527/diff/9022/9031
File webkit/glue/webdatasource.h (right):

http://codereview.chromium.org/42527/diff/9022/9031#newcode99
Line 99: // LoadTimes
nit: i would just delete this LoadTimes comment.  as is, it seems like it is
part of the comment for GetRequestTime, which is not what you intended.

http://codereview.chromium.org/42527/diff/9022/9031#newcode100
Line 100: // Returns the time the document was request by the user.
nit: "was request[ed] by..."

nit: don't you want to say something about what value this has if SetRequestTime
was not called?  it might help to explain that SetRequestTime is an optional
means to override the request time...

http://codereview.chromium.org/42527/diff/9022/9031#newcode109
Line 109: // This is before onload() is called.
nit: mentioning that this happens before onload() is a bit confusing since it
seems to suggest proximity to onload, which isn't accurate, right?  maybe you
mean after DOMContentLoaded?  (there could be minutes between the time the
document finished loading an onload fires since there could be many sub
resources loading.)

http://codereview.chromium.org/42527/diff/9022/9029
File webkit/glue/webdatasource_impl.h (right):

http://codereview.chromium.org/42527/diff/9022/9029#newcode129
Line 129: // The time the request to load the page was received.
nit: it is unfortunate to have to repeat the description of these time stamps. 
it might be easy for them to get out-of-sync with the descriptions in
webdatasource.h.  maybe they are not necessary to have here?  you could say "see
webdatasource.h for a description of these time stamps."

http://codereview.chromium.org/42527/diff/9022/9026
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/42527/diff/9022/9026#newcode391
Line 391: webframe_->frame()->loader()->activeDocumentLoader();
nit: 4 space indent for line continuations... or at least be consistent :)

http://codereview.chromium.org/42527/diff/9022/9028
File webkit/glue/webframeloaderclient_impl.h (right):

http://codereview.chromium.org/42527/diff/9022/9028#newcode208
Line 208: void dumpLoadHistograms();
i think this method is no longer defined

http://codereview.chromium.org/42527/diff/9022/9027
File webkit/glue/weburlrequest_impl.h (right):

http://codereview.chromium.org/42527/diff/9022/9027#newcode66
Line 66: 
nit: looks like this is a gratuitous whitespace change that should not be a part
of this CL.

[Matt Perry, 2009-04-06 22:09:15]

We have an unfortunate naming collision here.  chrome/renderer/extensions was
intended to house chrome-extension-related code - not misc v8 extensions.  My
vote would be to move load_extension.* out to chrome/renderer/.