Log sent and received bytes in net::BidirectionalStream

Log sent and received bytes in net::BidirectionalStream

This CL adds logging of sent and received bytes in net::BidirectionalStream if
capturing all bytes.

BUG=602669
Committed: https://crrev.com/be07785e0727b7b70b55487cca10fcfd2e1796b0
Cr-Commit-Position: refs/heads/master@{#387425}

[xunjieli, 2016-04-13 17:47:47]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-04-13 17:49:35]

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

[xunjieli, 2016-04-13 17:49:36]

Matt, is this the right approach? Please advise. Sorry for sending you another
review.

[mmenke, 2016-04-13 18:03:50]

https://codereview.chromium.org/1883883002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/1883883002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:162:
net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, bytes_read,
TYPE_SOCKET_BYTES_SENT is used for bytes sent at the socket layer.  Should
probably just add a new event for these.

[xunjieli, 2016-04-13 18:15:30]

Patchset #2 (id:40001) has been deleted

[xunjieli, 2016-04-13 18:16:03]

https://codereview.chromium.org/1883883002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/1883883002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:162:
net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, bytes_read,
On 2016/04/13 18:03:50, mmenke wrote:
> TYPE_SOCKET_BYTES_SENT is used for bytes sent at the socket layer.  Should
> probably just add a new event for these.

Done.

[mmenke, 2016-04-14 15:38:20]

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream.h:15: #include "net/log/net_log.h"
Need to include base/memory/ref_counted.h

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:507: // ReadData and it should return
asynchronously because no data is buffered.
The grammar on this sentence needs to be fixed - I'm not sure what it's trying
to say.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:511:
sequenced_data_->RunUntilPaused();
Think the reason for this pause should probably be explained - seemed
unnecessary to me at first.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:513: base::RunLoop().RunUntilIdle();
Can we just run until the delegate's OnReadComplete method is called instead, or
whatever it's called?  Prefer to avoid depending on test fixture magic unless we
need to.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:516: sequenced_data_->Resume();
Is this pause actually needed?

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:517: base::RunLoop().RunUntilIdle();
Why don't you need to manually fire the timer again here?

https://codereview.chromium.org/1883883002/diff/60001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/1883883002/diff/60001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1167: // is queued to be sent.
This is only logged once they're sent

[xunjieli, 2016-04-14 18:57:06]

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream.h:15: #include "net/log/net_log.h"
On 2016/04/14 15:38:19, mmenke wrote:
> Need to include base/memory/ref_counted.h

Done.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:507: // ReadData and it should return
asynchronously because no data is buffered.
On 2016/04/14 15:38:20, mmenke wrote:
> The grammar on this sentence needs to be fixed - I'm not sure what it's trying
> to say.

Done.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:511:
sequenced_data_->RunUntilPaused();
On 2016/04/14 15:38:20, mmenke wrote:
> Think the reason for this pause should probably be explained - seemed
> unnecessary to me at first.

Done.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:513: base::RunLoop().RunUntilIdle();
On 2016/04/14 15:38:20, mmenke wrote:
> Can we just run until the delegate's OnReadComplete method is called instead,
or
> whatever it's called?  Prefer to avoid depending on test fixture magic unless
we
> need to.


Can we handle it in a separate CL? All tests in this file are like this
currently.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:516: sequenced_data_->Resume();
On 2016/04/14 15:38:20, mmenke wrote:
> Is this pause actually needed?

Yes, we need this to be between first and second DATA frame, so we can fire the
timer. Without this pause, the read might contain two frames, and we have no way
to test that synchronous reads will log bytes.

This tests both asynchronous read and synchronous read.

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:517: base::RunLoop().RunUntilIdle();
On 2016/04/14 15:38:20, mmenke wrote:
> Why don't you need to manually fire the timer again here?

The timer is stopped in BidirectionalStreamSpdyImpl::OnClose() which is invoked
when all data is received and the write side is closed.

I added a comment.

[mmenke, 2016-04-14 19:04:13]

LGTM

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1883883002/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:513: base::RunLoop().RunUntilIdle();
On 2016/04/14 18:57:06, xunjieli wrote:
> On 2016/04/14 15:38:20, mmenke wrote:
> > Can we just run until the delegate's OnReadComplete method is called
instead,
> or
> > whatever it's called?  Prefer to avoid depending on test fixture magic
unless
> we
> > need to.
> 
> 
> Can we handle it in a separate CL? All tests in this file are like this
> currently. 

Sure, makes sense.

[xunjieli, 2016-04-14 20:12:28]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-04-14 20:12:45]

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

[commit-bot: I haz the power, 2016-04-14 21:15:39]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-04-14 21:16:58]

Description was changed from

==========
Log sent and received bytes in net::BidirectionalStream

This CL adds logging of sent and received bytes in net::BidirectionalStream if
capturing all bytes.

BUG=602669
==========

to

==========
Log sent and received bytes in net::BidirectionalStream

This CL adds logging of sent and received bytes in net::BidirectionalStream if
capturing all bytes.

BUG=602669

Committed: https://crrev.com/be07785e0727b7b70b55487cca10fcfd2e1796b0
Cr-Commit-Position: refs/heads/master@{#387425}
==========

[commit-bot: I haz the power, 2016-04-14 21:16:59]

Patchset 3 (id:??) landed as
https://crrev.com/be07785e0727b7b70b55487cca10fcfd2e1796b0
Cr-Commit-Position: refs/heads/master@{#387425}