[webkit-reviews] review granted: [Bug 61209] Factor CORS request preparation out of DocumentThreadableLoader : [Attachment 94262] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 61209: Factor CORS request preparation out of DocumentThreadableLoader
https://bugs.webkit.org/show_bug.cgi?id=61209

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94262&action=review

> Source/WebCore/loader/CrossOriginAccessControl.cpp:99
> +void updateRequestForAccessControl(ResourceRequest& request, SecurityOrigin*
securityOrigin, bool allowCredentials)

I wish we had a more descriptive name for this function.

> Source/WebCore/loader/CrossOriginAccessControl.h:30
> +#include "ResourceRequest.h"

Can't this be a forward declaration?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:112
> +    // Cross-origin requests are only defined for HTTP. We would catch this
> +    // when checking response headers later, but there is no reason to send
a
> +    // request that's guaranteed to be denied.

I don't think that there is a reason to make lines this short - we don't have
any kind of 80-character limit, and value vertical space. There is a much
longer code line below!