Bug 109923 - [Curl] The function cookiesForDOM() does not behave correctly.
Summary: [Curl] The function cookiesForDOM() does not behave correctly.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-02-15 04:04 PST by peavo
Modified: 2013-02-19 00:29 PST (History)
2 users (show)

See Also:

Patch (6.12 KB, patch)
2013-02-15 05:11 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2013-02-18 04:37 PST, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Brent Fulgham 2013-02-15 20:34:48 PST
Comment on attachment 188545 [details]

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]
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]

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

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!