[webkit-reviews] review denied: [Bug 28134] Move Access Control out of XMLHttpRequest : [Attachment 34432] Proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 10 10:23:57 PDT 2009

Alexey Proskuryakov <ap at webkit.org> has denied Aaron Boodman
<aa at chromium.org>'s request for review:
Bug 28134: Move Access Control out of XMLHttpRequest

Attachment 34432: Proposed patch

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+void DocumentThreadableLoader::loadResourceSynchronously(Document* document,
const ResourceRequest& request, ThreadableLoaderClient& client,
ThreadableLoaderOptions options)
-    <...>
+    RefPtr<DocumentThreadableLoader> loader = adoptRef(new
DocumentThreadableLoader(document, &client, false, request, options));

It's great to re-use async logic here. Some comments though:
- it's worth adding a comment and maybe an assertion (hasOneRef()) to explain
that the loader will actually be deleted once this function returns;
- in create(), we have ASSERT(document); please add it here, as well;
- the new sync/async argument should be an enum, not a boolean.

One more question about sharing code - have you checked how auth dialogs behave
with sync loads?

+    if (!m_options.forceCrossOriginPreflight &&
request.httpHeaderFields())) {
+	 makeSimpleCrossOriginAccessRequest(request);
+    } else {

No braces around single-line blocks.

+	 if
rigin()->toString(), request.url(), m_options.allowCredentials,
request.httpMethod(), request.httpHeaderFields())) {
+	     preflightSuccess();
+	 } else


+	 loadRequest(request, skipCanLoadCheck, m_options.sendLoadCallbacks,

It's highly confusing that loadRequest() has two sets of these parameters, one
in m_options, and another in explicit arguments. Can anything be done to
improve this?

+	 preflightRequest.addHTTPHeaderFields(requestHeaderFields);

As an aside, Mozilla debates that this is based on an incorrect reading of the
spec, and actual request headers shouldn't be sent with OPTIONS request.
Certainly not something to change right now, of course.

+    // FIXME: None of the option parameters are used in the synchronous path.
Currently only XHR uses the synchronous path, and it doesn't need these

I think this means "none are supported". Could you add assertions to enforce

+    // FIXME: This is different than what documents do. Why?
+    if (request.url().isLocalFile())
+      options.sniffContent = true;

I'll defer to Dave to comment on this.


I sense evil from check-webkit-style. Not too bad for this particular patch,
but fixing style in functions that are not otherwise modified is not helpful.

In general, I really like this patch. Eric's comment about moving the code into
a separate class makes sense to me, but that's probably too difficult to do in
one step. Also, it's not much practical difference, as ThreadableLoader is used
in all the same places we need (or potentially need) access control in.

By the way, why didn't you use bug 24462 for this patch?

More information about the webkit-reviews mailing list