Bug 41571 – [Qt] Fix http/tests/loading/redirect-methods.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Robert Hogan]

and document our redirection behaviour.

[Robert Hogan]

Created attachment 60459 [details]
Patch

[Kenneth Rohde Christiansen]

Brady, can you have a look at this?

[Adam Barth]

Comment on attachment 60459 [details]
Patch

I know how all this stuff is supposed to work, but I'm too tired right now to remember.  If you don't get a review soon, feel free to ping me and I'll look in more detail.  In general, what the RFC says about this stuff is bogus.  :(

[Adam Barth]

Comment on attachment 60459 [details]
Patch

Your comment is making me think you don't know this history of this topic.  How to handle these redirects is a somewhat sad tale.  The requirements for these status codes are changing in the next generation of the HTTP spec to more closely align with reality.

I need to look up the proper behavior, but POST shouldn't be special-cased.  I think HEAD (and possibly OPTIONS) are the only ones with special cases.

[Robert Hogan]

(In reply to comment #4)
> (From update of attachment 60459 [details])
> Your comment is making me think you don't know this history of this topic.  How to handle these redirects is a somewhat sad tale.  The requirements for these status codes are changing in the next generation of the HTTP spec to more closely align with reality.
> 
> I need to look up the proper behavior, but POST shouldn't be special-cased.  I think HEAD (and possibly OPTIONS) are the only ones with special cases.

So the code is wrong as well as the comments? I thought http/tests/loading/redirect-methods.html and others were all about handling cases where POST was used and multiple redirects followed.

I'm confused now.

[Robert Hogan]

Created attachment 66668 [details]
Patch

[Robert Hogan]

(In reply to comment #4)
> (From update of attachment 60459 [details])
> Your comment is making me think you don't know this history of this topic.  How to handle these redirects is a somewhat sad tale.  The requirements for these status codes are changing in the next generation of the HTTP spec to more closely align with reality.
> 
> I need to look up the proper behavior, but POST shouldn't be special-cased.  I think HEAD (and possibly OPTIONS) are the only ones with special cases.

I've resubmitted and removed the comments about the RFC. I just describe what we do in order to pass the layout tests.

[Adam Barth]

Comment on attachment 66668 [details]
Patch

I don't think this is quite right.  You shouldn't need m_receivedPermanentRedirect.  What happens is that the first 301 (or whatever) changes the method to GET.  By the time you get to the 307, you shouldn't have a POST method anymore.  307 should always preserve the method of the request that received the 307.  That's the whole point of 307 existing.

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 66668 [details])
> I don't think this is quite right.  You shouldn't need m_receivedPermanentRedirect.  What happens is that the first 301 (or whatever) changes the method to GET.  

Yes.

>By the time you get to the 307, you shouldn't have a POST method anymore.  
We do. We have it in the original request (m_resourceHandle->firstRequest()), which is what QNetworkReplyHandler inspects here. We don't currently keep a record of the most recent method sent in response to a redirect.

>307 should always preserve the method of the request that received the 307.  That's the whole point of 307 existing.

OK. The patch does that though - by reinstating the GET used for the previous 301-303, if that was what was used.

The code in sendResponseIfNeeded() isn't very clear so I'll refactor it.

[Robert Hogan]

Created attachment 68949 [details]
Patch

[Early Warning System Bot]

Attachment 68949 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4057156

[Robert Hogan]

Created attachment 68951 [details]
Patch

[Adam Barth]

Comment on attachment 68951 [details]
Patch

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

Looks great!  Thanks.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:393
> +        if ((statusCode >= 301 && statusCode <= 303) &&  m_resourceHandle->firstRequest().httpMethod() == "POST")

You've got an extra space in "&&  m_resourceHandle"

[Robert Hogan]

Created attachment 69379 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 69379 [details]
Patch

Clearing flags on attachment: 69379

Committed r68906: <http://trac.webkit.org/changeset/68906>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.