Add a "LoadLog*" parameter to transactions, hostresolver, clientsocketpool.

Add a "LoadLog" parameter to transactions, hostresolver, clientsocketpool and proyxservice.This dependency comes from the parent URLRequest, and is used as a container for per-request profiling data.This change is strictly a no-op refactor -- the parameter is unused, and LoadLog does nothing.BUG=http://crbug.com/14478TEST=none -- just needs to compile and pass existing tests.DESIGN=<http://docs.google.com/Doc?id=dfhcnb2v_21gbtrcpr3&hl=en>

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

[eroman, 2009-06-17 23:44:11]

Amended the description:

-This dependency comes from the parent URLRequestContext
+This dependency comes from the parent URLRequest

[eroman, 2009-06-17 23:46:23]

Also:

-the parameter is used ...
+the parameter is unused ...

(published before re-reading the description apparently).

[eroman, 2009-06-23 04:33:41]

ping.

[darin (slow to review), 2009-06-23 20:02:01]

This is a lot of plumbing...

As I was reading this over, it occurred to me that LoadMetrics might be another
good name for this class.  WDYT?

http://codereview.chromium.org/126303/diff/2363/2403
File net/base/load_information.h (right):

http://codereview.chromium.org/126303/diff/2363/2403#newcode14
Line 14: #if 0
please add a TODO(eroman) here.

http://codereview.chromium.org/126303/diff/2363/2371
File net/http/http_transaction.h (right):

http://codereview.chromium.org/126303/diff/2363/2371#newcode40
Line 40: virtual int Start(LoadInformation* load_info,
Maybe we should just have a setter on HttpTransaction so that the load info can
be set prior to calling Start?

http://codereview.chromium.org/126303/diff/2363/2379
File net/url_request/url_request.h (right):

http://codereview.chromium.org/126303/diff/2363/2379#newcode528
Line 528: net::LoadInformation load_info_;
hmm.. since the URLRequest can die before the URLRequestJob, don't we have to
worry about load_info_ being destroyed too soon?  maybe it should be reference
counted.

[eroman, 2009-06-25 17:12:05]

> As I was reading this over, it occurred to me that LoadMetrics might
> be another good name for this class.  WDYT?

Something future I was thinking, is that LoadInformation can replace our need
for the various *::GetLoadState() methods. Since we would already be tracking
the enter/leave states through calls to LoadInformation, it would be sufficient
to expose a method:

  LoadState LoadInformation::GetLoadState();

And to just call this from URLRequest for the latest load state info.

If we include this use case, then I think LoadInformation is a better name than
LoadMetrics. But I am fine changing to LoadMetrics if you think it is stronger.

http://codereview.chromium.org/126303/diff/2363/2371
File net/http/http_transaction.h (right):

http://codereview.chromium.org/126303/diff/2363/2371#newcode40
Line 40: virtual int Start(LoadInformation* load_info,
On 2009/06/23 20:02:01, darin wrote:
> Maybe we should just have a setter on HttpTransaction so that the load info
can
> be set prior to calling Start?

I would like LoadInformation to be a required parameter. Moving it to a setter
seems to imply it would be optional, and then lots and lots of callsites will
have to NULL-check (also see use-case above for moving GetLoadState() into
LoadInformation).

http://codereview.chromium.org/126303/diff/2363/2379
File net/url_request/url_request.h (right):

http://codereview.chromium.org/126303/diff/2363/2379#newcode528
Line 528: net::LoadInformation load_info_;
On 2009/06/23 20:02:01, darin wrote:
> hmm.. since the URLRequest can die before the URLRequestJob, don't we have to
> worry about load_info_ being destroyed too soon?  maybe it should be reference
> counted.

I can't think of use-cases where we would want to access the load-state after
the request is cancelled, so probably not worth augmenting its lifespan.

What If I simply comment that the validity of LoadInformation* is tied to that
of the request?

[darin (slow to review), 2009-06-25 18:47:15]

Oh yeah... LoadStates... I guess LoadInformation (or LoadInfo) is fine then.

http://codereview.chromium.org/126303/diff/2363/2379
File net/url_request/url_request.h (right):

http://codereview.chromium.org/126303/diff/2363/2379#newcode528
Line 528: net::LoadInformation load_info_;
What I'm concerned about is crashes.  Are you sure that the
URLRequestJob+HttpTransaction+HostResolver will no longer touch LoadInformation
after the URLRequest is destroyed?

[darin (slow to review), 2009-06-25 18:48:01]

Or.. maybe LoadLog, LoadJournal, LoadActivityLog.. ugh

[eroman, 2009-07-09 20:50:06]

Sorry for the delay; I got pre-empted by more important stuff.

Note that I have attached an updated design doc to the bug, which should serve
as a good overview for this change. See
<http://docs.google.com/View?id=dfhcnb2v_21gbtrcpr3>.

> Or.. maybe LoadLog, LoadJournal, LoadActivityLog.. ugh

Done. Renamed to LoadLog.

http://codereview.chromium.org/126303/diff/2363/2379
File net/url_request/url_request.h (right):

http://codereview.chromium.org/126303/diff/2363/2379#newcode528
Line 528: net::LoadInformation load_info_;
On 2009/06/25 18:47:15, darin wrote:
> What I'm concerned about is crashes.  Are you sure that the
> URLRequestJob+HttpTransaction+HostResolver will no longer touch
LoadInformation
> after the URLRequest is destroyed?

Done. Made it ref counted.

[darin (slow to review), 2009-08-11 16:41:50]

http://codereview.chromium.org/126303/diff/8064/7090
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/126303/diff/8064/7090#newcode54
Line 54: unused_log, resolve_info, &addresses_, &net_callback_);
API-wise, it sure would be nice for the LoadLog parameters to be optionally
null.  Will it make the implementation code too fugly?

http://codereview.chromium.org/126303/diff/8064/7106
File net/base/load_log.h (right):

http://codereview.chromium.org/126303/diff/8064/7106#newcode20
Line 20: };
perhaps to make the null checking of LoadLog feasible, you could switch to
static methods:

  LoadLog::TrackEnterState(load_log, load_state);

