Add SPDY request headers to DevTools.

Add SPDY request headers to DevTools.

R=eroman@chromium.org, yurys@chromium.org
BUG=73945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155413

[mmenke, 2012-08-24 20:28:20]

Wanted to know what you thought of this approach before I add unit tests
(Probably in 3 places - net-internals, DevTools, and net/).

Having a dictionary for one type of headers and lists for the rest makes the
Javascript a bit funky, but otherwise, the code to create the
SpdyHeaderBlockThingy from the headers would be a bit funky instead.

It's also a little weird that the read event is getting the http response
headers rather than the raw spdy headers.  May want to change that in a future
CL.

[mmenke, 2012-08-30 20:42:59]

On 2012/08/24 20:28:20, Matt Menke wrote:
> Wanted to know what you thought of this approach before I add unit tests
> (Probably in 3 places - net-internals, DevTools, and net/).
> 
> Having a dictionary for one type of headers and lists for the rest makes the
> Javascript a bit funky, but otherwise, the code to create the
> SpdyHeaderBlockThingy from the headers would be a bit funky instead.
> 
> It's also a little weird that the read event is getting the http response
> headers rather than the raw spdy headers.  May want to change that in a future
> CL.

Eric:  Ping.

[eroman, 2012-08-31 01:30:35]

Looking! for some reason I thought it was being done by a different reviewer

[eroman, 2012-08-31 02:27:17]

lgtm

http://codereview.chromium.org/10870080/diff/12001/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10870080/diff/12001/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:126: case
net::NetLog::TYPE_HTTP_TRANSACTION_SPDY_SEND_REQUEST_HEADERS: {
Sadness. I dream of a day where these values are pulled out more directly. I
guess not as bad for devtools observer since at least that is active logging.

[mmenke, 2012-08-31 02:31:10]

I'll go ahead and add unit tests, just wanted to make sure you didn't have any
better ideas before doing so.

http://codereview.chromium.org/10870080/diff/12001/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10870080/diff/12001/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:126: case
net::NetLog::TYPE_HTTP_TRANSACTION_SPDY_SEND_REQUEST_HEADERS: {
On 2012/08/31 02:27:17, eroman wrote:
> Sadness. I dream of a day where these values are pulled out more directly. I
> guess not as bad for devtools observer since at least that is active logging.

Yea, I agree with you there.  I'd love to be able to have NetLog disabled most
of the time, with neither DevTools nor LogTiming using it.

[mmenke, 2012-09-05 20:58:54]

[+pfeldman]:  Please review content\browser\debugger.

[eroman]:  Could you take another look?  I fixed a bug or two (Code was trying
to convert arrays to arrays, since arrays are also objects, and wasn't handling
the case filtering was disabled) and added unit tests.

Thanks.

[eroman, 2012-09-05 21:44:32]

lgtm

http://codereview.chromium.org/10870080/diff/26025/chrome/browser/resources/n...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

http://codereview.chromium.org/10870080/diff/26025/chrome/browser/resources/n...
chrome/browser/resources/net_internals/log_view_painter.js:241: // If headers an
in an object, convert them to an array, for better display.
typo: "headers an in"

http://codereview.chromium.org/10870080/diff/26025/net/spdy/spdy_header_block.h
File net/spdy/spdy_header_block.h (right):

http://codereview.chromium.org/10870080/diff/26025/net/spdy/spdy_header_block...
net/spdy/spdy_header_block.h:26: // Converts NetLog event parameters into a SPDY
header block and write them
"write" --> "writes"

[mmenke, 2012-09-06 17:26:33]

http://codereview.chromium.org/10870080/diff/26025/chrome/browser/resources/n...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

http://codereview.chromium.org/10870080/diff/26025/chrome/browser/resources/n...
chrome/browser/resources/net_internals/log_view_painter.js:241: // If headers an
in an object, convert them to an array, for better display.
On 2012/09/05 21:44:32, eroman wrote:
> typo: "headers an in"

Done.

http://codereview.chromium.org/10870080/diff/26025/net/spdy/spdy_header_block.h
File net/spdy/spdy_header_block.h (right):

http://codereview.chromium.org/10870080/diff/26025/net/spdy/spdy_header_block...
net/spdy/spdy_header_block.h:26: // Converts NetLog event parameters into a SPDY
header block and write them
On 2012/09/05 21:44:32, eroman wrote:
> "write" --> "writes"

Done.

[mmenke, 2012-09-06 17:30:41]

Looks like pfeldman's on vacation for the next 3 weeks.

[+yurys]  Could you review the changes to content\browser\debugger?  Thanks!

[vsevik, 2012-09-06 17:32:45]

http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:143:
info->request_headers_text = "";
Can we treat request_headers_text better?

http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:143:
info->request_headers_text = "";
Can we treat request_headers_text better?

[mmenke, 2012-09-06 17:45:31]

http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:143:
info->request_headers_text = "";
On 2012/09/06 17:32:45, vsevik wrote:
> Can we treat request_headers_text better?

Note that SPDY doesn't have any request line - instead it breaks that
information into special headers (Which appear in request_headers).  There's no
code that strongly expects it to start with a request line?  I've gone ahead and
copied the behavior of HttpRequestHeaders::ToString.

[yurys, 2012-09-07 07:35:36]

On 2012/09/06 17:45:31, Matt Menke wrote:
>
http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
> File content/browser/debugger/devtools_netlog_observer.cc (right):
> 
>
http://codereview.chromium.org/10870080/diff/13040/content/browser/debugger/d...
> content/browser/debugger/devtools_netlog_observer.cc:143:
> info->request_headers_text = "";
> On 2012/09/06 17:32:45, vsevik wrote:
> > Can we treat request_headers_text better?
> 
> Note that SPDY doesn't have any request line - instead it breaks that
> information into special headers (Which appear in request_headers).  There's
no
> code that strongly expects it to start with a request line?  I've gone ahead
and
> copied the behavior of HttpRequestHeaders::ToString.

request_headers_text is expected to contain raw text of the request
header(everything before the body). If we cannot extract the raw text of the
request then I believe we should leave the field empty as the same formatting
may well be done on the front-end and that would reduce the amount of data sent
to the front-end.

[vsevik, 2012-09-07 08:01:56]

I see now that request_headers_text does not make much sense even in http
request case. So I think we should land the original patch (#6)

[vsevik, 2012-09-07 08:02:35]

content/browser/debugger/devtools_netlog_observer.cc in Patch #6 looks good to
me

[yurys, 2012-09-07 08:04:28]

On 2012/09/07 08:02:35, vsevik wrote:
> content/browser/debugger/devtools_netlog_observer.cc in Patch #6 looks good to
> me

content/browser/debugger/devtools_netlog_observer.cc from path #6 LGTM

[mmenke, 2012-09-07 13:22:33]

On 2012/09/07 08:04:28, Yury Semikhatsky wrote:
> On 2012/09/07 08:02:35, vsevik wrote:
> > content/browser/debugger/devtools_netlog_observer.cc in Patch #6 looks good
to
> > me
> 
> content/browser/debugger/devtools_netlog_observer.cc from path #6 LGTM

Great, thank you both.  I'll go ahead and land patch set #6, then.

[commit-bot: I haz the power, 2012-09-07 14:35:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10870080/22021

[commit-bot: I haz the power, 2012-09-07 16:46:26]

Try job failure for 10870080-22021 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...