Bug 109923

Summary: [Curl] The function cookiesForDOM() does not behave correctly.
Product: WebKit Reporter: peavo
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description peavo 2013-02-15 04:04:45 PST
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).
Comment 1 peavo 2013-02-15 05:11:31 PST
Created attachment 188545 [details]
Patch
Comment 2 Brent Fulgham 2013-02-15 20:34:48 PST
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.
Comment 3 peavo 2013-02-18 04:37:56 PST
Created attachment 188848 [details]
Patch
Comment 4 peavo 2013-02-18 04:40:27 PST
Updated patch after review.
Comment 5 Brent Fulgham 2013-02-18 15:21:42 PST
Comment on attachment 188848 [details]
Patch

Looks good.  Let's get it landed!
Comment 6 WebKit Review Bot 2013-02-18 16:02:28 PST
Comment on attachment 188848 [details]
Patch

Clearing flags on attachment: 188848

Committed r143272: <http://trac.webkit.org/changeset/143272>
Comment 7 WebKit Review Bot 2013-02-18 16:02:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 peavo 2013-02-19 00:29:06 PST
(In reply to comment #5)
> (From update of attachment 188848 [details])
> Looks good.  Let's get it landed!

Thanks for the review!