Bug 61209 – Factor CORS request preparation out of DocumentThreadableLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Factor CORS request preparation out of DocumentThreadableLoader

[Adam Barth]

Created attachment 94262 [details]
Patch

[Alexey Proskuryakov]

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!

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Adam Barth]

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

[Sam Weinig]

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

[Adam Barth]

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.