[webkit-reviews] review denied: [Bug 50773] CORS origin header not set on GET when content type request header is set : [Attachment 81216] Proposed fix and regression test

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Martin Galpin <martin at 66laps.com>'s
request for review:
Bug 50773: CORS origin header not set on GET when content type request header
is set
https://bugs.webkit.org/show_bug.cgi?id=50773

Attachment 81216: Proposed fix and regression test
https://bugs.webkit.org/attachment.cgi?id=81216&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81216&action=review

This looks good to me. I just have a few minor suggestions to clean up and then
we can get this committed.

> Source/WebCore/ChangeLog:5
> +	   Bug 50773: CORS origin header not set on GET when a preflight
request

Ideally this is formatted more like other ChangeLog entries:

Bug title
Bug link

(on separate lines).

> Source/WebCore/loader/DocumentThreadableLoader.cpp:301
> +    // Explicitly set the origin of this request

Add "." to end of sentence.

> LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get.html:3
> +<p>Test case for issue #50773 - the "Origin" header should be properly sent
with a non-simple cross-origin resource sharing request that uses the GET
method.</p>

No need to mention the bug, so you can omit "Test case for issue #50773 -"

> LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get.html:20
> +    xhr.setRequestHeader("X-Proprietary-Header", "foo"); // make this a
non-simple CORS request

Please make the comment a proper sentence.

"Make this a non-simple CORS request."

>
LayoutTests/http/tests/xmlhttprequest/resources/cross-origin-preflight-get.php:
2
> +// Test case for the preflight cross-origin request using GET (issue #50773)


You don't need to reference the bug number. If someone needs to find it, they
can look at when the test was added and see the commit message, ChangeLog, etc.
which will all reference the bug.