Bug 86357 – Web Inspector: Request / response headers should be stored in name-value pairs array, not a map on front-end.

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

[Vsevolod Vlasov]

Storing headers as name-value pairs array information more accurate and allows to treat Set-Cookie headers (which become not parseable when joined by comma) correctly.

[Vsevolod Vlasov]

Created attachment 141695 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 141695 [details]
Patch

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

> Source/WebCore/inspector/front-end/HAREntry.js:75
> +            headers: this._request.requestHeaders.slice(),

I think slice() is redundant here, we do not modify HAR headers anywhere.

> Source/WebCore/inspector/front-end/HAREntry.js:96
> +            headers: this._request.responseHeaders.slice(),

ditto

> Source/WebCore/inspector/front-end/NetworkManager.js:164
> +                if (values[i])

Why this check? What do we do about headers with empty value?

> Source/WebCore/inspector/front-end/NetworkRequest.js:456
> +        this._sortedRequestHeaders.sort(function(a,b) { return a.name.localeCompare(b.name) });

Shouldn't we sort according to a case-insensitive comparator()?

> Source/WebCore/inspector/front-end/NetworkRequest.js:558
> +        this._sortedResponseHeaders.sort(function(a,b) { return a.name.localeCompare(b.name) });

Ditto.

> Source/WebCore/inspector/front-end/NetworkRequest.js:653
> +        for (var i = 0; i < headers.length; ++i) {
> +            if (headers[i].name.toLowerCase() === headerName)

I wonder if it makes sense to binary-search over sorted headers?

> Source/WebCore/inspector/front-end/NetworkRequest.js:657
> +        if (headerName.toLowerCase() === "Set-Cookie".toLowerCase())

/^set-cookie$/i.test()?

[Vsevolod Vlasov]

Created attachment 141708 [details]
Patch

[Vsevolod Vlasov]

(In reply to comment #2)
> (From update of attachment 141695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141695&action=review
> 
> > Source/WebCore/inspector/front-end/HAREntry.js:75
> > +            headers: this._request.requestHeaders.slice(),
> 
> I think slice() is redundant here, we do not modify HAR headers anywhere.
Done.

> > Source/WebCore/inspector/front-end/NetworkManager.js:164
> > +                if (values[i])
> 
> Why this check? What do we do about headers with empty value?
Removed this check.

> > Source/WebCore/inspector/front-end/NetworkRequest.js:456
> > +        this._sortedRequestHeaders.sort(function(a,b) { return a.name.localeCompare(b.name) });
> 
> Shouldn't we sort according to a case-insensitive comparator()?
Done.

> > Source/WebCore/inspector/front-end/NetworkRequest.js:653
> > +        for (var i = 0; i < headers.length; ++i) {
> > +            if (headers[i].name.toLowerCase() === headerName)
> 
> I wonder if it makes sense to binary-search over sorted headers?
I don't think it worth cluttering the code (the number of headers is usually about 10-15 and we will need two loops for multiple results binary search).

> > Source/WebCore/inspector/front-end/NetworkRequest.js:657
> > +        if (headerName.toLowerCase() === "Set-Cookie".toLowerCase())
> 
> /^set-cookie$/i.test()?
headerName is already lower case, so I made it just (headerName === "set-cookie")

[Andrey Kosyakov]

Comment on attachment 141708 [details]
Patch

LGTM

[Vsevolod Vlasov]

Committed r116952: <http://trac.webkit.org/changeset/116952>