[webkit-reviews] review granted: [Bug 57600] cross-origin XMLHttpRequest doesn't work with redirect : [Attachment 133245] Proposed Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Bill Budge <bbudge at gmail.com>'s
request for review:
Bug 57600: cross-origin XMLHttpRequest doesn't work with redirect
https://bugs.webkit.org/show_bug.cgi?id=57600

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133245&action=review


> Source/WebCore/ChangeLog:4
> +	   DocumentThreadableLoader follows the CORS redirect steps when
asynchronously loading a
> +	   cross origin request with access control.

You might consider adding a link to the spec in the ChangeLog.	Traditionally,
we use the line above the bug URL for the title of the bug and then put a
description like this under the bug URL (after a blank line), but this way
works too.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:185
> +	       allowRedirect =
> +		  
SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol())

WebKit would usually merge these two lines.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:201
> +	       if
(!originalOrigin.get()->isSameSchemeHostPort(requestOrigin.get()))

This line is pretty subtle.  One complication that we need to worry about that
the spec doesn't need to worry about is "universal" origins that can access any
URL...	However, in that case, we won't be in the UseAccessControl codepath, so
I think this is actually correct.

nit: originalOrigin.get()->isSameSchemeHostPort can be written as
originalOrigin->isSameSchemeHostPort because RefPtr overloads operator->