Bug 49668 – Web Inspector: provide response code and status text as a part of raw headers data.

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

[Pavel Feldman]

Otherwise 304 resources may appear as 200 and such.

[Pavel Feldman]

Created attachment 74122 [details]
[PATCH] Proposed change.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/inspector/InspectorResourceAgent.cpp', u'WebCore/inspector/front-end/NetworkPanel.js', u'WebCore/inspector/front-end/ResourceManager.js', u'WebCore/platform/network/ResourceRawHeaders.h', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebResourceRawHeaders.h', u'WebKit/chromium/src/WebResourceRawHeaders.cpp']" exit_code: 1
WebCore/platform/network/ResourceRawHeaders.h:35:  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.
Total errors found: 1 in 8 files


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

[Yury Semikhatsky]

Comment on attachment 74122 [details]
[PATCH] Proposed change.

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

> WebKit/chromium/public/WebResourceRawHeaders.h:61
> +    WEBKIT_API int responseCode();

Is there a chance to use response code and text from the response itself? Would be much better if you fixed the embedder code that modifies 304 code to 200 for 304 response codes.

[Pavel Feldman]

(In reply to comment #3)
> (From update of attachment 74122 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74122&action=review
> 
> > WebKit/chromium/public/WebResourceRawHeaders.h:61
> > +    WEBKIT_API int responseCode();
> 
> Is there a chance to use response code and text from the response itself? Would be much better if you fixed the embedder code that modifies 304 code to 200 for 304 response codes.

I chatted with fishd@ and we agreed that it is better to provide raw data separately. I also renamed few classes in order to address his concerns. Patch to follow.

[Pavel Feldman]

Created attachment 74260 [details]
[PATCH] Proposed change.

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

> WebCore/platform/network/ResourceLoadInfo.h:39
> +    int responseCode;
> +    String statusText;

nit: it might be nice to use names for responseCode and statusText
that are more related since statusText is just a human readable
version of responseCode.  how about statusCode instead of responseCode?

or how about responseStatusCode and responseStatusText?  too verbose?

what about the response HTTP version?  sometimes that is interesting if
you are interogating a HTTP response.

what about including a requestMethod field?  do that in a separate patch?

> WebKit/chromium/public/WebHTTPLoadInfo.h:42
> +class WebHTTPHeaderVisitor;

looks like this is not needed.

> WebKit/chromium/public/WebHTTPLoadInfo.h:60
> +    WEBKIT_API int responseCode();

this member function should be const

> WebKit/chromium/public/WebHTTPLoadInfo.h:63
> +    WEBKIT_API WebString statusText();

this member function should be const

> WebKit/chromium/public/WebHTTPLoadInfo.h:66
> +    WEBKIT_API void addRequestHeader(const WebString& name, const WebString& value);

what happens if you call addRequestHeader("foo", "bar") twice?  what
is your expectation about duplicate request/response headers?  do they
get coalesced or do you expect that someone already did that before
calling these methods?

> WebKit/chromium/src/WebHTTPLoadInfo.cpp:96
> +        result.first->second += String("\n") + value;

oh, i see.  that works!  it seems like you should document this on ResourceLoadInfo.

[Eric Seidel (no email)]

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

[Pavel Feldman]

(In reply to comment #7)

Thanks for your review.

> > WebCore/platform/network/ResourceLoadInfo.h:39
> > +    int responseCode;
> > +    String statusText;
> 
> nit: it might be nice to use names for responseCode and statusText
> that are more related since statusText is just a human readable
> version of responseCode.  how about statusCode instead of responseCode?
> 

I fixed it and now use notation from ResourceResponseBase (httpStatusCode/httpStatusText).

> or how about responseStatusCode and responseStatusText?  too verbose?
> 
> what about the response HTTP version?  sometimes that is interesting if
> you are interogating a HTTP response.
> 
> what about including a requestMethod field?  do that in a separate patch?
> 

Next patch (which is not going to require synchronized landing).

> > WebKit/chromium/public/WebHTTPLoadInfo.h:42
> > +class WebHTTPHeaderVisitor;
> 
> looks like this is not needed.
> 

Removed.

> > WebKit/chromium/public/WebHTTPLoadInfo.h:60
> > +    WEBKIT_API int responseCode();
> 
> this member function should be const
> 

Done. Now using notation from WebURLResponse (httpStatusCode/Text) as a name.

> > WebKit/chromium/public/WebHTTPLoadInfo.h:63
> > +    WEBKIT_API WebString statusText();
> 
> this member function should be const
> 

Done.

> > WebKit/chromium/public/WebHTTPLoadInfo.h:66
> > +    WEBKIT_API void addRequestHeader(const WebString& name, const WebString& value);
> 
> what happens if you call addRequestHeader("foo", "bar") twice?  what
> is your expectation about duplicate request/response headers?  do they
> get coalesced or do you expect that someone already did that before
> calling these methods?
> 
> > WebKit/chromium/src/WebHTTPLoadInfo.cpp:96
> > +        result.first->second += String("\n") + value;
> 
> oh, i see.  that works!  it seems like you should document this on ResourceLoadInfo.

It was just a rename, but I see the point it documenting it.

Thanks!

[WebKit Review Bot]

http://trac.webkit.org/changeset/72375 might have broken Chromium Linux Release

[Pavel Feldman]

Landed as r72375.