Add a DebugVisitor to QuicConnection to facilitate logging the activity on a QUIC connection.

Add a DebugVisitor to QuicConnection to facilitate logging the activity on a QUIC connection.

Merge internal change: 42368829


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182685

[jar (doing other things), 2013-02-14 17:13:19]

LGTM... but I had a question below.

https://codereview.chromium.org/12254037/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/12254037/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:572: debug_visitor_->OnPacketReceived(self_address,
peer_address, packet);
nit/comment: It is IMO common for the observer to want to be reminded which
instance is reporting this event.  As a result, for many of these interfaces, it
is tempting to pass a const ref to |*this|.   This method is a good example,
where you could have probably called after line 578, and then the instance would
have most of the parameters that you're handing off. [...but perhaps you wanted
to know what the "old" values were??].

I guess it could be that you'd only have one connection being "debuged" at a
time... or you could store a pointer to  |*this| inside a slot in your specific
|debug_visitor_|. 

Was the "typical" plan one of those... or something else?

[Ryan Hamilton, 2013-02-14 17:46:19]

Thanks

https://codereview.chromium.org/12254037/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/12254037/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:572: debug_visitor_->OnPacketReceived(self_address,
peer_address, packet);
On 2013/02/14 17:13:19, jar wrote:
> nit/comment: It is IMO common for the observer to want to be reminded which
> instance is reporting this event.  As a result, for many of these interfaces,
it
> is tempting to pass a const ref to |*this|.   This method is a good example,
> where you could have probably called after line 578, and then the instance
would
> have most of the parameters that you're handing off. [...but perhaps you
wanted
> to know what the "old" values were??].
> 
> I guess it could be that you'd only have one connection being "debuged" at a
> time... or you could store a pointer to  |*this| inside a slot in your
specific
> |debug_visitor_|. 
> 
> Was the "typical" plan one of those... or something else?

*nod*  Definitely something to consider.  At the moment, I expect there to be a
1:1 mapping between the observer (as you call it) and the connection.  As such,
storing connection_ would work well.  In the follow up CL I create a
QuicConnectionLogger class which is a subclass of the observer.  This doesn't
store a pointer to the connection, but instead it stores a BoundNetLog whose
source in the Session (which owns the connection).  So it's basically doing the
same thing.  If we ever decided that we wanted to have an N:1 mapping, I would
definitely add |this| to these methods.

[commit-bot: I haz the power, 2013-02-14 17:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/12254037/1

[commit-bot: I haz the power, 2013-02-14 22:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/12254037/1

[commit-bot: I haz the power, 2013-02-15 13:08:07]

Change committed as 182685