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.
Created attachment 68093 [details] proposed patch
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.
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.
Created attachment 68096 [details] patch Fixed style.
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.
> 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?
Created attachment 68104 [details] patch - Fixed style again - Switched to OwnPtr in WebURLResponsePrivate
(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.
Attachment 68093 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4054090
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.
Created attachment 69186 [details] patch
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.
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.
Created attachment 69201 [details] patch
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.
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?
Created attachment 69230 [details] patch to land
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.
Committed manually as r68781: http://trac.webkit.org/changeset/68781 Committed build fix as r68783: http://trac.webkit.org/changeset/68783