Generalize the net module's LoadLog facility from a passive container, to an event stream (NetLog).

Generalize the net module's LoadLog facility from a passive container, to an event stream (NetLog).

This makes it possible to associate a single NetLog with a URLRequestContext, and then attach observers to that log to watch the stream of events.

This changelist attempts to do the most direct translation, so there will be subsequent iterations to clean up.

The user-visible behavior should remain unchanged.

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

[willchan no longer on Chromium, 2010-03-12 23:52:34]

General comments.  Some of them are half-baked.  It was a lot of stuff to fit
into my head.  Figured I'd get some of the comments out of the way first.

http://codereview.chromium.org/848006/diff/58002/59116
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/58002/59116#newcode7
chrome/browser/net/chrome_net_log.cc:7: #include
"chrome/browser/net/chrome_net_log.h"
This should be first

http://codereview.chromium.org/848006/diff/58002/59116#newcode62
chrome/browser/net/chrome_net_log.cc:62:
ChromeNetLog::RequestTracker::RequestTracker(net::NetLog::SourceType
source_type)
80 cols

http://codereview.chromium.org/848006/diff/58002/59113
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59113#newcode11
chrome/browser/net/chrome_net_log.h:11: #include "base/hash_tables.h"
alphabetical

http://codereview.chromium.org/848006/diff/58002/59113#newcode46
chrome/browser/net/chrome_net_log.h:46: RequestTracker(net::NetLog::SourceType
source_type);
explicit

http://codereview.chromium.org/848006/diff/58002/59113#newcode51
chrome/browser/net/chrome_net_log.h:51: RequestInfoList GetLiveRequests();
This sounds const

http://codereview.chromium.org/848006/diff/58002/59113#newcode53
chrome/browser/net/chrome_net_log.h:53: RequestInfoList GetRecentlyDeceased();
This sounds const

