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
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.
Comment on attachment 188848 [details] Patch Looks good. Let's get it landed!
Comment on attachment 188848 [details] Patch Clearing flags on attachment: 188848 Committed r143272: <http://trac.webkit.org/changeset/143272>
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!