Bug 61209 - Factor CORS request preparation out of DocumentThreadableLoader
: Factor CORS request preparation out of DocumentThreadableLoader
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Adam Barth
:
Depends on:
Blocks: 61015
  Show dependency treegraph
 
Reported: 2011-05-20 12:49 PDT by Adam Barth
Modified: 2011-05-21 19:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.17 KB, patch)
2011-05-20 12:52 PDT, Adam Barth
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-05-20 12:49:38 PDT
Factor CORS request preparation out of DocumentThreadableLoader
Comment 1 Adam Barth 2011-05-20 12:52:00 PDT
Created attachment 94262 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-05-20 13:48:59 PDT
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 3 Alexey Proskuryakov 2011-05-20 13:50:08 PDT
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 4 Adam Barth 2011-05-20 13:53:55 PDT
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.
Comment 5 Adam Barth 2011-05-20 13:58:39 PDT
Committed r86980: <http://trac.webkit.org/changeset/86980>
Comment 6 Sam Weinig 2011-05-21 16:40:39 PDT
> >> 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.
Comment 7 Adam Barth 2011-05-21 19:26:51 PDT
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.