Bug 58259 – Web Inspector: Enable raw HTTP headers support

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

Web Inspector should be able to show raw HTTP headers and report true headers size.

[Vsevolod Vlasov]

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.

[Pavel Feldman]

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.

[Andrey Kosyakov]

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?

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

> > 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.

[Vsevolod Vlasov]

Created attachment 89258 [details]
Patch with fixes

[Pavel Feldman]

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?

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

Created attachment 89384 [details]
Patch

[Pavel Feldman]

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).

[Vsevolod Vlasov]

Created attachment 89562 [details]
Patch

All done.

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

Created attachment 89620 [details]
patch with binary

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89620 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 89620 [details]
patch with binary

Clearing flags on attachment: 89620

Committed r83950: <http://trac.webkit.org/changeset/83950>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89620 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
The commit-queue is continuing to process your patch.