[webkit-reviews] review denied: [Bug 132523] [WK2] NetworkProcess doesn't set response URL on error : [Attachment 230751] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 132523: [WK2] NetworkProcess doesn't set response URL on error
https://bugs.webkit.org/show_bug.cgi?id=132523

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230751&action=review


r- because the ambiguity in the code change concerns me

> Source/WebKit2/ChangeLog:4
> +	   [WK2] NetworkProcess doesn't set response URL on error
> +	   https://bugs.webkit.org/show_bug.cgi?id=132523

Would like a more accurate  ChangeLog message here, as this is specifically
about synchronous XHRs

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:105
> +    if (m_response.isNull() && m_response.url().isNull())
> +	   m_response.setURL(m_originalRequest.url());

The bug title suggests that response urls are never set for these loads, yet
this code suggests that they are sometimes set...?

In which cases is it already set?

> LayoutTests/ChangeLog:8
> +	   Un-marking test as fail

I find it kind of hard to parse this.

"Re-enable test that now passes"?