Bug 58259

Summary: Web Inspector: Enable raw HTTP headers support
Product: WebKit Reporter: Vsevolod Vlasov <vsevik>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, commit-queue, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 58127    
Attachments:
Description Flags
Patch
pfeldman: review-
Patch with fixes
pfeldman: review-
Patch
pfeldman: review-
Patch
pfeldman: review+, commit-queue: commit-queue-
patch with binary none

Description Vsevolod Vlasov 2011-04-11 13:01:16 PDT
Web Inspector should be able to show raw HTTP headers and report true headers size.
Comment 1 Vsevolod Vlasov 2011-04-11 13:14:03 PDT
Created attachment 89064 [details]
Patch

Some notes for this patch:
1) Only raw headers information is used if it is received by Inspector, except request/status lines parameters are not extracted from raw headers.
2) Double-click to toggle between raw/parsed headers.
3) Lazy headers size computation.
Comment 2 Pavel Feldman 2011-04-11 22:33:02 PDT
Comment on attachment 89064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89064&action=review

> LayoutTests/inspector/http-headers-parser.html:9
> +InspectorTest.createHeaders = function(lines)

just declare them as "function createHeaders()" in your "function test()"

> Source/WebCore/inspector/Inspector.json:285
> +                    "rawHeaders": { "type": "string", "optional": true, "description": "Raw HTTP response headers." },

I'd suggest renaming these to rawHttpRequest and rawHttpResponse since they contain not only headers. It might require you to rename it in multiple places.

> Source/WebCore/inspector/front-end/Resource.js:387
> +        if (!this._requestHeaders && this._rawRequestHeaders)

So we only use this parser when there are no requestHeaders? But I think we do have them at all times.
Comment 3 Andrey Kosyakov 2011-04-12 06:19:55 PDT
Comment on attachment 89064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89064&action=review

> Source/WebCore/inspector/front-end/HTTPHeadersParser.js:36
> +        if (!this._initialize(requestHeaders))

When are we supposed to hit this check (and the one in _initialize())?

> Source/WebCore/inspector/front-end/HTTPHeadersParser.js:40
> +            return null;

So we return undefined in line 37, null here -- is this the intent?

> Source/WebCore/inspector/front-end/HTTPHeadersParser.js:51
> +            console.log("Failed parsing status line from: " + this._input);

This seems to be the only line different from parseRequesHeaders(). I'd rather not copy 10 lines because of a log message -- let's just have a generic error message instead?

> Source/WebCore/inspector/front-end/Resource.js:409
> +        this._requestHeaders = null;

Why not just delete this for consistency with the rest of the code?

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:327
> +            this._refreshURL();

Shouldn't we also call this._refreshFormData() here?
Comment 4 Vsevolod Vlasov 2011-04-12 13:21:12 PDT
As discussed raw headers are passed to inspector as well as parsed ones,
so there is no need in HTTPHeadersParser anymore.

> > Source/WebCore/inspector/Inspector.json:285
> > +                    "rawHeaders": { "type": "string", "optional": true, "description": "Raw HTTP response headers." },
> 
> I'd suggest renaming these to rawHttpRequest and rawHttpResponse since they contain not only headers. It might require you to rename it in multiple places.

As discussed current naming is fine and less confusing than rawHttpRequest and rawHttpResponse.
Comment 5 Vsevolod Vlasov 2011-04-12 13:24:40 PDT
> > Source/WebCore/inspector/front-end/Resource.js:409
> > +        this._requestHeaders = null;
> 
> Why not just delete this for consistency with the rest of the code?
Done.
 
> > Source/WebCore/inspector/front-end/ResourceHeadersView.js:327
> > +            this._refreshURL();
> 
> Shouldn't we also call this._refreshFormData() here?
This piece of code is removed now, but I checked that we call this._refreshFormData() in each scenario.
Comment 6 Vsevolod Vlasov 2011-04-12 13:26:09 PDT
Created attachment 89258 [details]
Patch with fixes
Comment 7 Pavel Feldman 2011-04-13 04:28:10 PDT
Comment on attachment 89258 [details]
Patch with fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=89258&action=review

> Source/WebCore/inspector/Inspector.json:344
> +                    "timing": { "$ref": "ResourceTiming", "optional": true, "description": "Timing information for the given request." },

you will need to merge this.