http://codereview.chromium.org/848006/diff/58002/59113#newcode83
chrome/browser/net/chrome_net_log.h:83: RequestTracker* url_request_tracker() {
can we just return const references to these guys?  or do they need to be
mutated?

http://codereview.chromium.org/848006/diff/58002/59103
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/848006/diff/58002/59103#newcode15
net/base/host_resolver_impl_unittest.cc:15: #include
"net/base/net_log_unittest.h"
out of order

http://codereview.chromium.org/848006/diff/58002/59100
File net/base/net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59100#newcode28
net/base/net_log.h:28: class NetLog : public base::RefCountedThreadSafe<NetLog>
{
Does this need to be ref counted at all?  What if we instantiated a ChromeNetLog
as an IOThread member?  It seems to be that IOThread should own it.  I haven't
thought through the lifetime issues with the visualization frontend, but I
figured I'd ask the question and let you think it through and explain it to me.

http://codereview.chromium.org/848006/diff/58002/59100#newcode63
net/base/net_log.h:63: // The |id| field must be unique accross all source IDs.
The |type| field is
I'm still reading through, but it seems strange that the |type| field is in the
SourceID struct.  I don't conceptually think of the |type| as part of an "id". 
How about naming this struct Source instead of SourceID?

http://codereview.chromium.org/848006/diff/58002/59100#newcode145
net/base/net_log.h:145: BoundNetLog() {}
I'm trying to figure out why this is provided.  Shouldn't we always be using the
other constructor?

http://codereview.chromium.org/848006/diff/58002/59100#newcode160
net/base/net_log.h:160: void AddEvent(NetLog::EventType event_type) const;
In situations like this, I usually think that you should inline the definitions
to make it obvious that they're just simple wrappers.  Also, I don't think they
should be const.  BoundNetLog is conceptually basically the same thing as
NetLog, it just binds the SourceID.  If that's the case, then why is the
AddEvent() member function (conceptually a mutating function) non-const?  I know
C++ lets you get away with this, but I think we should keep these as non-const
member functions.

http://codereview.chromium.org/848006/diff/58002/59100#newcode174
net/base/net_log.h:174: NetLog* net_log() const { return net_log_.get(); }
Rather than return the raw pointer, how about just returning the const
scoped_refptr<NetLog>&?

In general, I'm starting to lean towards not using raw pointers.  I think using
const scoped_refptr<Foo>& instead of Foo* helps document that it is ref counted
and makes people think about it.  Also, I think jamesr has a bug to prevent
implicit conversions from scoped_refptr to pointers and vice versa, so it's
better to pass around the scoped_refptr reference rather than raw pointers. 
Just my personal preference.  Up to you.

http://codereview.chromium.org/848006/diff/58002/59100#newcode178
net/base/net_log.h:178: scoped_refptr<NetLog> net_log_;
Can both these members be const?

http://codereview.chromium.org/848006/diff/58002/59100#newcode182
net/base/net_log.h:182: // bounded buffer.
Is this only for tests?  I wonder if it belongs in the net_log_unittest.h.

http://codereview.chromium.org/848006/diff/58002/59100#newcode189
net/base/net_log.h:189: CapturingNetLog() : next_id_(0), max_num_entries_(-1) {}
hrm, this seems equivalent to a default parameter value, which is disallowed. 
can we just use the other constructor?

http://codereview.chromium.org/848006/diff/58002/59100#newcode193
net/base/net_log.h:193: CapturingNetLog(size_t max_num_entries)
explicit

http://codereview.chromium.org/848006/diff/58002/59100#newcode204
net/base/net_log.h:204: 
extra newline

http://codereview.chromium.org/848006/diff/58002/59100#newcode219
net/base/net_log.h:219: CapturingBoundNetLog() : capturing_net_log_(new
CapturingNetLog) {}
same comment about default parameter value

http://codereview.chromium.org/848006/diff/58002/59100#newcode221
net/base/net_log.h:221: CapturingBoundNetLog(const NetLog::SourceID&  source_id,
extra whitespace

http://codereview.chromium.org/848006/diff/58002/59100#newcode226
net/base/net_log.h:226: CapturingBoundNetLog(size_t max_num_entries)
explicit

http://codereview.chromium.org/848006/diff/58002/59100#newcode242
net/base/net_log.h:242: void AppendTo(const BoundNetLog& net_log) const;
These shouldn't be const.

http://codereview.chromium.org/848006/diff/58002/59106
File net/base/net_log_unittest.h (right):

http://codereview.chromium.org/848006/diff/58002/59106#newcode10
net/base/net_log_unittest.h:10: #include "net/base/net_log.h"
Should go first

[eroman, 2010-03-13 00:38:05]

Thanks for getting started on this!

Some responses to the big questions.

http://codereview.chromium.org/848006/diff/58002/59113
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59113#newcode83
chrome/browser/net/chrome_net_log.h:83: RequestTracker* url_request_tracker() {
On 2010/03/12 23:52:35, willchan wrote:
> can we just return const references to these guys?  or do they need to be
> mutated?

This gets mutated by view_net_internals_job_factory.cc.

I could rename it mutable_url_request_tracker() ?

http://codereview.chromium.org/848006/diff/58002/59100
File net/base/net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59100#newcode28
net/base/net_log.h:28: class NetLog : public base::RefCountedThreadSafe<NetLog>
{
On 2010/03/12 23:52:35, willchan wrote:
> Does this need to be ref counted at all?

Currently this needs to be refcounted threadsafe because of how it is used by
SingledThreadedProxyResolver. I would like to simplify how these cross-thread
users work (perhaps by making NetLog threadsafe, or adding a threadsafe wrapper
around it), but I think that is better left to a follow-up CLs.

> What if we instantiated a ChromeNetLog
> as an IOThread member?  It seems to be that IOThread should own it.  I haven't
> thought through the lifetime issues with the visualization frontend, but I
> figured I'd ask the question and let you think it through and explain it to
me.

In the current patch, there is one NetLog per URLRequestContext. This matches
how RequestTracker (the passive LoadLog collector) used to work, by collecting
passive data per context, and only displaying the information related to the
context you open about:net-internals from (so if you opened it from an incognito
window you would see different stuff). By tying it to the URLRequestContext, it
is easier to drop the passive data pertaining to incognito windows once the last
incognito window goes away.

I am not opposed to having a single NetLog that is used everywhere, but it will
mean augmenting the API to identify which URLRequestContext messages relate to.
I would rather tackle this as a separate change if we do it.

http://codereview.chromium.org/848006/diff/58002/59100#newcode63
net/base/net_log.h:63: // The |id| field must be unique accross all source IDs.
The |type| field is
On 2010/03/12 23:52:35, willchan wrote:
> I'm still reading through, but it seems strange that the |type| field is in
the
> SourceID struct.  I don't conceptually think of the |type| as part of an "id".

> How about naming this struct Source instead of SourceID?

You are right, Source is better. I'll do a rename.

http://codereview.chromium.org/848006/diff/58002/59100#newcode145
net/base/net_log.h:145: BoundNetLog() {}
On 2010/03/12 23:52:35, willchan wrote:
> I'm trying to figure out why this is provided.  Shouldn't we always be using
the
> other constructor?

This is to help with the refactor, since it matches the previous behavior of
scoped_refptr<LoadLog>.

Basically what happens is some classes have a BoundNetLog as a member variable,
but that member isn't initialized until some method like ::Start(xxx, net_log)
gets called.

Having the default avoids spraying null initializers all over the place.

http://codereview.chromium.org/848006/diff/58002/59100#newcode160
net/base/net_log.h:160: void AddEvent(NetLog::EventType event_type) const;
On 2010/03/12 23:52:35, willchan wrote:
> In situations like this, I usually think that you should inline the
definitions
> to make it obvious that they're just simple wrappers. 

Sure I can do that.
Actually, I originally had it that way but found it made the header less
readable because of many lines of pollution.

> Also, I don't think they should be const.
> BoundNetLog is conceptually basically the same thing as
> NetLog, it just binds the SourceID.  If that's the case, then why is the
> AddEvent() member function (conceptually a mutating function) non-const?  I
know
> C++ lets you get away with this, but I think we should keep these as non-const
> member functions.

Heh, I knew this would come up :)

Basically the issue is how to pass around BoundNetLog into all of the consumers.
The cleanest way IMO was to have callers take a |const BoundNetLog& log|
parameter, which they could then use to initialize their own BoundNetLog member
variable. However, there were also a lot of callsites which wanted to log
directly into their function argument... having the log methods be non-const
meant needing to make a temporary copy of the parameter, give it a new name, and
then call the method that "writes" into it. This quickly lead to widespread
uglification, and seemed very artificial since effectively we are still logging
into that destination. So I decided to avoid the problem by making all the
methods const as well so they could be called directly. In this sense I think of
BoundNetLog like a handle to a log; the handle always points to the same thing
(immutable), but you can still do actions on its destination.

I considered some other alternatives:

(1) Make the function arguments |BoundNetLog& log|. This goes against google
style, and is wonky.

(2) Make the function arguments |BoundNetLog log|. This means each caller gets a
copy. This is workable, but not desirable from a performance standpoint since we
are doing extra AddRef/Release (on threadsaferefcounted) for no good reason.

(3) Make the function arguments |BoundNetLog* log|. I don't like this approach
because it means callers can pass in NULL, and I don't want to handle NULLs (in
fact I am trying to get rid of all the NULLs). It also means callers can mutate
the log itself (replace it with something else) which really isn't something I
want to be part of the API.


If you want me to change these, please allow me to leave the const in as a
refactor tool, since the other styles don't lend themselves well to automatic
search/replace.

http://codereview.chromium.org/848006/diff/58002/59100#newcode182
net/base/net_log.h:182: // bounded buffer.
On 2010/03/12 23:52:35, willchan wrote:
> Is this only for tests?  I wonder if it belongs in the net_log_unittest.h.

This is used by both tests and non-tests.
(SingleThreadedProxyResolver and ClientSocketPoolBase use this to buffer up
events, and later "append" them to the stream. I would like to get rid of this
behavior, but I think it is best done in follow-up changes. The socket pool
consumer will be easier to change since we just need to give a different
source_id to the connect job, and then reference it after it is bound... but the
proxy resolver and host resolver cases will require a thread bridge or making
the log threadsafe).

http://codereview.chromium.org/848006/diff/58002/59100#newcode219
net/base/net_log.h:219: CapturingBoundNetLog() : capturing_net_log_(new
CapturingNetLog) {}
On 2010/03/12 23:52:35, willchan wrote:
> same comment about default parameter value

This is used primarily by unittests, and it got very tedious adding the default
argument.

[eroman, 2010-03-15 03:23:25]

I also changed the client_socket_pool_base mechanism for logging (so it sends it
directly to the NetLog, rather than buffering it until the job completes)...
this required some changes to the ChromeNetLog so it can re-group things. Next I
will have to fix some conflict's from Mike's change to that same file

http://codereview.chromium.org/848006/diff/58002/59116
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/58002/59116#newcode7
chrome/browser/net/chrome_net_log.cc:7: #include
"chrome/browser/net/chrome_net_log.h"
On 2010/03/12 23:52:35, willchan wrote:
> This should be first

Done.

http://codereview.chromium.org/848006/diff/58002/59116#newcode62
chrome/browser/net/chrome_net_log.cc:62:
ChromeNetLog::RequestTracker::RequestTracker(net::NetLog::SourceType
source_type)
On 2010/03/12 23:52:35, willchan wrote:
> 80 cols

Done.

http://codereview.chromium.org/848006/diff/58002/59113
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59113#newcode46
chrome/browser/net/chrome_net_log.h:46: RequestTracker(net::NetLog::SourceType
source_type);
On 2010/03/12 23:52:35, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/848006/diff/58002/59113#newcode51
chrome/browser/net/chrome_net_log.h:51: RequestInfoList GetLiveRequests();
On 2010/03/12 23:52:35, willchan wrote:
> This sounds const

Done.

http://codereview.chromium.org/848006/diff/58002/59113#newcode53
chrome/browser/net/chrome_net_log.h:53: RequestInfoList GetRecentlyDeceased();
On 2010/03/12 23:52:35, willchan wrote:
> This sounds const

Done.

http://codereview.chromium.org/848006/diff/58002/59103
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/848006/diff/58002/59103#newcode15
net/base/host_resolver_impl_unittest.cc:15: #include
"net/base/net_log_unittest.h"
On 2010/03/12 23:52:35, willchan wrote:
> out of order

Done.

http://codereview.chromium.org/848006/diff/58002/59100
File net/base/net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59100#newcode28
net/base/net_log.h:28: class NetLog : public base::RefCountedThreadSafe<NetLog>
{
On 2010/03/12 23:52:35, willchan wrote:
> Does this need to be ref counted at all?  What if we instantiated a
ChromeNetLog
> as an IOThread member?  It seems to be that IOThread should own it.  I haven't
> thought through the lifetime issues with the visualization frontend, but I
> figured I'd ask the question and let you think it through and explain it to
me.

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode63
net/base/net_log.h:63: // The |id| field must be unique accross all source IDs.
The |type| field is
On 2010/03/12 23:52:35, willchan wrote:
> I'm still reading through, but it seems strange that the |type| field is in
the
> SourceID struct.  I don't conceptually think of the |type| as part of an "id".

> How about naming this struct Source instead of SourceID?

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode178
net/base/net_log.h:178: scoped_refptr<NetLog> net_log_;
On 2010/03/12 23:52:35, willchan wrote:
> Can both these members be const?

If I make these const, then operator= no longer works (which is going to require
more refactoring work to overcome).

http://codereview.chromium.org/848006/diff/58002/59100#newcode189
net/base/net_log.h:189: CapturingNetLog() : next_id_(0), max_num_entries_(-1) {}
On 2010/03/12 23:52:35, willchan wrote:
> hrm, this seems equivalent to a default parameter value, which is disallowed. 
> can we just use the other constructor?

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode193
net/base/net_log.h:193: CapturingNetLog(size_t max_num_entries)
On 2010/03/12 23:52:35, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode204
net/base/net_log.h:204: 
On 2010/03/12 23:52:35, willchan wrote:
> extra newline

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode221
net/base/net_log.h:221: CapturingBoundNetLog(const NetLog::SourceID&  source_id,
On 2010/03/12 23:52:35, willchan wrote:
> extra whitespace

Done.

http://codereview.chromium.org/848006/diff/58002/59100#newcode226
net/base/net_log.h:226: CapturingBoundNetLog(size_t max_num_entries)
On 2010/03/12 23:52:35, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/848006/diff/58002/59106
File net/base/net_log_unittest.h (right):

http://codereview.chromium.org/848006/diff/58002/59106#newcode10
net/base/net_log_unittest.h:10: #include "net/base/net_log.h"
On 2010/03/12 23:52:35, willchan wrote:
> Should go first

Even for header file? Ok, done.

[willchan no longer on Chromium, 2010-03-15 06:29:50]

I'm going to sleep soon because I'm too tired to think properly, but I'm stuck
right now on the RequestTrackers.  It looks like the connect_job_tracker_ is
special cased for some reason.  I'll spend tomorrow trying to grok that.

http://codereview.chromium.org/848006/diff/58002/59106
File net/base/net_log_unittest.h (right):

http://codereview.chromium.org/848006/diff/58002/59106#newcode10
net/base/net_log_unittest.h:10: #include "net/base/net_log.h"
On 2010/03/15 03:23:25, eroman wrote:
> On 2010/03/12 23:52:35, willchan wrote:
> > Should go first
> 
> Even for header file? Ok, done.

Oops, I missed that this was in a header file.  I just saw the testing stuff and
assumed it was a test.cc file.  No, please undo that.  My bad.

http://codereview.chromium.org/848006/diff/121002/88116
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/121002/88116#newcode25
chrome/browser/net/chrome_net_log.cc:25: const size_t
ChromeNetLog::PassiveLogCollector::RequestTracker::kMaxGraveyardURLSize = 1000;
Hhahahahah, long variable name lol.  You might want to indent after the size_t
in order to fit in 80 columns.

http://codereview.chromium.org/848006/diff/121002/88116#newcode38
chrome/browser/net/chrome_net_log.cc:38:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
You should add logging.h for this.

http://codereview.chromium.org/848006/diff/121002/88116#newcode49
chrome/browser/net/chrome_net_log.cc:49: bool ChromeNetLog::HasListener() {
Can this be const?

http://codereview.chromium.org/848006/diff/121002/88113
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/848006/diff/121002/88113#newcode10
chrome/browser/net/chrome_net_log.h:10: #include "base/observer_list.h"
alphabetical order?

http://codereview.chromium.org/848006/diff/121002/88113#newcode30
chrome/browser/net/chrome_net_log.h:30: class PassiveLogCollector;
I'm not sure if it's worth keeping this as an inner class.  I'm not sure what
makes PassiveLogCollector a more special observer than any other.  It seems like
it should just be a normal class.

http://codereview.chromium.org/848006/diff/121002/88113#newcode49
chrome/browser/net/chrome_net_log.h:49: scoped_ptr<PassiveLogCollector>
passive_collector_;
Minor quibbling, so ignore if you disagree.
I wonder if the PassiveLogCollector should just directly be a member of the
IOThread rather than being a member of ChromeNetLog.  If I use ChromeNetLog for
extensions, I don't think I'd add the Extensions observer directly here.  I'm
not sure if there's a good reason for one observer to be part of ChromeNetLog
directly.

http://codereview.chromium.org/848006/diff/121002/88113#newcode50
chrome/browser/net/chrome_net_log.h:50: ObserverList<Observer> observers_;
If you want to DCHECK that all observers have been removed by shutdown, you
should add the second template parameter.

http://codereview.chromium.org/848006/diff/121002/88113#newcode63
chrome/browser/net/chrome_net_log.h:63: void Add(const Entry& entry, bool
is_unbounded);
Technically structs shouldn't have member functions with behavior.  These have
some behavior, so it's probably best to move them out elsewhere.  Perhaps they
should be moved to RequestTracker, or make it a free function (within an
anonymous namespace in the .cc file)?

http://codereview.chromium.org/848006/diff/121002/88113#newcode76
chrome/browser/net/chrome_net_log.h:76: size_t max_gravyard_size);
s/max_gravyard_size/max_graveyard_size/.

[eroman, 2010-03-15 20:07:22]

http://codereview.chromium.org/848006/diff/58002/59106
File net/base/net_log_unittest.h (right):

http://codereview.chromium.org/848006/diff/58002/59106#newcode10
net/base/net_log_unittest.h:10: #include "net/base/net_log.h"
On 2010/03/15 06:29:50, willchan wrote:
> On 2010/03/15 03:23:25, eroman wrote:
> > On 2010/03/12 23:52:35, willchan wrote:
> > > Should go first
> > 
> > Even for header file? Ok, done.
> 
> Oops, I missed that this was in a header file.  I just saw the testing stuff
and
> assumed it was a test.cc file.  No, please undo that.  My bad.

Done.

http://codereview.chromium.org/848006/diff/121002/88116
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/121002/88116#newcode25
chrome/browser/net/chrome_net_log.cc:25: const size_t
ChromeNetLog::PassiveLogCollector::RequestTracker::kMaxGraveyardURLSize = 1000;
On 2010/03/15 06:29:51, willchan wrote:
> Hhahahahah, long variable name lol.  You might want to indent after the size_t
> in order to fit in 80 columns.

No longer applicable (moved to a separate class).

http://codereview.chromium.org/848006/diff/121002/88116#newcode38
chrome/browser/net/chrome_net_log.cc:38:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
On 2010/03/15 06:29:51, willchan wrote:
> You should add logging.h for this.

Done.

http://codereview.chromium.org/848006/diff/121002/88116#newcode49
chrome/browser/net/chrome_net_log.cc:49: bool ChromeNetLog::HasListener() {
On 2010/03/15 06:29:51, willchan wrote:
> Can this be const?

Done.

http://codereview.chromium.org/848006/diff/121002/88113
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/848006/diff/121002/88113#newcode10
chrome/browser/net/chrome_net_log.h:10: #include "base/observer_list.h"
On 2010/03/15 06:29:51, willchan wrote:
> alphabetical order?

Done.

http://codereview.chromium.org/848006/diff/121002/88113#newcode30
chrome/browser/net/chrome_net_log.h:30: class PassiveLogCollector;
On 2010/03/15 06:29:51, willchan wrote:
> I'm not sure if it's worth keeping this as an inner class.  I'm not sure what
> makes PassiveLogCollector a more special observer than any other.  It seems
like
> it should just be a normal class.

Done.

http://codereview.chromium.org/848006/diff/121002/88113#newcode49
chrome/browser/net/chrome_net_log.h:49: scoped_ptr<PassiveLogCollector>
passive_collector_;
On 2010/03/15 06:29:51, willchan wrote:
> Minor quibbling, so ignore if you disagree.
> I wonder if the PassiveLogCollector should just directly be a member of the
> IOThread rather than being a member of ChromeNetLog.  If I use ChromeNetLog
for
> extensions, I don't think I'd add the Extensions observer directly here.  I'm
> not sure if there's a good reason for one observer to be part of ChromeNetLog
> directly.

I agree this would be better moved out of ChromeNetLog.
There is extra plumbing necessary to support that, I will leave it as a TODO.

http://codereview.chromium.org/848006/diff/121002/88113#newcode50
chrome/browser/net/chrome_net_log.h:50: ObserverList<Observer> observers_;
On 2010/03/15 06:29:51, willchan wrote:
> If you want to DCHECK that all observers have been removed by shutdown, you
> should add the second template parameter.

Done.

http://codereview.chromium.org/848006/diff/121002/88113#newcode63
chrome/browser/net/chrome_net_log.h:63: void Add(const Entry& entry, bool
is_unbounded);
On 2010/03/15 06:29:51, willchan wrote:
> Technically structs shouldn't have member functions with behavior.  These have
> some behavior, so it's probably best to move them out elsewhere.  Perhaps they
> should be moved to RequestTracker, or make it a free function (within an
> anonymous namespace in the .cc file)?

Done.

http://codereview.chromium.org/848006/diff/121002/88113#newcode76
chrome/browser/net/chrome_net_log.h:76: size_t max_gravyard_size);
On 2010/03/15 06:29:51, willchan wrote:
> s/max_gravyard_size/max_graveyard_size/.

Done.

[willchan no longer on Chromium, 2010-03-16 01:46:24]

LGTM

I've run out of steam, but the design looks good and I added comments for
anything I saw in a file that jumped out at me, but I definitely didn't give
everything a detailed look.

We've discussed this offline, and the direction is correct, although we'll need
more changes down the line.  There's an open question as to whether it makes
sense to have multiple PassiveLogCollectors (per URLRequestContext) or to have
one.  Also, I've noted that BoundNetLog's methods shouldn't all be const.  That
should be addressed in a followup changelist.

I'm excited for this change.  This should help us have much more visibility into
our networking code and allow for cool visualizations.

http://codereview.chromium.org/848006/diff/58002/59100
File net/base/net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59100#newcode160
net/base/net_log.h:160: void AddEvent(NetLog::EventType event_type) const;
On 2010/03/13 00:38:05, eroman wrote:
> On 2010/03/12 23:52:35, willchan wrote:
> > In situations like this, I usually think that you should inline the
> definitions
> > to make it obvious that they're just simple wrappers. 
> 
> Sure I can do that.
> Actually, I originally had it that way but found it made the header less
> readable because of many lines of pollution.
> 
> > Also, I don't think they should be const.
> > BoundNetLog is conceptually basically the same thing as
> > NetLog, it just binds the SourceID.  If that's the case, then why is the
> > AddEvent() member function (conceptually a mutating function) non-const?  I
> know
> > C++ lets you get away with this, but I think we should keep these as
non-const
> > member functions.
> 
> Heh, I knew this would come up :)
> 
> Basically the issue is how to pass around BoundNetLog into all of the
consumers.
> The cleanest way IMO was to have callers take a |const BoundNetLog& log|
> parameter, which they could then use to initialize their own BoundNetLog
member
> variable. However, there were also a lot of callsites which wanted to log
> directly into their function argument... having the log methods be non-const
> meant needing to make a temporary copy of the parameter, give it a new name,
and
> then call the method that "writes" into it. This quickly lead to widespread
> uglification, and seemed very artificial since effectively we are still
logging
> into that destination. So I decided to avoid the problem by making all the
> methods const as well so they could be called directly. In this sense I think
of
> BoundNetLog like a handle to a log; the handle always points to the same thing
> (immutable), but you can still do actions on its destination.
> 
> I considered some other alternatives:
> 
> (1) Make the function arguments |BoundNetLog& log|. This goes against google
> style, and is wonky.
> 
> (2) Make the function arguments |BoundNetLog log|. This means each caller gets
a
> copy. This is workable, but not desirable from a performance standpoint since
we
> are doing extra AddRef/Release (on threadsaferefcounted) for no good reason.
> 
> (3) Make the function arguments |BoundNetLog* log|. I don't like this approach
> because it means callers can pass in NULL, and I don't want to handle NULLs
(in
> fact I am trying to get rid of all the NULLs). It also means callers can
mutate
> the log itself (replace it with something else) which really isn't something I
> want to be part of the API.

I think I prefer (3) here.  I'd document this by using DCHECK(log).  I'm fine if
you want to use this as a refactoring tool, but I don't think we should use
const just so we can use a const reference to show that it can't be NULL.  I
think assertions are the way to go.  We have plenty of other code that assumes
some pointers are never NULL.

> 
> 
> If you want me to change these, please allow me to leave the const in as a
> refactor tool, since the other styles don't lend themselves well to automatic
> search/replace.

http://codereview.chromium.org/848006/diff/189001/190124
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/848006/diff/189001/190124#newcode18
chrome/browser/io_thread.h:18: class ChromeNetLog;
OCD, but perhaps sort ChromeNetLog alphabetically with Listvalue.

http://codereview.chromium.org/848006/diff/189001/190115
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/189001/190115#newcode14
chrome/browser/net/chrome_net_log.cc:14: 
extra newline

http://codereview.chromium.org/848006/diff/189001/190085
File net/base/host_resolver.h (right):

http://codereview.chromium.org/848006/diff/189001/190085#newcode145
net/base/host_resolver.h:145: const BoundNetLog& net_log) = 0;
I think this shouldn't be const.

http://codereview.chromium.org/848006/diff/189001/190085#newcode201
net/base/host_resolver.h:201: const BoundNetLog& net_log);
I think this shouldn't be const either.

http://codereview.chromium.org/848006/diff/189001/190103
File net/base/load_log_unittest.cc (right):

http://codereview.chromium.org/848006/diff/189001/190103#newcode2
net/base/load_log_unittest.cc:2: // Use of this source code is governed by a
BSD-style license that can be
Can't we delete this file?

http://codereview.chromium.org/848006/diff/189001/190023
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/848006/diff/189001/190023#newcode324
net/proxy/single_threaded_proxy_resolver.cc:324:
job->net_log()->EndEvent(NetLog::TYPE_WAITING_FOR_SINGLE_PROXY_RESOLVER_THREAD);
80 columns

http://codereview.chromium.org/848006/diff/189001/190068
File net/socket/client_socket.h (right):

http://codereview.chromium.org/848006/diff/189001/190068#newcode31
net/socket/client_socket.h:31: virtual int Connect(CompletionCallback* callback,
const BoundNetLog& net_log) = 0;
80 columns

http://codereview.chromium.org/848006/diff/189001/190064
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/848006/diff/189001/190064#newcode18
net/socket/ssl_client_socket_mac.cc:18: #include "net/base/net_log.h"
alphabetical

http://codereview.chromium.org/848006/diff/189001/190078
File net/url_request/url_request.h (right):

http://codereview.chromium.org/848006/diff/189001/190078#newcode512
net/url_request/url_request.h:512: const net::BoundNetLog& net_log() { return
net_log_; }
The accessor can be const, right?

http://codereview.chromium.org/848006/diff/189001/190083
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/848006/diff/189001/190083#newcode44
net/url_request/url_request_context.h:44: net::NetLog* net_log() const {
I think this should be non-const.

[eroman, 2010-03-16 02:58:44]

I made the RequestTracker split change we discussed.

http://codereview.chromium.org/848006/diff/58002/59100
File net/base/net_log.h (right):

http://codereview.chromium.org/848006/diff/58002/59100#newcode160
net/base/net_log.h:160: void AddEvent(NetLog::EventType event_type) const;
On 2010/03/16 01:46:25, willchan wrote:
> On 2010/03/13 00:38:05, eroman wrote:
> > On 2010/03/12 23:52:35, willchan wrote:
> > > In situations like this, I usually think that you should inline the
> > definitions
> > > to make it obvious that they're just simple wrappers. 
> > 
> > Sure I can do that.
> > Actually, I originally had it that way but found it made the header less
> > readable because of many lines of pollution.
> > 
> > > Also, I don't think they should be const.
> > > BoundNetLog is conceptually basically the same thing as
> > > NetLog, it just binds the SourceID.  If that's the case, then why is the
> > > AddEvent() member function (conceptually a mutating function) non-const? 
I
> > know
> > > C++ lets you get away with this, but I think we should keep these as
> non-const
> > > member functions.
> > 
> > Heh, I knew this would come up :)
> > 
> > Basically the issue is how to pass around BoundNetLog into all of the
> consumers.
> > The cleanest way IMO was to have callers take a |const BoundNetLog& log|
> > parameter, which they could then use to initialize their own BoundNetLog
> member
> > variable. However, there were also a lot of callsites which wanted to log
> > directly into their function argument... having the log methods be non-const
> > meant needing to make a temporary copy of the parameter, give it a new name,
> and
> > then call the method that "writes" into it. This quickly lead to widespread
> > uglification, and seemed very artificial since effectively we are still
> logging
> > into that destination. So I decided to avoid the problem by making all the
> > methods const as well so they could be called directly. In this sense I
think
> of
> > BoundNetLog like a handle to a log; the handle always points to the same
thing
> > (immutable), but you can still do actions on its destination.
> > 
> > I considered some other alternatives:
> > 
> > (1) Make the function arguments |BoundNetLog& log|. This goes against google
> > style, and is wonky.
> > 
> > (2) Make the function arguments |BoundNetLog log|. This means each caller
gets
> a
> > copy. This is workable, but not desirable from a performance standpoint
since
> we
> > are doing extra AddRef/Release (on threadsaferefcounted) for no good reason.
> > 
> > (3) Make the function arguments |BoundNetLog* log|. I don't like this
approach
> > because it means callers can pass in NULL, and I don't want to handle NULLs
> (in
> > fact I am trying to get rid of all the NULLs). It also means callers can
> mutate
> > the log itself (replace it with something else) which really isn't something
I
> > want to be part of the API.
> 
> I think I prefer (3) here.  I'd document this by using DCHECK(log).  I'm fine
if
> you want to use this as a refactoring tool, but I don't think we should use
> const just so we can use a const reference to show that it can't be NULL.  I
> think assertions are the way to go.  We have plenty of other code that assumes
> some pointers are never NULL.
> 
> > 
> > 
> > If you want me to change these, please allow me to leave the const in as a
> > refactor tool, since the other styles don't lend themselves well to
automatic
> > search/replace.
> 
> 

Ok, I will work on this post-commit.

http://codereview.chromium.org/848006/diff/189001/190124
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/848006/diff/189001/190124#newcode18
chrome/browser/io_thread.h:18: class ChromeNetLog;
On 2010/03/16 01:46:25, willchan wrote:
> OCD, but perhaps sort ChromeNetLog alphabetically with Listvalue.

Done.

http://codereview.chromium.org/848006/diff/189001/190115
File chrome/browser/net/chrome_net_log.cc (right):

http://codereview.chromium.org/848006/diff/189001/190115#newcode14
chrome/browser/net/chrome_net_log.cc:14: 
On 2010/03/16 01:46:25, willchan wrote:
> extra newline

Done.

http://codereview.chromium.org/848006/diff/189001/190103
File net/base/load_log_unittest.cc (right):

http://codereview.chromium.org/848006/diff/189001/190103#newcode2
net/base/load_log_unittest.cc:2: // Use of this source code is governed by a
BSD-style license that can be
On 2010/03/16 01:46:25, willchan wrote:
> Can't we delete this file?

Oops, thought I had svn deleted it already!
Done.

http://codereview.chromium.org/848006/diff/189001/190023
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/848006/diff/189001/190023#newcode324
net/proxy/single_threaded_proxy_resolver.cc:324:
job->net_log()->EndEvent(NetLog::TYPE_WAITING_FOR_SINGLE_PROXY_RESOLVER_THREAD);
On 2010/03/16 01:46:25, willchan wrote:
> 80 columns

Done.

http://codereview.chromium.org/848006/diff/189001/190064
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/848006/diff/189001/190064#newcode18
net/socket/ssl_client_socket_mac.cc:18: #include "net/base/net_log.h"
On 2010/03/16 01:46:25, willchan wrote:
> alphabetical

Done.

http://codereview.chromium.org/848006/diff/189001/190078
File net/url_request/url_request.h (right):

http://codereview.chromium.org/848006/diff/189001/190078#newcode512
net/url_request/url_request.h:512: const net::BoundNetLog& net_log() { return
net_log_; }
On 2010/03/16 01:46:25, willchan wrote:
> The accessor can be const, right?

Done.