Add logging to the QUIC write path.

Add logging to the QUIC write path.

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

[Ryan Hamilton, 2013-06-20 21:39:41]

eroman: can you look at the net-internals changes in both the event type and the
quic_connection_logger

ramant: everything :>

[ramant (doing other things), 2013-06-20 21:51:13]

lgtm

[eroman, 2013-06-20 22:20:04]

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1287: //     "size": <The size of the packet
in bytes>
Add a closing brace.

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1366: //     "error_code": <QuicErrorCode in
the frame>,
What is this value? If it is a net error code you should call it net_error for
pretty printing. Similarly if it is a quick error you need to call it quic_error

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1422: //     "error_code": <QuicErrorCode in
the frame>,
same question

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

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:1146: rv == -1 ? *error : rv);
What is the range of |*error|?

Is it a NetError code? Is it a QuicError code? Is it something else?

This is important, since the code is assuming that *error can only be negative,
and is not -1. (Otherwise there is ambiguity with "rv" which is non-negative.

Please add some explanation in the comments at least, or choose a more
informative variable name.

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection_logg...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection_logg...
net/quic/quic_connection_logger.cc:33: DictionaryValue* dict = new
DictionaryValue();
Drop the "t". It is cleaner.
Nah, just kidding :)

[Ryan Hamilton, 2013-06-20 22:44:16]

thanks, eric

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1287: //     "size": <The size of the packet
in bytes>
On 2013/06/20 22:20:04, eroman wrote:
> Add a closing brace.

Done.

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1366: //     "error_code": <QuicErrorCode in
the frame>,
On 2013/06/20 22:20:04, eroman wrote:
> What is this value? If it is a net error code you should call it net_error for
> pretty printing. Similarly if it is a quick error you need to call it
quic_error

Whoops.  Looks like I should have fixed this in the pretty-print CL.  Done.  (It
should have been called quic_rst_stream_error, and the previous as well)

https://codereview.chromium.org/17518002/diff/1/net/base/net_log_event_type_l...
net/base/net_log_event_type_list.h:1422: //     "error_code": <QuicErrorCode in
the frame>,
On 2013/06/20 22:20:04, eroman wrote:
> same question

Also should have been fixed in earlier CL :<  Should be quic_error.  Fixed. 
Thanks!

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

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:1146: rv == -1 ? *error : rv);
On 2013/06/20 22:20:04, eroman wrote:
> What is the range of |*error|?

-1 == error, otherwise positive values equal the number of bytes written.

> Is it a NetError code? Is it a QuicError code? Is it something else?

Something else.  |error| will be set with a network error code in the event of
an error.
 
> This is important, since the code is assuming that *error can only be
negative,
> and is not -1. (Otherwise there is ambiguity with "rv" which is non-negative.

> Please add some explanation in the comments at least, or choose a more
> informative variable name.

Change it to bytes_written and added a comment.

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection_logg...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/17518002/diff/1/net/quic/quic_connection_logg...
net/quic/quic_connection_logger.cc:33: DictionaryValue* dict = new
DictionaryValue();
On 2013/06/20 22:20:04, eroman wrote:
> Drop the "t". It is cleaner.
> Nah, just kidding :)

:>

[eroman, 2013-06-20 23:06:25]

lgtm

[commit-bot: I haz the power, 2013-06-21 15:29:10]

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

[commit-bot: I haz the power, 2013-06-21 15:44:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-22 20:47:10]

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

[commit-bot: I haz the power, 2013-06-22 22:34:15]

Change committed as 208098