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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=28134

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

------- 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 &&
isSimpleCrossOriginAccessRequest(request.httpMethod(),
request.httpHeaderFields())) {
+	 makeSimpleCrossOriginAccessRequest(request);
+    } else {

No braces around single-line blocks.

+	 if
(CrossOriginPreflightResultCache::shared().canSkipPreflight(document->securityO
rigin()->toString(), request.url(), m_options.allowCredentials,
request.httpMethod(), request.httpHeaderFields())) {
+	     preflightSuccess();
+	 } else

Ditto.

+	 loadRequest(request, skipCanLoadCheck, m_options.sendLoadCallbacks,
m_options.sniffContent);

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
options.

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

+    // 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?