[webkit-reviews] review granted: [Bug 24462] Move cross-origin access control code out of XMLHttpRequest : [Attachment 28413] Part 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 24462: Move cross-origin access control code out of XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=24462

Attachment 28413: Part 1
https://bugs.webkit.org/attachment.cgi?id=28413&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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