Bug 109923 – [Curl] The function cookiesForDOM() does not behave correctly.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

The Curl implementation of the function cookiesForDOM() has a couple of problems:

It never returns more than one cookie, when it should return a list of all matching cookies.
It only returns cookies set with setCookiesFromDOM() in this session,
when it should return persistent cookies, and cookies set with http (which are not http only).

Created attachment 188545 [details]
Patch

[Brent Fulgham]

Comment on attachment 188545 [details]
Patch

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.

Created attachment 188848 [details]
Patch

Updated patch after review.

[Brent Fulgham]

Comment on attachment 188848 [details]
Patch

Looks good.  Let's get it landed!

[WebKit Review Bot]

Comment on attachment 188848 [details]
Patch

Clearing flags on attachment: 188848

Committed r143272: <http://trac.webkit.org/changeset/143272>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #5)
> (From update of attachment 188848 [details])
> Looks good.  Let's get it landed!

Thanks for the review!