Bug 86357 - Web Inspector: Request / response headers should be stored in name-value pairs array, not a map on front-end.
Summary: Web Inspector: Request / response headers should be stored in name-value pair...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-14 04:37 PDT by Vsevolod Vlasov
Modified: 2012-05-14 08:43 PDT (History)
12 users (show)

See Also:


Attachments
Patch (21.45 KB, patch)
2012-05-14 04:53 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (21.41 KB, patch)
2012-05-14 06:12 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-05-14 04:37:40 PDT
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.
Comment 1 Vsevolod Vlasov 2012-05-14 04:53:31 PDT
Created attachment 141695 [details]
Patch
Comment 2 Andrey Kosyakov 2012-05-14 05:15:11 PDT
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()?
Comment 3 Vsevolod Vlasov 2012-05-14 06:12:43 PDT
Created attachment 141708 [details]
Patch
Comment 4 Vsevolod Vlasov 2012-05-14 06:16:27 PDT
(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")
Comment 5 Andrey Kosyakov 2012-05-14 06:56:34 PDT
Comment on attachment 141708 [details]
Patch

LGTM
Comment 6 Vsevolod Vlasov 2012-05-14 08:43:21 PDT
Committed r116952: <http://trac.webkit.org/changeset/116952>