Bug 61209 - Factor CORS request preparation out of DocumentThreadableLoader
: Factor CORS request preparation out of DocumentThreadableLoader
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 61015
  Show dependency treegraph
 
Reported: 2011-05-20 12:49 PST by
Modified: 2011-05-21 19:26 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-20 12:49:38 PST
Factor CORS request preparation out of DocumentThreadableLoader
------- Comment #1 From 2011-05-20 12:52:00 PST -------
Created an attachment (id=94262) [details]
Work in progress
------- Comment #2 From 2011-05-20 13:48:59 PST -------
(From update of attachment 94262 [details])
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 From 2011-05-20 13:50:08 PST -------
(From update of attachment 94262 [details])
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 From 2011-05-20 13:53:55 PST -------
(From update of attachment 94262 [details])
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 From 2011-05-20 13:58:39 PST -------
Committed r86980: <http://trac.webkit.org/changeset/86980>
------- Comment #6 From 2011-05-21 16:40:39 PST -------
> >> 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 From 2011-05-21 19:26:51 PST -------
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.