[webkit-reviews] review denied: [Bug 28313] Combine ThreadableLoaderOptions::CrossOriginRequestPolicy and CrossOriginRedirectPolicy : [Attachment 34930] Fix formatting in WebCore/ChangeLog

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Aug 16 19:35:34 PDT 2009


Alexey Proskuryakov <ap at webkit.org> has denied Aaron Boodman
<aa at chromium.org>'s request for review:
Bug 28313: Combine ThreadableLoaderOptions::CrossOriginRequestPolicy and
CrossOriginRedirectPolicy
https://bugs.webkit.org/show_bug.cgi?id=28313

Attachment 34930: Fix formatting in WebCore/ChangeLog
https://bugs.webkit.org/attachment.cgi?id=34930&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
How does this fix relate to bug 27740?

+	 Bug 28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy
and ThreadableLoaderOptions::crossOriginRedirectPolicy
+	 since they are always set the same way.

Please also include bug URL for easy clicking. Personally, I think that "Bug
28313: " prefix becomes unnecessary with that, and do not use it, but in the
past, some people have indicated that they liked it.

+	 for access control, so we should never redirect across origins. But in
two edge cases, we were:
+
+	 * Synchronous XHR: cross-origin request that redirects to same-origin
resource.

Were we? I think this is caught by checks in ResourceHandle, e.g. in
connection:willSendRequest:redirectResponse: in ResourceHandleMac.mm. The first
time this function is called, original url is stored to m_url, and then, the
redirected URL is compared to that, not to document origin.

+	 (WebCore::DocumentThreadableLoader::isLegalRedirect): Ditto.

Maybe "allowed" would be more neutral? "Legal" makes me think that this is a
precisely defined term from some spec (like characters legal in some
production), which it isn't.

Just asking for your opinion.

+    // request and response URLs. This isn't a perfect though, since a URL
could redirect to itself.

Do you mean that a series of redirects could return us to the same host?
"Redirect to itself" seem confusing.

+#include "KURL.h"

No need to include the header, KURL is only used as a reference here.

+	 Also, tightened up behavior of XMLHttpRequest with cross-origin
redirects and access control. We have not implemented redirects
+	 for access control, so we should never redirect across origins. But in
two edge cases, we were:

LayoutTests ChangeLog lacks bug URL, and starts with "also".

+Tests that a cross-origin request that redirects to a same-origin resource
succeeds.

Looks like all subtests actually expect failure, not success.

There are tabs in layout test.

r- since you are not a committer yet, and this needs some fix-up - but all
these comments could be addressed during landing otherwise.


More information about the webkit-reviews mailing list