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

net/quic/quic_connection.cc

Index: net/quic/quic_connection.cc
diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc
index c0d2194e79006c12240135b2957c223a78222727..8dc19942fc7aa40d77fe24bb6bb632be505d70cc 100644
--- a/net/quic/quic_connection.cc
+++ b/net/quic/quic_connection.cc
@@ -98,6 +98,7 @@ QuicConnection::QuicConnection(QuicGuid guid,
       peer_least_packet_awaiting_ack_(0),
       handling_retransmission_timeout_(false),
       write_blocked_(false),
+      debug_visitor_(NULL),
       packet_creator_(guid_, &framer_),
       timeout_(QuicTime::Delta::FromMicroseconds(kDefaultTimeoutUs)),
       time_of_last_packet_(clock_->Now()),
@@ -183,6 +184,9 @@ void QuicConnection::OnPacket() {
 
 void QuicConnection::OnPublicResetPacket(
     const QuicPublicResetPacket& packet) {
+  if (debug_visitor_) {
+    debug_visitor_->OnPublicResetPacket(packet);
+  }
   CloseConnection(QUIC_PUBLIC_RESET, true);
 }
 
@@ -190,6 +194,9 @@ void QuicConnection::OnRevivedPacket() {
 }
 
 bool QuicConnection::OnPacketHeader(const QuicPacketHeader& header) {
+  if (debug_visitor_) {
+    debug_visitor_->OnPacketHeader(header);
+  }
   if (header.public_header.guid != guid_) {
     DLOG(INFO) << "Ignoring packet from unexpected GUID: "
                << header.public_header.guid << " instead of " << guid_;
@@ -222,10 +229,16 @@ void QuicConnection::OnFecProtectedPayload(StringPiece payload) {
 }
 
 void QuicConnection::OnStreamFrame(const QuicStreamFrame& frame) {
+  if (debug_visitor_) {
+    debug_visitor_->OnStreamFrame(frame);
+  }
   last_stream_frames_.push_back(frame);
 }
 
 void QuicConnection::OnAckFrame(const QuicAckFrame& incoming_ack) {
+  if (debug_visitor_) {
+    debug_visitor_->OnAckFrame(incoming_ack);
+  }
   DVLOG(1) << "Ack packet: " << incoming_ack;
 
   if (last_header_.packet_sequence_number <= largest_seen_packet_with_ack_) {
@@ -264,6 +277,9 @@ void QuicConnection::OnAckFrame(const QuicAckFrame& incoming_ack) {
 
 void QuicConnection::OnCongestionFeedbackFrame(
     const QuicCongestionFeedbackFrame& feedback) {
+  if (debug_visitor_) {
+    debug_visitor_->OnCongestionFeedbackFrame(feedback);
+  }
   congestion_manager_.OnIncomingQuicCongestionFeedbackFrame(feedback);
 }
 
@@ -438,6 +454,9 @@ void QuicConnection::OnFecData(const QuicFecData& fec) {
 }
 
 void QuicConnection::OnRstStreamFrame(const QuicRstStreamFrame& frame) {
+  if (debug_visitor_) {
+    debug_visitor_->OnRstStreamFrame(frame);
+  }
   DLOG(INFO) << "Stream reset with error "
              << QuicUtils::ErrorToString(frame.error_code);
   visitor_->OnRstStream(frame);
@@ -445,6 +464,9 @@ void QuicConnection::OnRstStreamFrame(const QuicRstStreamFrame& frame) {
 
 void QuicConnection::OnConnectionCloseFrame(
     const QuicConnectionCloseFrame& frame) {
+  if (debug_visitor_) {
+    debug_visitor_->OnConnectionCloseFrame(frame);
+  }
   DLOG(INFO) << "Connection closed with error "
              << QuicUtils::ErrorToString(frame.error_code);
   CloseConnection(frame.error_code, true);
@@ -546,6 +568,9 @@ void QuicConnection::SendRstStream(QuicStreamId id,
 void QuicConnection::ProcessUdpPacket(const IPEndPoint& self_address,
                                       const IPEndPoint& peer_address,
                                       const QuicEncryptedPacket& packet) {
+  if (debug_visitor_) {
+    debug_visitor_->OnPacketReceived(self_address, peer_address, packet);

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

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]

*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.

+  }
   last_packet_revived_ = false;
   last_size_ = packet.length();
   last_self_address_ = self_address;
@@ -942,6 +967,10 @@ void QuicConnection::MaybeProcessRevivedPacket() {
   delete group;
 
   last_packet_revived_ = true;
+  if (debug_visitor_) {
+    debug_visitor_->OnRevivedPacket(revived_header,
+                                    StringPiece(revived_payload, len));
+  }
   framer_.ProcessRevivedPacket(revived_header,
                                StringPiece(revived_payload, len));
 }