Bug 24462 - Move cross-origin access control code out of XMLHttpRequest
Summary: Move cross-origin access control code out of XMLHttpRequest
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexey Proskuryakov
Depends on:
Reported: 2009-03-09 08:31 PDT by Alexey Proskuryakov
Modified: 2009-08-10 20:30 PDT (History)
2 users (show)

See Also:

Part 1 (42.88 KB, patch)
2009-03-09 08:36 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.