Then the implementation of TrackEnterState would perform the null checking. 
That way the callsites can remain simple.

[eroman, 2009-08-11 22:55:47]

http://codereview.chromium.org/126303/diff/8064/7090
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/126303/diff/8064/7090#newcode54
Line 54: unused_log, resolve_info, &addresses_, &net_callback_);
On 2009/08/11 16:41:50, darin wrote:
> API-wise, it sure would be nice for the LoadLog parameters to be optionally
> null.  Will it make the implementation code too fugly?

Done -- made LoadLog* an optional parameter everywhere.

<tedious_explanation>

This dramatically reduces the fallout in the unit-test files (since I am now
simply adding a bunch of NULLs). I agree this makes for a cleaner API.

Initially my concern with making it optional, was that:
 (a) increases the surface area for NULL derefs
 (b) the testing codepaths are different than production.

Your suggestion of statics nicely addresses (a).

And as for (b): I plan to gradually add in real LoadLog instances to the
unittests in the next phase of this change. (When I start teaching
implementations how to fill it, and then verify it was filled properly in the
tests).

</tedious_explanation>

[darin (slow to review), 2009-08-11 23:08:48]

LGTM

http://codereview.chromium.org/126303/diff/9028/10163
File net/base/load_log.h (right):

http://codereview.chromium.org/126303/diff/9028/10163#newcode12
Line 12: // LoadLog stores profiling information no where time was spent while
servicing
nit: "to know where" ??

[eroman, 2009-08-11 23:19:54]

http://codereview.chromium.org/126303/diff/9028/10163
File net/base/load_log.h (right):

http://codereview.chromium.org/126303/diff/9028/10163#newcode12
Line 12: // LoadLog stores profiling information no where time was spent while
servicing
On 2009/08/11 23:08:48, darin wrote:
> nit: "to know where" ??

Done.

(I meant to write "on" instead of "no")

[darin (slow to review), 2009-08-11 23:36:27]

Makes sense, LGTM

On Tue, Aug 11, 2009 at 4:19 PM, <eroman@chromium.org> wrote:

>
> http://codereview.chromium.org/126303/diff/9028/10163
> File net/base/load_log.h (right):
>
> http://codereview.chromium.org/126303/diff/9028/10163#newcode12
> Line 12: // LoadLog stores profiling information no where time was spent
> while servicing
> On 2009/08/11 23:08:48, darin wrote:
>
>> nit: "to know where" ??
>>
>
> Done.
>
> (I meant to write "on" instead of "no")
>
>
> http://codereview.chromium.org/126303
>

[willchan no longer on Chromium, 2009-08-13 20:34:37]

I'm merging this into one of my changelists right now and wanted to check
something.  LoadLog is intended to be an output parameter, right?  If so, should
it be the ordered after the input parameters in all the function prototypes
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

I'm wondering whether or not to fix up the code I'm merging now, or just let it
be.  If people agree with my assessment, then I'll go ahead and fix all the
ClientSocketPool* function parameter orderings.

On 2009/08/11 23:36:27, darin wrote:
> Makes sense, LGTM
> 
> On Tue, Aug 11, 2009 at 4:19 PM, <eroman@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/126303/diff/9028/10163
> > File net/base/load_log.h (right):
> >
> > http://codereview.chromium.org/126303/diff/9028/10163#newcode12
> > Line 12: // LoadLog stores profiling information no where time was spent
> > while servicing
> > On 2009/08/11 23:08:48, darin wrote:
> >
> >> nit: "to know where" ??
> >>
> >
> > Done.
> >
> > (I meant to write "on" instead of "no")
> >
> >
> > http://codereview.chromium.org/126303
> >
>

[eroman, 2009-08-13 20:46:28]

> I'm wondering whether or not to fix up the code I'm merging 
> now, or just let it be.  If people agree with my assessment, 
> then I'll go ahead and fix all the ClientSocketPool* 
> function parameter orderings.

Moving the parameter is fine by me.

If you do move it, can you make it be the very last parameter everywhere? (My
hope is to to have a consistent convention on where in the parameter list the
LoadLog* is given).

Fixes to my style are appreciated.

[willchan no longer on Chromium, 2009-08-13 21:03:54]

On Thu, Aug 13, 2009 at 1:46 PM, <eroman@chromium.org> wrote:
>
>> I'm wondering whether or not to fix up the code I'm merging
>> now, or just let it be. =A0If people agree with my assessment,
>> then I'll go ahead and fix all the ClientSocketPool*
>> function parameter orderings.
>
> Moving the parameter is fine by me.
>
> If you do move it, can you make it be the very last parameter
> everywhere? (My hope is to to have a consistent convention on where in
> the parameter list the LoadLog* is given).
>
> Fixes to my style are appreciated.

Sure thing.

>
> http://codereview.chromium.org/126303
>