Bug 24462 – Move cross-origin access control code out of XMLHttpRequest

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Cross-origin resource sharing is a separate specification that will be used for more than just XHR.

[Alexey Proskuryakov]

Created attachment 28413 [details]
Part 1

[Darin Adler]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

This work is continued in bug 28134.