Bug 27414 - Cairo-based Windows port does not handle cookies properly
Summary: Cairo-based Windows port does not handle cookies properly
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Brent Fulgham
Depends on:
Reported: 2009-07-18 07:00 PDT by Kwang Yul Seo
Modified: 2009-08-07 15:25 PDT (History)
3 users (show)

See Also:

Replace CookieJarWin.cpp with CookieJarCurl.cpp (3.57 KB, patch)
2009-07-18 08:20 PDT, Kwang Yul Seo
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2009-07-18 07:00:14 PDT
Cairo-based windows port uses curl as its http backend. curl handles cookies by itself and it does not share cookies with WinINet. However, CookieJarWin.cpp uses WinINet functions, InternetGetCookie and InternetSetCookie to get and set cookies respectively. 

Using CookieJarCurl.cpp is also wrong because it does not interact with curl's cookie manager. It a simple hash map to get and set cookies. It is inevitable because there is no such API to get and set cookies in curl.

I think the right direction is to use CookieJarCurl.cpp and ask curl developers to provide a public API to manipulate cookies. Then we can implement cookie code correctly.

In the meantime, replacing CookieJarWin.cpp with CookieJarCurl.cpp has no side effect unless our intention is to share cookies with Internet Explorer or other WinINet-based clients.
Comment 1 Kwang Yul Seo 2009-07-18 08:20:52 PDT
Created attachment 33026 [details]
Replace CookieJarWin.cpp with CookieJarCurl.cpp
Comment 2 Brent Fulgham 2009-08-06 20:40:25 PDT
I am not a reviewer, but I think that this patch is correct.  I threw together the existing Windows Cookie logic in an effort to get things building and running, without much concern about correctness or proper utility.

I am in the process of trying to flesh out more of the cURL support, and think this is a good step in that direction.  I support this patch being approved and landed.
Comment 3 Brent Fulgham 2009-08-06 22:54:58 PDT
I can confirm that this patch lands cleanly and runs properly on my WinCairo build.
Comment 4 Kwang Yul Seo 2009-08-07 04:29:52 PDT
I think the maintainer of Windows Cairo port should review this. Who's in chare of?
Comment 5 Eric Seidel (no email) 2009-08-07 12:46:24 PDT
Comment on attachment 33026 [details]
Replace CookieJarWin.cpp with CookieJarCurl.cpp

Assuming this *only* affects the Cairo build and does not break the regular Windows build, this is fine.  I can't tell from this patch if it affects the Apple Win build or not.
Comment 6 Eric Seidel (no email) 2009-08-07 12:47:17 PDT
Comment on attachment 33026 [details]
Replace CookieJarWin.cpp with CookieJarCurl.cpp

Based on the above comments, I'll r+ this.  We can roll it out if it breaks the Apple windows build.
Comment 7 Adam Barth 2009-08-07 15:25:13 PDT
Comment on attachment 33026 [details]
Replace CookieJarWin.cpp with CookieJarCurl.cpp

Clearing review flag on attachment: 33026

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
Committed r46918
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
r46918 = c4b233180d13184eb3bdafeae92b4f8bb02b022c (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
Comment 8 Adam Barth 2009-08-07 15:25:16 PDT
All reviewed patches have been landed.  Closing bug.