Add logging for headers sent and received in BidirectionalStreamQuicImpl

Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be
shared with BidirectionalStreamQuicImpl.

BUG=596680
Committed: https://crrev.com/5fafe14b17f8ed28dc4e28adf17cbc699753413c
Cr-Commit-Position: refs/heads/master@{#382972}

[xunjieli, 2016-03-23 17:35:54]

Description was changed from

==========
Add logging for headers in BidirectionalStreamQuicImpl

BUG=
==========

to

==========
Add logging for headers in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

[xunjieli, 2016-03-23 17:35:54]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org, rch@chromium.org

[xunjieli, 2016-03-23 17:38:06]

Description was changed from

==========
Add logging for headers in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

to

==========
Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

[xunjieli, 2016-03-23 17:38:20]

PTAL. Thanks!

[xunjieli, 2016-03-23 18:53:15]

Patchset #1 (id:1) has been deleted

[mmenke, 2016-03-23 20:32:21]

LGTM

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:326: MockUDPClientSocket*
socket = new MockUDPClientSocket(
Not realted to this CL, but this should be a scoped_ptr, and should use
std::move to pass it to QuicChromiumClientSession's constructor.

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:462: const BoundTestNetLog&
net_log() const { return net_log_; }
This should be a TestNetLog

BoundTestNetLog is for when you have a class that takes a BoundNetLog created by
a higher layer.  When you have a class that takes a full NetLog, use TestNetLog.

https://codereview.chromium.org/1824403002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/1824403002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_stream.cc:112: // Log request headers to the
QuicSession's net log.
Think these comments are confusing - we're logging to the
QuicChromiumClientStream's BoundNetLog, which is called net_log_....Which does
use the same NetLog with the QuicSession, but that uses a different BoundNetLog.

I suggest just removing all 3 of them.

[Ryan Hamilton, 2016-03-23 22:09:30]

lgtm % mmenke's comments

[mmenke, 2016-03-23 22:11:59]

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:462: const BoundTestNetLog&
net_log() const { return net_log_; }
On 2016/03/23 20:32:21, mmenke wrote:
> This should be a TestNetLog
> 
> BoundTestNetLog is for when you have a class that takes a BoundNetLog created
by
> a higher layer.  When you have a class that takes a full NetLog, use
TestNetLog.

Oops...intended to delete this comment - looks like this is done elsewhere, and
the delegate->Start calls all take a BoundNetLog.  Could just create one
ourselves from the TestNetLog, but...This works.

[xunjieli, 2016-03-23 22:19:23]

Thanks for the review!

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:326: MockUDPClientSocket*
socket = new MockUDPClientSocket(
On 2016/03/23 20:32:21, mmenke wrote:
> Not realted to this CL, but this should be a scoped_ptr, and should use
> std::move to pass it to QuicChromiumClientSession's constructor.

You are right. I copied from quic_http_stream_test.cc, which needs to be changed
as well. Will do that as a  follow-up.

https://codereview.chromium.org/1824403002/diff/20001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:462: const BoundTestNetLog&
net_log() const { return net_log_; }
On 2016/03/23 22:11:59, mmenke wrote:
> On 2016/03/23 20:32:21, mmenke wrote:
> > This should be a TestNetLog
> > 
> > BoundTestNetLog is for when you have a class that takes a BoundNetLog
created
> by
> > a higher layer.  When you have a class that takes a full NetLog, use
> TestNetLog.
> 
> Oops...intended to delete this comment - looks like this is done elsewhere,
and
> the delegate->Start calls all take a BoundNetLog.  Could just create one
> ourselves from the TestNetLog, but...This works.

Acknowledged.

https://codereview.chromium.org/1824403002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/1824403002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_stream.cc:112: // Log request headers to the
QuicSession's net log.
On 2016/03/23 20:32:21, mmenke wrote:
> Think these comments are confusing - we're logging to the
> QuicChromiumClientStream's BoundNetLog, which is called net_log_....Which does
> use the same NetLog with the QuicSession, but that uses a different
BoundNetLog.
> 
> I suggest just removing all 3 of them.

Done.

[xunjieli, 2016-03-23 22:19:28]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-23 22:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1824403002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2016-03-23 22:20:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1824403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1824403002/40001

[commit-bot: I haz the power, 2016-03-23 23:33:05]

Description was changed from

==========
Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

to

==========
Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

[commit-bot: I haz the power, 2016-03-23 23:33:06]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-03-23 23:37:38]

Description was changed from

==========
Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be 
shared with BidirectionalStreamQuicImpl.

BUG=596680
==========

to

==========
Add logging for headers sent and received in BidirectionalStreamQuicImpl

This CL refactors headers logging in QuicHttpStream and moves
them to QuicChromiumClientStream so the logging can be
shared with BidirectionalStreamQuicImpl.

BUG=596680

Committed: https://crrev.com/5fafe14b17f8ed28dc4e28adf17cbc699753413c
Cr-Commit-Position: refs/heads/master@{#382972}
==========

[commit-bot: I haz the power, 2016-03-23 23:37:39]

Patchset 2 (id:??) landed as
https://crrev.com/5fafe14b17f8ed28dc4e28adf17cbc699753413c
Cr-Commit-Position: refs/heads/master@{#382972}