[webkit-reviews] review denied: [Bug 41210] Cross Origin XMLHttpRequest can not expose headers indicated in Access-Control-Expose-Headers HTTP Response Header : [Attachment 122361] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Joe Thomas <joethomas at motorola.com>'s
request for review:
Bug 41210: Cross Origin XMLHttpRequest can not expose headers indicated in
Access-Control-Expose-Headers HTTP Response Header
https://bugs.webkit.org/show_bug.cgi?id=41210

Attachment 122361: Patch
https://bugs.webkit.org/attachment.cgi?id=122361&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122361&action=review


> Source/WebCore/xml/XMLHttpRequest.cpp:157
> +static void addToAccessControlAllowList(const String& string, unsigned
start, unsigned end, HashSet<String, HashType>& set)

We should use a better name for the first argument than "string".  Perhaps
"headerName" ?

> Source/WebCore/xml/XMLHttpRequest.cpp:161
> +    StringImpl* stringImpl = string.impl();
> +    if (!stringImpl)
> +	   return;

This can only happen when the string is the null string.  We should filter that
case out in parseAccessControlAllowList and skip this check here.

> Source/WebCore/xml/XMLHttpRequest.cpp:173
> +    // Skip white space from start.
> +    while (start <= end && isSpaceOrNewline((*stringImpl)[start]))
> +	   ++start;
> +
> +    // only white space
> +    if (start > end) 
> +	   return;
> +
> +    // Skip white space from end.
> +    while (end && isSpaceOrNewline((*stringImpl)[end]))
> +	   --end;

It's probably better to use String::stripWhiteSpace rather than reimplementing
it here.

> Source/WebCore/xml/XMLHttpRequest.cpp:178
> +template<class HashType>

Why is function templated?  It's only ever called with one sort of set.

> Source/WebCore/xml/XMLHttpRequest.cpp:183
> +    while ((end = string.find(',', start)) != notFound) {

Consider using String::split rather than this sort of look.  You can write this
whole function in like three lines of code if you use some of the advanced
features of String (rather than working with c-style strings).