[webkit-reviews] review denied: [Bug 109923] [Curl] The function cookiesForDOM() does not behave correctly. : [Attachment 188545] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied peavo at outlook.com's request for
review:
Bug 109923: [Curl] The function cookiesForDOM() does not behave correctly.
https://bugs.webkit.org/show_bug.cgi?id=109923

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188545&action=review


Nice work putting all of these pieces together.  A few minor formatting nits,
but otherwise it looks great!

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:32
> +static void readCurlCookieToken(const char*& cookie, String &token)

This should be "String& token)"

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:37
> +	   token.append(cookie[0]);

It's too bad WTF String doesn't support a character append with a length (like
it does with UChar).  Then we could use strchr to find the '\t' character, then
append the distance without having to handroll this loop.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:44
> +static void addMatchingCurlCookie(const char* cookie, const String &domain,
const String &path, String &cookies)

These should be written as "String& domain" and "String& path", not "String
&domain", etc.

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:119
> +    cookies = cookies + strName + String("=") + strValue + String("; ");

Do you know if we are paying the cost of creating String temporaries on this
line (e.g., the "=" and "; " values)? Could these be declared as static const
values so we don't reconstruct them each time we pass through here?  Maybe this
function isn't used enough for this to be a concern.