|Summary:||[Curl] The function cookiesForDOM() does not behave correctly.|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Version:||528+ (Nightly build)|
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 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); 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 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.