Add net log entries that summarize transmit and receive byte counts.

Add net log entries that summarize transmit and receive byte counts.

Tx/Rx summaries are integrated into the net log at the last point that bytes were transmitted or received.

Hopefully this will help resolve http://crbug.com/37729 by showing if we've received bytes over the network when we hit the "Waiting for cache" bug.

This change also modernizes the use of NetLog:
 - ClientSocket now has a net_log() accessor
 - ClientSocket::Connect no longer takes a NetLog, instead the TCPClientSocket constructor takes one, others use their transport socket's NetLog
 - TCPClientSocket creates a new source id with source type SOCKET

Also updates PassiveLogCollector infrastructure:
 - The LiveRequestsObserver lets a RequestTracker update a RequestInfo just before it is displayed.  This allows ConnectJobs to be associated with URLRequests while connecting and then reassociated if they are late-bound to a different request.

BUG=37729
TEST=tx/rx lines show up in chrome://net-internals/

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

[vandebo (ex-Chrome), 2010-04-21 02:29:41]

Eric, this should be a lot closer to what you expected.  Thanks.

[eroman, 2010-04-21 18:32:22]

Nice change! (thanks for doing this)

I have reviewed everything except PassiveLogCollector -- I will do that as soon
as I get back from lunch :)

So far, looks good.

http://codereview.chromium.org/1696005/diff/2001/3015
File net/socket/client_socket.h (right):

http://codereview.chromium.org/1696005/diff/2001/3015#newcode55
net/socket/client_socket.h:55: virtual const BoundNetLog& net_log() const = 0;
nit: we only use hacker_style() for accessors. Since this is a virtual, please
use CamelCase().

http://codereview.chromium.org/1696005/diff/2001/3018
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3018#newcode48
net/socket/client_socket_pool_base.cc:48: if (delegate_ && !idle_) {
Ah, thanks for fixing that.

http://codereview.chromium.org/1696005/diff/2001/3018#newcode237
net/socket/client_socket_pool_base.cc:237:
NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID, job_net_log.source().id);
Oh I see. So you initially associate it with the connect job created in response
to the request, but may re-associate it later when handing out... I like it!

http://codereview.chromium.org/1696005/diff/2001/3020
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3020#newcode67
net/socket/client_socket_pool_base_unittest.cc:67: return net_log_;
nit: I reckon you can return BoundNetLog() here instead, and do away with the
|net_log_| member (since it is unused).

