Log SPDY request headers in URL_REQUEST events in net-internals.

Log SPDY request headers in URL_REQUEST events in net-internals.


BUG=135203
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149482

[pauljensen, 2012-07-31 17:46:13]

Please review this little CL to log SPDY request headers in URL_REQUEST events
in net-internals.

The goal is to make it easier to understand URL_REQUEST events that use the SPDY
protocol, especially when things like redirections occur.  URL_REQUEST events
that use the HTTP protocol already include the request headers in the
URL_REQUEST log.  The idea being that its much easier to read through the
URL_REQUEST than to go digging into the HTTP_STREAM_JOB and then into the
SPDY_SESSION event log to find the request headers.  This addresses
crbug.com/135203

[pauljensen, 2012-07-31 17:51:55]

I should mention that one potential improvement to this CL would be to share
about 7 lines of code between NetLogAddressListCallback and
NetLogSpdySynCallback and my new NetLogSpdySendRequestCallback function.  I'm
not sure this is worth the added complexity.

[mmenke, 2012-07-31 17:54:57]

Driveby comment:  You're also going to want to modify
getParamaterWriterForEventType in
chrome/browser/resources/net_internals/log_view_painter.js to format the headers
like they are for HTTP requests.  You'll either need to use
"writeParamsForRequestHeaders" and modify it to work when there's no "line"
parameter, or make a similar function that handles that case.

Change should be straight forward - just note that
"getParamaterWriterForEventType" is returning a function.

[szym, 2012-07-31 17:59:26]

lgtm

http://codereview.chromium.org/10824115/diff/1/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10824115/diff/1/net/spdy/spdy_http_stream.cc#n...
net/spdy/spdy_http_stream.cc:224: *headers));
nit: no need for line break

[cbentzel, 2012-07-31 18:38:02]

Removing myself. Raman, can you review the SPDY change?

[ramant (doing other things), 2012-07-31 18:59:22]

lgtm

[mmenke, 2012-07-31 19:20:21]

Nevermind - I was wrong.  We have code that displays lists named "headers"
correctly.  LGTM.

[ramant (doing other things), 2012-07-31 19:38:32]

On 2012/07/31 19:20:21, Matt Menke wrote:
> Nevermind - I was wrong.  We have code that displays lists named "headers"
> correctly.  LGTM.

spdy_session.cc's NetLogSpdySynCallback is also logging the SpdyHeaderBlock.

[mmenke, 2012-07-31 19:40:06]

On 2012/07/31 19:38:32, ramant wrote:
> On 2012/07/31 19:20:21, Matt Menke wrote:
> > Nevermind - I was wrong.  We have code that displays lists named "headers"
> > correctly.  LGTM.
> 
> spdy_session.cc's NetLogSpdySynCallback is also logging the SpdyHeaderBlock.

Right, but for HTTP, we have code that formats the ListValue based on net log
event code.  I incorrectly assumed the same was true in all cases.

[eroman, 2012-07-31 19:45:39]

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:33: Value* NetLogSpdySendRequestCallback(const
SpdyHeaderBlock& headers,
IMPORTANT: Define this parameter as a |const SpdyHeaderBlock*|. The issue has to
do with how base::Bind() works -- if it is defined as a const reference,
base::Bind() infrastructure will create a copy of the header block which is
undesirable for our logging purposes.

[ramant (doing other things), 2012-07-31 19:48:56]

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:34: NetLog::LogLevel /* log_level */) {
nit: +1 to eroman's comments. Was wondering why we didn't pass const
SpdyHeaderBlock* headers as it was done in  NetLogSpdySynCallback.

[ramant (doing other things), 2012-07-31 21:22:30]

lgtm

[ramant (doing other things), 2012-07-31 21:23:23]

On 2012/07/31 21:22:30, ramant wrote:
> lgtm

Thanks for changing it to const
SpdyHeaderBlock*.

[eroman, 2012-07-31 21:33:49]

lgtm

[szym, 2012-07-31 22:49:34]

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:33: Value* NetLogSpdySendRequestCallback(const
SpdyHeaderBlock& headers,
On 2012/07/31 19:45:39, eroman wrote:
> IMPORTANT: Define this parameter as a |const SpdyHeaderBlock*|. The issue has
to
> do with how base::Bind() works -- if it is defined as a const reference,
> base::Bind() infrastructure will create a copy of the header block which is
> undesirable for our logging purposes.

I believe that the same can be accomplished with base::ConstRef (defined in
bind_helpers.h). It implies that the argument must outlive the callback, which
is the case here, and might be more readable (style-compliant) than passing a
pointer.

[eroman, 2012-08-01 00:20:29]

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10824115/diff/4002/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:33: Value* NetLogSpdySendRequestCallback(const
SpdyHeaderBlock& headers,
On 2012/07/31 22:49:34, szym wrote:
> On 2012/07/31 19:45:39, eroman wrote:
> > IMPORTANT: Define this parameter as a |const SpdyHeaderBlock*|. The issue
has
> to
> > do with how base::Bind() works -- if it is defined as a const reference,
> > base::Bind() infrastructure will create a copy of the header block which is
> > undesirable for our logging purposes.
> 
> I believe that the same can be accomplished with base::ConstRef (defined in
> bind_helpers.h). It implies that the argument must outlive the callback, which
> is the case here, and might be more readable (style-compliant) than passing a
> pointer.

Thanks for the info szym, I didn't know about that!

[pauljensen, 2012-08-01 14:52:00]

I switched to using base::ConstRef(*headers).

[ramant (doing other things), 2012-08-01 17:47:14]

On 2012/08/01 14:52:00, pauljensen wrote:
> I switched to using base::ConstRef(*headers).

Sorry to be a bother, please don't forget to upload your changes. thanks.

[pauljensen, 2012-08-01 18:38:23]

Sorry, I did upload the changes but then after talking with szym I realized
every other logging call back just used const* so it would be confusing to break
the style in just one place.  So I deleted the change list before lunch.  I'm
just going to go with the const* CL.  Switching everything to ConstRef can be
brought up as a separate issue so it can be done as a whole.

[commit-bot: I haz the power, 2012-08-01 18:39:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/10824115/10001

[commit-bot: I haz the power, 2012-08-01 20:25:33]

Change committed as 149482