[webkit-reviews] review denied: [Bug 85072] [soup] URL of the ResourceResponse passed to willSendRequest is incorrect : [Attachment 139782] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 85072: [soup] URL of the ResourceResponse passed to willSendRequest is
incorrect
https://bugs.webkit.org/show_bug.cgi?id=85072

Attachment 139782: Patch
https://bugs.webkit.org/attachment.cgi?id=139782&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139782&action=review


I agree with Sergio it would be better to land this while unskipping the tests.
I had a couple of nits, so I'll mark this r- so it gets out of the review
queue, but the code looks good to me.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:218
> +// Called afer receiving all headers for the message.

I don't think this comment adds value, I suggest removing it. However, it may
be puzzling for someone reading the code to understand exactly why we handle
got-headers and update d->m_response while doing it, so...

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:236
> +    ResourceResponse response;
> +    response.updateFromSoupMessage(msg);
> +
> +    d->m_response = response;

... if you could add something to the effect of 'We need to have the original
response to feed to willSendRequest in case we are redirected, so we store it
here.' close to these lines, I think it would help our future readers =)