http://codereview.chromium.org/1696005/diff/2001/3023
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3023#newcode63
net/socket/socks5_client_socket.cc:63:
net_log_(transport_socket->socket()->net_log()) {
I reckon we probably will want to make this its own Source (separate from the
transport socket).

This is fine for this CL, just thinking out loud.

http://codereview.chromium.org/1696005/diff/2001/3025
File net/socket/socks5_client_socket_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3025#newcode133
net/socket/socks5_client_socket_unittest.cc:133:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
While you are here, can you fix this long line?

http://codereview.chromium.org/1696005/diff/2001/3025#newcode139
net/socket/socks5_client_socket_unittest.cc:139:
EXPECT_TRUE(LogContainsEndEvent(net_log_.entries(), -1,
NetLog::TYPE_SOCKS5_CONNECT));
While you are here, can you fix this long line?

http://codereview.chromium.org/1696005/diff/2001/3025#newcode248
net/socket/socks5_client_socket_unittest.cc:248:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
Ditto.

http://codereview.chromium.org/1696005/diff/2001/3025#newcode252
net/socket/socks5_client_socket_unittest.cc:252:
EXPECT_TRUE(LogContainsEndEvent(net_log_.entries(), -1,
NetLog::TYPE_SOCKS5_CONNECT));
Ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode271
net/socket/socks5_client_socket_unittest.cc:271:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode275
net/socket/socks5_client_socket_unittest.cc:275:
EXPECT_TRUE(LogContainsEndEvent(net_log_.entries(), -1,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode295
net/socket/socks5_client_socket_unittest.cc:295:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode295
net/socket/socks5_client_socket_unittest.cc:295:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode299
net/socket/socks5_client_socket_unittest.cc:299:
EXPECT_TRUE(LogContainsEndEvent(net_log_.entries(), -1,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode321
net/socket/socks5_client_socket_unittest.cc:321:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3025#newcode325
net/socket/socks5_client_socket_unittest.cc:325:
EXPECT_TRUE(LogContainsEndEvent(net_log_.entries(), -1,
NetLog::TYPE_SOCKS5_CONNECT));
ditto

http://codereview.chromium.org/1696005/diff/2001/3035
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3035#newcode72
net/socket/ssl_client_socket_unittest.cc:72: net::ClientSocket *transport = new
net::TCPClientSocket(addr, &log);
While you are here, can you fix the position of the '*' ?

http://codereview.chromium.org/1696005/diff/2001/3035#newcode116
net/socket/ssl_client_socket_unittest.cc:116: net::ClientSocket *transport = new
net::TCPClientSocket(addr, &log);
ditto.

http://codereview.chromium.org/1696005/diff/2001/3035#newcode161
net/socket/ssl_client_socket_unittest.cc:161: net::ClientSocket *transport = new
net::TCPClientSocket(addr, &log);
ditto

http://codereview.chromium.org/1696005/diff/2001/3035#newcode212
net/socket/ssl_client_socket_unittest.cc:212: net::ClientSocket *transport = new
net::TCPClientSocket(addr, NULL);
ditto

http://codereview.chromium.org/1696005/diff/2001/3035#newcode274
net/socket/ssl_client_socket_unittest.cc:274: net::ClientSocket *transport = new
net::TCPClientSocket(addr, NULL);
ditto

http://codereview.chromium.org/1696005/diff/2001/3035#newcode333
net/socket/ssl_client_socket_unittest.cc:333: net::ClientSocket *transport = new
net::TCPClientSocket(addr, NULL);
ditto

http://codereview.chromium.org/1696005/diff/2001/3035#newcode387
net/socket/ssl_client_socket_unittest.cc:387: net::ClientSocket *transport = new
net::TCPClientSocket(addr, NULL);
ditto

http://codereview.chromium.org/1696005/diff/2001/3041
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3041#newcode67
net/socket/tcp_client_socket_pool_unittest.cc:67: BoundNetLog net_log_;
I suggest removing this, as it is unused.

http://codereview.chromium.org/1696005/diff/2001/3041#newcode108
net/socket/tcp_client_socket_pool_unittest.cc:108: BoundNetLog net_log_;
Ditto.

[eroman, 2010-04-21 19:56:49]

I just ran this change locally, and got a crash:

 	chrome.dll!`anonymous namespace'::InvalidParameter()  Line 143	C++

	chrome.dll!std::vector<PassiveLogCollector::Entry,std::allocator<PassiveLogCollector::Entry>
>::operator[]()  Line 780	C++
>	chrome.dll!PassiveLogCollector::RequestTrackerBase::GetRequestInfo()  Line
227	C++
 	chrome.dll!PassiveLogCollector::SocketTracker::AppendLogEntries()  Line
385	C++
 	chrome.dll!PassiveLogCollector::RequestTracker::IntegrateSubordinateSource() 
Line 499	C++
 	chrome.dll!PassiveLogCollector::RequestTracker::DoAddEntry()  Line 474	C++
 	chrome.dll!PassiveLogCollector::RequestTrackerBase::OnAddEntry()  Line 158	C++
 	chrome.dll!PassiveLogCollector::OnAddEntry()  Line 104	C++
 	chrome.dll!ChromeNetLog::AddEntry()  Line 35	C++
 	chrome.dll!net::BoundNetLog::AddEntry()  Line 35	C++
 	chrome.dll!net::BoundNetLog::EndEventWithParameters()  Line 97	C++
 	chrome.dll!net::BoundNetLog::EndEvent()  Line 91	C++
 	chrome.dll!URLRequest::set_context()  Line 516	C++
 	chrome.dll!URLRequest::~URLRequest()  Line 64	C++
 	chrome.dll!URLRequest::`scalar deleting destructor'() 	C++
 	chrome.dll!URLFetcher::Core::OnReadCompleted()  Line 202	C++
 	chrome.dll!URLFetcher::Core::OnResponseStarted()  Line 180	C++
 	chrome.dll!URLRequest::ResponseStarted()  Line 380	C++
 	chrome.dll!URLRequestJob::NotifyHeadersComplete()  Line 430	C++
 	chrome.dll!URLRequestHttpJob::NotifyHeadersComplete()  Line 597	C++
 	chrome.dll!URLRequestHttpJob::SaveNextCookie()  Line 755	C++
 	chrome.dll!URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete()  Line
747	C++
 	chrome.dll!URLRequestHttpJob::OnStartCompleted()  Line 508	C++


i=1

The problem is, entries is empty, so index 0 is undefined.

    if (graveyard_[i].entries[0].source.id == source_id) {   <----- Crash here.
      return &graveyard_[i];
    }

http://codereview.chromium.org/1696005/diff/2001/3005
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/1696005/diff/2001/3005#newcode112
chrome/browser/net/passive_log_collector.h:112: // Find a request, either in the
live entries or the graveyard and return
nit: Find -> Finds, return -> returns.

http://codereview.chromium.org/1696005/diff/2001/3009
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/1696005/diff/2001/3009#newcode69
chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h:69:
net::BoundNetLog net_log_;
this is unused, i suggest removing it.

[vandebo (ex-Chrome), 2010-04-21 21:21:22]

I hadn't seen that crash, but I've addressed it.

http://codereview.chromium.org/1696005/diff/2001/3005
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/1696005/diff/2001/3005#newcode112
chrome/browser/net/passive_log_collector.h:112: // Find a request, either in the
live entries or the graveyard and return
On 2010/04/21 19:56:49, eroman wrote:
> nit: Find -> Finds, return -> returns.

Done.

http://codereview.chromium.org/1696005/diff/2001/3015
File net/socket/client_socket.h (right):

http://codereview.chromium.org/1696005/diff/2001/3015#newcode55
net/socket/client_socket.h:55: virtual const BoundNetLog& net_log() const = 0;
On 2010/04/21 18:32:22, eroman wrote:
> nit: we only use hacker_style() for accessors. Since this is a virtual, please
> use CamelCase().

Done.

http://codereview.chromium.org/1696005/diff/2001/3020
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3020#newcode67
net/socket/client_socket_pool_base_unittest.cc:67: return net_log_;
On 2010/04/21 18:32:22, eroman wrote:
> nit: I reckon you can return BoundNetLog() here instead, and do away with the
> |net_log_| member (since it is unused).

Can't return a reference to a temporary.

http://codereview.chromium.org/1696005/diff/2001/3023
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3023#newcode63
net/socket/socks5_client_socket.cc:63:
net_log_(transport_socket->socket()->net_log()) {
On 2010/04/21 18:32:22, eroman wrote:
> I reckon we probably will want to make this its own Source (separate from the
> transport socket).
> This is fine for this CL, just thinking out loud.

That was my thinking as well.

http://codereview.chromium.org/1696005/diff/2001/3025
File net/socket/socks5_client_socket_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3025#newcode133
net/socket/socks5_client_socket_unittest.cc:133:
EXPECT_TRUE(LogContainsBeginEvent(net_log_.entries(), 0,
NetLog::TYPE_SOCKS5_CONNECT));
On 2010/04/21 18:32:22, eroman wrote:
> While you are here, can you fix this long line?

All done.

http://codereview.chromium.org/1696005/diff/2001/3035
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3035#newcode72
net/socket/ssl_client_socket_unittest.cc:72: net::ClientSocket *transport = new
net::TCPClientSocket(addr, &log);
On 2010/04/21 18:32:22, eroman wrote:
> While you are here, can you fix the position of the '*' ?

All done.

http://codereview.chromium.org/1696005/diff/2001/3041
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3041#newcode67
net/socket/tcp_client_socket_pool_unittest.cc:67: BoundNetLog net_log_;
On 2010/04/21 18:32:22, eroman wrote:
> I suggest removing this, as it is unused.

Need a non temporary reference for the compiler to be happy.

[eroman, 2010-04-21 21:42:37]

http://codereview.chromium.org/1696005/diff/2001/3004
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3004#newcode190
chrome/browser/net/passive_log_collector.cc:190: OnLiveRequest(&list.back(),
req_it->first));
Doesn't this suffer the problem that if GetLiveRequests is called multiple
times, the same data will get appended multiple times? (Since
IntegrateSubordinateSource is called again).

http://codereview.chromium.org/1696005/diff/2001/3004#newcode301
chrome/browser/net/passive_log_collector.cc:301: if (entry.type ==
net::NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID) {
I don't think this is reachable -- SOCKET_POOL_CONNECT_JOB_ID is not emitted to
the SOURCE_CONNECT_JOB.

http://codereview.chromium.org/1696005/diff/2001/3004#newcode303
chrome/browser/net/passive_log_collector.cc:303: } else if (entry.type !=
net::NetLog::TYPE_SOCKET_POOL_SOCKET_ID) {
I don't think this is reachable either... AFAICT, TYPE_SOCKET_POOL_SOCKET_ID is
never emitted to SOURCE_CONNECT_JOB.

http://codereview.chromium.org/1696005/diff/2001/3018
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3018#newcode237
net/socket/client_socket_pool_base.cc:237:
NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID, job_net_log.source().id);
On 2010/04/21 18:32:22, eroman wrote:
> Oh I see. So you initially associate it with the connect job created in
response
> to the request, but may re-associate it later when handing out... I like it!

I take that back. It doesn't look like you handle the case where late binding
assigns us a different connect job then the one we attached initially.

[eroman, 2010-04-21 21:59:11]

http://codereview.chromium.org/1696005/diff/15001/16004
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/15001/16004#newcode374
chrome/browser/net/passive_log_collector.cc:374: case
net::NetLog::TYPE_TCP_CLOSE:
I don't think this is the right signal -- looking more in detail at the socket
implementations, TYPE_TCP_CLOSE can be emitted multiple times before the socket
is really established (i.e. for windows each attempt to try a new address in the
AddressList can emit a close).

I think a better solution is to have a signal that correspond with "this socket
is being deleted", and keep it alive until that happens. (i.e. emit that from
the destructor).

Note that this is basically what REQUEST_ALIVE [capture phase end] does for
URLRequest/SocketStream, and it also acts as a safety net in case logic forget
to emit a "close" along some code path, so the passive requests can never grow
unbounded.

(Also it should be possible in theory to call connect / disconnect multiple
times on one "Socket").

[vandebo (ex-Chrome), 2010-04-21 23:59:31]

http://codereview.chromium.org/1696005/diff/2001/3004
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3004#newcode190
chrome/browser/net/passive_log_collector.cc:190: OnLiveRequest(&list.back(),
req_it->first));
On 2010/04/21 21:42:37, eroman wrote:
> Doesn't this suffer the problem that if GetLiveRequests is called multiple
> times, the same data will get appended multiple times? (Since
> IntegrateSubordinateSource is called again).

Nope.  We integrate after adding it to the list, which creates a copy, so this
integrate is thrown away after it's used.

http://codereview.chromium.org/1696005/diff/2001/3004#newcode301
chrome/browser/net/passive_log_collector.cc:301: if (entry.type ==
net::NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID) {
On 2010/04/21 21:42:37, eroman wrote:
> I don't think this is reachable -- SOCKET_POOL_CONNECT_JOB_ID is not emitted
to
> the SOURCE_CONNECT_JOB.

It is when sockets layer, for example with SOCKSClientSocketPool and how
SSLSocketPool will.

But this is a corner that maybe should get cleaned up later.  In the same scope
as giving sockets that layer their own id.

http://codereview.chromium.org/1696005/diff/2001/3004#newcode303
chrome/browser/net/passive_log_collector.cc:303: } else if (entry.type !=
net::NetLog::TYPE_SOCKET_POOL_SOCKET_ID) {
On 2010/04/21 21:42:37, eroman wrote:
> I don't think this is reachable either... AFAICT, TYPE_SOCKET_POOL_SOCKET_ID
is
> never emitted to SOURCE_CONNECT_JOB.

Same here.

http://codereview.chromium.org/1696005/diff/2001/3018
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3018#newcode237
net/socket/client_socket_pool_base.cc:237:
NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID, job_net_log.source().id);
On 2010/04/21 21:42:37, eroman wrote:
> On 2010/04/21 18:32:22, eroman wrote:
> > Oh I see. So you initially associate it with the connect job created in
> response
> > to the request, but may re-associate it later when handing out... I like it!
> 
> I take that back. It doesn't look like you handle the case where late binding
> assigns us a different connect job then the one we attached initially.

Arg - I had this worked out in my previous (ugly) approach.  You're right, it
doesn't work correctly here.

I'll get back to you with a fix - I think I need to add another message and a
bit more state in the tracker.

[vandebo (ex-Chrome), 2010-04-22 07:06:52]

http://codereview.chromium.org/1696005/diff/2001/3018
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/1696005/diff/2001/3018#newcode237
net/socket/client_socket_pool_base.cc:237:
NetLog::TYPE_SOCKET_POOL_CONNECT_JOB_ID, job_net_log.source().id);
On 2010/04/21 21:42:37, eroman wrote:
> On 2010/04/21 18:32:22, eroman wrote:
> > Oh I see. So you initially associate it with the connect job created in
> response
> > to the request, but may re-associate it later when handing out... I like it!
> 
> I take that back. It doesn't look like you handle the case where late binding
> assigns us a different connect job then the one we attached initially.

Done.

http://codereview.chromium.org/1696005/diff/15001/16004
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/15001/16004#newcode374
chrome/browser/net/passive_log_collector.cc:374: case
net::NetLog::TYPE_TCP_CLOSE:
On 2010/04/21 21:59:11, eroman wrote:
> I don't think this is the right signal -- looking more in detail at the socket
> implementations, TYPE_TCP_CLOSE can be emitted multiple times before the
socket
> is really established (i.e. for windows each attempt to try a new address in
the
> AddressList can emit a close).
> 
> I think a better solution is to have a signal that correspond with "this
socket
> is being deleted", and keep it alive until that happens. (i.e. emit that from
> the destructor).
> 
> Note that this is basically what REQUEST_ALIVE [capture phase end] does for
> URLRequest/SocketStream, and it also acts as a safety net in case logic forget
> to emit a "close" along some code path, so the passive requests can never grow
> unbounded.
> 
> (Also it should be possible in theory to call connect / disconnect multiple
> times on one "Socket").

Done.

http://codereview.chromium.org/1696005/diff/43001/23005#newcode192
chrome/browser/net/passive_log_collector.cc:192:
std::sort(list.back().entries.begin(), list.back().entries.end(),
Would you like me to replace this with a virtual method call?

[eroman, 2010-04-22 21:46:08]

lgtm

http://codereview.chromium.org/1696005/diff/61001/5007
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/61001/5007#newcode326
chrome/browser/net/passive_log_collector.cc:326: StringPrintf("Used ConnectJob
id=%d", connect_id));
i think %u may be more appropriate for a uint32.

http://codereview.chromium.org/1696005/diff/61001/5007#newcode355
chrome/browser/net/passive_log_collector.cc:355: uint32 int_arg;
nit: perhaps this should just be |int| since the stored parameters is an int.

http://codereview.chromium.org/1696005/diff/61001/5007#newcode388
chrome/browser/net/passive_log_collector.cc:388: Entry new_entry(0,
net::NetLog::TYPE_TODO_STRING, base::TimeTicks(),
i wander if we shouldn't use a different |order| than 0 here.

http://codereview.chromium.org/1696005/diff/61001/5008
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/1696005/diff/61001/5008#newcode64
chrome/browser/net/passive_log_collector.h:64: uint32 last_tx_rx_position;
can you add a comment for this?

http://codereview.chromium.org/1696005/diff/61001/5008#newcode65
chrome/browser/net/passive_log_collector.h:65: base::TimeTicks last_tx_rx_time;
can you add a comment for this?

http://codereview.chromium.org/1696005/diff/61001/5008#newcode105
chrome/browser/net/passive_log_collector.h:105: virtual void
OnLiveRequest(RequestInfo* info) const {}
Can you add a comment for this?

[vandebo (ex-Chrome), 2010-04-23 01:55:31]

Thanks for the review eric.  I'll submit after green trybots.

http://codereview.chromium.org/1696005/diff/61001/5007
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1696005/diff/61001/5007#newcode326
chrome/browser/net/passive_log_collector.cc:326: StringPrintf("Used ConnectJob
id=%d", connect_id));
On 2010/04/22 21:46:09, eroman wrote:
> i think %u may be more appropriate for a uint32.

Done.

http://codereview.chromium.org/1696005/diff/61001/5007#newcode355
chrome/browser/net/passive_log_collector.cc:355: uint32 int_arg;
On 2010/04/22 21:46:09, eroman wrote:
> nit: perhaps this should just be |int| since the stored parameters is an int.

Done.

http://codereview.chromium.org/1696005/diff/61001/5007#newcode388
chrome/browser/net/passive_log_collector.cc:388: Entry new_entry(0,
net::NetLog::TYPE_TODO_STRING, base::TimeTicks(),
On 2010/04/22 21:46:09, eroman wrote:
> i wander if we shouldn't use a different |order| than 0 here.

We don't expect this code to run very frequently - Just after the data has been
cleared or if something has gone wrong. I don't think it's worth adding an
accesser to get the current value and I'm not sure UINT_MAX is particularly
better.  Thoughts?

http://codereview.chromium.org/1696005/diff/61001/5008
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/1696005/diff/61001/5008#newcode64
chrome/browser/net/passive_log_collector.h:64: uint32 last_tx_rx_position;
On 2010/04/22 21:46:09, eroman wrote:
> can you add a comment for this?

Done.

http://codereview.chromium.org/1696005/diff/61001/5008#newcode65
chrome/browser/net/passive_log_collector.h:65: base::TimeTicks last_tx_rx_time;
On 2010/04/22 21:46:09, eroman wrote:
> can you add a comment for this?

Done.

http://codereview.chromium.org/1696005/diff/61001/5008#newcode105
chrome/browser/net/passive_log_collector.h:105: virtual void
OnLiveRequest(RequestInfo* info) const {}
On 2010/04/22 21:46:09, eroman wrote:
> Can you add a comment for this?

Done.