Bug 61209

Created attachment 94262 [details] Patch

Comment on attachment 94262 [details] Patch 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!

Comment on attachment 94262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94262&action=review > Source/WebCore/loader/CrossOriginAccessControl.h:45 > +ResourceRequest createAccessControlPreflightRequest(const ResourceRequest&, SecurityOrigin*, bool allowCredentials); Would be nice to avoid copying the value.

Comment on attachment 94262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94262&action=review >> Source/WebCore/loader/CrossOriginAccessControl.h:30 >> +#include "ResourceRequest.h" > > Can't this be a forward declaration? Not with the copy below. >> Source/WebCore/loader/CrossOriginAccessControl.h:45 >> +ResourceRequest createAccessControlPreflightRequest(const ResourceRequest&, SecurityOrigin*, bool allowCredentials); > > Would be nice to avoid copying the value. Yeah, the one caller wanted to make a copy anyway. I could make it not a copy by using the heap, but that's probably a net loss. >> Source/WebCore/loader/DocumentThreadableLoader.cpp:112 >> + // 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! Ok. I just couldn't read the comment so I re-flowed it using gold-q. I'll revert this part of the change.

Committed r86980: <http://trac.webkit.org/changeset/86980>

> >> Source/WebCore/loader/CrossOriginAccessControl.h:30 > >> +#include "ResourceRequest.h" > > > > Can't this be a forward declaration? > > Not with the copy below. What copy are you referring to? It looks like it could be forward declared to me.

ResourceRequest createAccessControlPreflightRequest(const ResourceRequest&, SecurityOrigin*, bool allowCredentials); ^^^ The return is by copying. I could be wrong though. I'll try forward declaring it next time I have a build.