Bug 46092 – Web Inspector: display headers actually used by network stack in Resources tab

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

[Andrey Kosyakov]

Currently, the request and response headers Web Inspector displays in resource tab do not match those actually send over wire:
- for request headers, headers added by the platform network stack are missing. This includes Cookie:, Accept-*: and Cache-Control: headers;
- for response, this includes Set-Cookie header.
It is suggested to provide a way for network stack to return raw headers as part of ResouceResponse, conditionally on a flag set in request.

[Andrey Kosyakov]

Created attachment 68093 [details]
proposed patch

[WebKit Review Bot]

Attachment 68093 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/WebResourceRawHeaders.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebResourceRawHeaders.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebResourceRawHeaders.h:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebResourceRawHeaders.h:80:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 68093 [details]
proposed patch

If this is Chromium only, please file a bug about it needed to be implemeneted for other ports. Or find a way to make it work for everyone.

[Andrey Kosyakov]

Created attachment 68096 [details]
patch

Fixed style.

[WebKit Review Bot]

Attachment 68096 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/WebResourceRawHeaders.h:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

nit: please delete this line.

> WebKit/chromium/src/WebResourceRawHeaders.cpp:49
> +    delete m_requestHeaders;
> +    delete m_responseHeaders;

It'd be better to use OwnPtr for these members.

> WebKit/chromium/src/WebResourceRawHeaders.cpp:76
> +    pair<HTTPHeaderMap::iterator, bool> result =
> +        m_private->add(name, value);

Line wrapping doesn't necessary here.

> WebKit/chromium/src/WebURLResponsePrivate.h:60
> +        delete m_resourceRawHeaders;

Can we use OwnPtr here too?

[Andrey Kosyakov]

Created attachment 68104 [details]
patch

- Fixed style again
- Switched to OwnPtr in WebURLResponsePrivate

[Andrey Kosyakov]

(In reply to comment #6)
> > WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> nit: please delete this line.

Done.

> > WebKit/chromium/src/WebResourceRawHeaders.cpp:49
> > +    delete m_requestHeaders;
> > +    delete m_responseHeaders;
> 
> It'd be better to use OwnPtr for these members.

Can't do it here -- the WebResourceRawHeaders belongs to WebKit public interface, we can't expose internal WebCore types there.
> 
> > WebKit/chromium/src/WebResourceRawHeaders.cpp:76
> > +    pair<HTTPHeaderMap::iterator, bool> result =
> > +        m_private->add(name, value);
> 
> Line wrapping doesn't necessary here.

Fixed.

> 
> > WebKit/chromium/src/WebURLResponsePrivate.h:60
> > +        delete m_resourceRawHeaders;
> 
> Can we use OwnPtr here too?

Fixed.

[WebKit Review Bot]

Attachment 68093 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4054090

[Pavel Feldman]

Comment on attachment 68104 [details]
patch

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

A bunch of nits, otherwise looks good.

> WebCore/inspector/InspectorResource.cpp:180
> +        m_responseHeaderFields = headers->responseHeaders;

Either mark request as changed here or modify WebInspector.updateResource to re-read request headers from the payload upon response change.

> WebCore/platform/network/ResourceResponseBase.h:117
> +    PassRefPtr<ResourceRawHeaders> resourceRawHeaders() const;

There is a number of header-related methods (get / set individual headers) that are not going to affect raw headers. So we add a bit of confusion here. Maybe we should push raw headers straight into the inspector controller. Did you track usages of these methods?

> WebKit/chromium/public/WebResourceRawHeaders.h:47
> +class WebResourceRawHeaders {

1. We should do WebResourceHeadersMap I guess and have separate setters in the WebURLRequest
2. Interestingly, we are using headers visitors in the rest of the places. We should talk to the authors of that code to find out why. Are we hesitant copying those for the performance reasons?

> WebKit/chromium/public/WebResourceRawHeaders.h:58
> +        WebCore::HTTPHeaderMap* m_private;

You should use WebPrivatePtr when wrapping WebCore. See the pattern in WebURLLoadTiming.

> WebKit/chromium/public/WebURLResponse.h:86
> +    WEBKIT_API WebResourceRawHeaders* resourceRawHeaders();

Must have a setter here.

> WebKit/chromium/src/WebDevToolsAgentImpl.cpp:328
> +        if (ic->hasFrontend() && request.reportLoadTiming())

I think ic->willSendRequest will do this for you.

> WebKit/chromium/src/WebURLResponsePrivate.h:49
> +    OwnPtr<WebResourceRawHeaders> m_resourceRawHeaders;

Why hiding public WebKit API field here? In fact, we don't need it at all. You should serve both getter and a setter against corresponding ResourceResponse's fields.

[Andrey Kosyakov]

Created attachment 69186 [details]
patch

[WebKit Review Bot]

Attachment 69186 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/WebResourceRawHeaders.h:52:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebResourceRawHeaders.cpp:83:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebResourceRawHeaders.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebResourceRawHeaders.cpp:42:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 11 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 69186 [details]
patch

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

> WebKit/chromium/public/WebResourceRawHeaders.h:37
> +namespace WebCore {

#if WEBKIT_IMPLEMENTATION

> WebKit/chromium/public/WebResourceRawHeaders.h:62
> +        WebCore::HTTPHeaderMap* m_private;

Please use generic WebPrivatePtr instead.

> WebKit/chromium/public/WebURLResponse.h:87
> +    WEBKIT_API WebResourceRawHeaders* resourceRawHeaders();

setter, please

> WebKit/chromium/public/WebURLResponse.h:176
> +    WebPrivateOwnPtr<WebResourceRawHeaders> m_resourceRawHeaders;

WebPrivateOwnPtr is used to map to WebCore, you are simply pointing to public WebKit api class here.

[Andrey Kosyakov]

Created attachment 69201 [details]
patch

[WebKit Review Bot]

Attachment 69201 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebResourceRawHeaders.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 69201 [details]
patch

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

> WebCore/WebCore.vcproj/WebCore.vcproj:27431
> +					RelativePath="..\platform\network\ResourceRawHeaders.h"

You should add new files to Xcode project.

> WebKit/chromium/public/WebResourceRawHeaders.h:38
> +class ResourceResponse;

mind import order.

> WebKit/chromium/public/WebResourceRawHeaders.h:50
> +    class HeadersMap {

Having addRequestHeader and addResponseHeader would look much more elegant.

> WebKit/chromium/public/WebResourceRawHeaders.h:62
> +    private:

remove this line.

> WebKit/chromium/src/WebResourceRawHeaders.cpp:79
> +    m_private->responseHeaders = *(const WebCore::HTTPHeaderMap*)value;

Can you do static cast here?

[Andrey Kosyakov]

Created attachment 69230 [details]
patch to land

[Eric Seidel (no email)]

Comment on attachment 69201 [details]
patch

Cleared Pavel Feldman's review+ from obsolete attachment 69201 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Andrey Kosyakov]

Committed manually as r68781: http://trac.webkit.org/changeset/68781
Committed build fix as r68783: http://trac.webkit.org/changeset/68783