Bug 117307 – [curl] Set the http response status text

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Peter Gal]

The ResoureResponse class has a HTTP status text field which is not set with the curl backend.

[Peter Gal]

Created attachment 203945 [details]
proposed patch

Sadly the curl does not provide a way to extract the http status text so a little workaround is needed.

[Peter Gal]

Comment on attachment 203945 [details]
proposed patch

I've just found out this patch will result a failing http test. So removing the flags and trying to fix it.

[Peter Gal]

Created attachment 204022 [details]
fixed patch

[Kenneth Rohde Christiansen]

Comment on attachment 204022 [details]
fixed patch

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

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:321
> +        else if (header.startsWith("HTTP", false)) {

what does false mean? add /* ... */ ?

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:323
> +            // This is (probably) the first line of the response.
> +            // Try to extract the http status text from this.

not very trustworthy comment, can you be more specific?

Attempt to extract HTTP status...

[Peter Gal]

(In reply to comment #4)
> (From update of attachment 204022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204022&action=review
> 
> > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:321
> > +        else if (header.startsWith("HTTP", false)) {
> 
> what does false mean? add /* ... */ ?

The second argument specifies if the string matching should be case sensitive or not. I've never seen that someone adds the comment for this method. But I can do.

> > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:323
> > +            // This is (probably) the first line of the response.
> > +            // Try to extract the http status text from this.
> 
> not very trustworthy comment, can you be more specific?
> 
> Attempt to extract HTTP status...

In case of the curl backend the FOLLOWLOCATION is set to 1, that'll mean that the libcurl will internally follow the redirections. Thus the headerCallback method will be called more than one times with the line starting "HTTP".

[Brent Fulgham]

Comment on attachment 204022 [details]
fixed patch

Peter: Can you please revise the comments on line 322-323 with the additional detail you added in the bug?  Once you do, I'll r+ this.  I don't think you need to add a comment to the 'startsWith' method, as it's used all-over-the-place.

[Peter Gal]

(In reply to comment #6)
> (From update of attachment 204022 [details])
> Peter: Can you please revise the comments on line 322-323 with the additional detail you added in the bug?  Once you do, I'll r+ this.  I don't think you need to add a comment to the 'startsWith' method, as it's used all-over-the-place.

Okay, will do.

[Peter Gal]

Created attachment 204714 [details]
updated patch

[Brent Fulgham]

Comment on attachment 204714 [details]
updated patch

r=me.

[WebKit Commit Bot]

Comment on attachment 204714 [details]
updated patch

Clearing flags on attachment: 204714

Committed r151668: <http://trac.webkit.org/changeset/151668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.