> Source/WebCore/inspector/Inspector.json:345
> +                    "rawHeaders": { "type": "string", "optional": true, "description": "Raw HTTP response headers." },

rawHeadersText ?
rawRequestHeadersText ?

> Source/WebCore/inspector/front-end/Resource.js:425
> +        if (typeof(this._requestHeadersSize) === 'undefined') {

Please use double quotes.

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:219
> +            this._refreshHeaders(WebInspector.UIString("Request Headers"), this._resource.sortedRequestHeaders, additionalRow, this._requestHeadersTreeElement, rawHeadersToggleFunction);

Could you pass boolean flag instead of passing the function?

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:282
> +    _refreshHeaders: function(title, headers, additionalRow, headersTreeElement, rawHeadersToggleFunction)

usually we don't pass functions other than callbacks. What is it needed for?

> Source/WebKit/chromium/src/WebHTTPLoadInfo.cpp:120
> +    m_private->rawRequestHeaders = rawHeaders;

Headers above are not less raw, should you rename this to setRawRequestHeadersText?
Comment 8 Vsevolod Vlasov 2011-04-13 09:00:30 PDT
Comment on attachment 89258 [details]
Patch with fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=89258&action=review

>> Source/WebCore/inspector/Inspector.json:344
>> +                    "timing": { "$ref": "ResourceTiming", "optional": true, "description": "Timing information for the given request." },
> 
> you will need to merge this.

Merged.

>> Source/WebCore/inspector/Inspector.json:345
>> +                    "rawHeaders": { "type": "string", "optional": true, "description": "Raw HTTP response headers." },
> 
> rawHeadersText ?
> rawRequestHeadersText ?

Done.

>> Source/WebCore/inspector/front-end/Resource.js:425
>> +        if (typeof(this._requestHeadersSize) === 'undefined') {
> 
> Please use double quotes.

Done.

>> Source/WebCore/inspector/front-end/ResourceHeadersView.js:219

> 
> Could you pass boolean flag instead of passing the function?

Removed this logic from _refreshHeaders/_refreshRawHeaders functions.

>> Source/WebKit/chromium/src/WebHTTPLoadInfo.cpp:120
>> +    m_private->rawRequestHeaders = rawHeaders;
> 
> Headers above are not less raw, should you rename this to setRawRequestHeadersText?

Added text everywhere.
Comment 9 Vsevolod Vlasov 2011-04-13 09:02:43 PDT
Created attachment 89384 [details]
Patch
Comment 10 Pavel Feldman 2011-04-14 03:53:03 PDT
Comment on attachment 89384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89384&action=review

Couple of nits, otherwise looks good.

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:226
> +

nit: remove extra space

> Source/WebCore/inspector/front-end/ResourceHeadersView.js:324
> +        var title = "<span class=\"header-value source-code\">" + String(rawHeadersText).trim().escapeHTML() + "</span>"

You should fill this is programmatically (once item is appended, you can reference its listItemElement and fill it in).
Comment 11 Vsevolod Vlasov 2011-04-14 05:35:56 PDT
Created attachment 89562 [details]
Patch

All done.
Comment 12 WebKit Commit Bot 2011-04-14 08:47:52 PDT
Comment on attachment 89562 [details]
Patch

Rejecting attachment 89562 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 1

Last 500 characters of output:
ls/Scripts/webkitpy/common/system/executive.py", line 372, in run_command
    output = process.communicate(string_to_communicate)[0]
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 671, in communicate
    return self._communicate(input)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 1177, in _communicate
    bytes_written = os.write(self.stdin.fileno(), chunk)
OSError: [Errno 32] Broken pipe

Full output: http://queues.webkit.org/results/8399663
Comment 13 Vsevolod Vlasov 2011-04-14 12:04:12 PDT
Created attachment 89620 [details]
patch with binary
Comment 14 WebKit Commit Bot 2011-04-14 22:52:58 PDT
The commit-queue encountered the following flaky tests while processing attachment 89620 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2011-04-14 22:55:52 PDT
Comment on attachment 89620 [details]
patch with binary

Clearing flags on attachment: 89620

Committed r83950: <http://trac.webkit.org/changeset/83950>
Comment 16 WebKit Commit Bot 2011-04-14 22:55:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Commit Bot 2011-04-15 00:08:46 PDT
The commit-queue encountered the following flaky tests while processing attachment 89620 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.