Bug 24462

Summary: Move cross-origin access control code out of XMLHttpRequest
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Alexey Proskuryakov <ap>
Severity: Normal CC: mike, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Part 1 none

Description Alexey Proskuryakov 2009-03-09 08:31:03 PDT
Cross-origin resource sharing is a separate specification that will be used for more than just XHR.
Comment 1 Alexey Proskuryakov 2009-03-09 08:36:41 PDT
Created attachment 28413 [details]
Part 1
Comment 2 Darin Adler 2009-03-09 09:05:16 PDT
Comment on attachment 28413 [details]
Part 1

> +bool accessControlCheck(const ResourceResponse& response, bool includeCredentials, SecurityOrigin* securityOrigin)

I'm not sure this name is clear enough; we normally try to name functions so they describe their return values. Maybe something like passesAccessControlCheck would be better? Or some other name that describes the boolean return value. I also think the "named boolean" idiom might be best for the includeCredentials boolean, assuming that most callers pass a constant.

> +    unsigned expiryDelta = 0;
> +    if (!parseAccessControlMaxAge(response.httpHeaderField("Access-Control-Max-Age"), expiryDelta))
> +        expiryDelta = 5;

Annoying to find this magic number hiding here. Ideally the 5 would be named constant. I know you didn't add this. It also seems strange to initialize expiryDelta to 0 and then set it to 5. Do we need to initialize it at all?

> +bool CrossOriginPreflightResultCache::canSkipPreflight(const String& origin, const KURL& url, bool includeCredentials,
> +                                            const String& method, const HTTPHeaderMap& requestHeaders)

The above is a practical demonstration of why I prefer to indent by a single tab stop instead of lining things up.

> +        template<class HashType>
> +        static void addToAccessControlAllowList(const String& string, unsigned start, unsigned end, HashSet<String, HashType>&);
> +        template<class HashType>
> +        static bool parseAccessControlAllowList(const String& string, HashSet<String, HashType>&);
> +        static bool parseAccessControlMaxAge(const String& string, unsigned& expiryDelta);

I think the template declarations either need to be all one one line, or perhaps with some blank space.

I'm not sure why these functions are class members -- they could be free functions instead.

The argument name "string" could be omitted.

> +        // FIXME: A better solution to holding onto the absolute expiration time might be
> +        // to start a timer for the expiration delta, that removes this from the cache when
> +        // it fires.

This has a comma that's incorrect and should be removed.

r=me, with or without changes
Comment 3 Alexey Proskuryakov 2009-03-10 01:14:14 PDT
Comment on attachment 28413 [details]
Part 1

Committed revision 41548, clearing review flag.

I made the changes, except for adding an enum for includeCredentials argument (because the funciton is always called with a variable for it), and for adding a constant for expiryDelta (my understanding that this code will need to be refactored soon).
Comment 4 Alexey Proskuryakov 2009-08-10 20:30:37 PDT
This work is continued in bug 28134.