Bug 17917 – Cookie support for HTTP soup backend

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

See attached patch summary for details.

[Xan Lopez]

Created attachment 19866 [details]
0001-Initial-implementation-of-cookies-for-the-http-soup.patch

[Alp Toker]

Comment on attachment 19866 [details]
0001-Initial-implementation-of-cookies-for-the-http-soup.patch

A few coding style issues here (I fixed them up, will attach the fixes).

We shouldn't really land this till the feature hits soup upstream or there's a clear indication that it will go in unchanged.

[Alp Toker]

Created attachment 19909 [details]
Cleaned up patch

[Xan Lopez]

Created attachment 20009 [details]
0001-Initial-implementation-of-cookies-for-the-http-soup.patch

Updated to new APIs after Dan's review, and also updated to build on trunk (needed changes in ResourceHandle.h). Added the getCookieJar hack so people can actually test the patch.

[Xan Lopez]

Created attachment 20010 [details]
0001-Initial-implementation-of-cookies-for-the-http-soup.patch

Updated to new APIs after Dan's review, and also updated to build on trunk
(needed changes in ResourceHandle.h). Added the getCookieJar hack so people can
actually test the patch. (Fix style issue)

[Julien Chaffraix]

I do not know libsoup, so I will just comment on the interaction with the other backend.

CookieJar.h and CookieJarGtk.h are currently shared with the cURL backend and your changes do not take that into account.

For CookieJarGtk, we have the choice between 
adding the proper #if USE guards or split it into two files.
I do not really think we can share code in this case so I would vote for splitting the files and moving them in their own directory.


diff --git a/WebCore/platform/CookieJar.h b/WebCore/platform/CookieJar.h

index 38efd04..3cd86dd 100644

--- a/WebCore/platform/CookieJar.h

+++ b/WebCore/platform/CookieJar.h

@@ -26,6 +26,10 @@

 #ifndef CookieJar_h

 #define CookieJar_h

 

+#if PLATFORM(GTK)


Shoud be USE(SOUP).

+#include <libsoup/soup.h>

+#endif

+

 namespace WebCore {

 

     class KURL;

@@ -35,7 +39,9 @@ namespace WebCore {

     String cookies(const Document* document, const KURL&);

     void setCookies(Document* document, const KURL&, const KURL& policyBaseURL, const String&);

     bool cookiesEnabled(const Document* document);

-

+#if PLATFORM(GTK)


Same as above.

+    SoupCookieJar* getCookieJar(void);

+#endif

 }

 

 #endif

diff --git a/WebCore/platform/gtk/CookieJarGtk.cpp b/WebCore/platform/gtk/CookieJarGtk.cpp

index 2f76ebc..eace692 100644

--- a/WebCore/platform/gtk/CookieJarGtk.cpp

+++ b/WebCore/platform/gtk/CookieJarGtk.cpp

@@ -1,4 +1,7 @@

+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; -*- */

 /*

+ *  Copyright (C) 2008 Xan Lopez <[e-mail]>

+ *

  *  This library is free software; you can redistribute it and/or

  *  modify it under the terms of the GNU Lesser General Public

  *  License as published by the Free Software Foundation; either

@@ -15,31 +18,56 @@

  */

 

 #include "config.h"

+#include "CString.h"

 #include "CookieJar.h"

 

 #include "KURL.h"

 #include "PlatformString.h"

 #include "StringHash.h"

 

-#include <wtf/HashMap.h>

+#include <libsoup/soup.h>

 

 namespace WebCore {

 

-static HashMap<String, String> cookieJar;

+SoupCookieJar* getCookieJar()

+{

+    static SoupCookieJar* jar = NULL;

+

+    if (!jar)

+        jar = soup_cookie_jar_new ();

+

+    return jar;

+}

 

-void setCookies(Document* /*document*/, const KURL& url, const KURL& /*policyURL*/, const String& value)

+void setCookies(Document* document, const KURL& url, const KURL& policyURL, const String& value)


Why do you change this line while you do not use the parameters?

 {

-    cookieJar.set(url.string(), value);

+    SoupCookieJar* jar = getCookieJar();

+    if (!jar) return;


We usually put the if and the return on two different lines.
+

+    SoupURI* origin = soup_uri_new(url.string().utf8().data());

+

+    soup_cookie_jar_set_cookie(jar, origin, value.utf8().data());

+    soup_uri_free(origin);

 }

 

-String cookies(const Document* /*document*/, const KURL& url)

+String cookies(const Document* document, const KURL& url)


Same as above.

 {

-    return cookieJar.get(url.string());

+    SoupCookieJar* jar = getCookieJar();

+    if (!jar) return String();


Same as above.

[Xan Lopez]

(In reply to comment #6)
> I do not know libsoup, so I will just comment on the interaction with the other
> backend.
> 
> CookieJar.h and CookieJarGtk.h are currently shared with the cURL backend and
> your changes do not take that into account.
> 
> For CookieJarGtk, we have the choice between 
> adding the proper #if USE guards or split it into two files.
> I do not really think we can share code in this case so I would vote for
> splitting the files and moving them in their own directory.

As I said it was not meant to be a good solution or anything, just a quick hack to actually test the code. So you think a proper solution would be to add CookieJarSoup or something like that?

[Xan Lopez]

Created attachment 20407 [details]
0001-Initial-implementation-of-cookies-for-the-http-soup.patch

Update the patch to trunk and address review points (still pending jar sharing).

[Julien Chaffraix]

(In reply to comment #7)
> (In reply to comment #6)
> > I do not know libsoup, so I will just comment on the interaction with the other
> > backend.
> > 
> > CookieJar.h and CookieJarGtk.h are currently shared with the cURL backend and
> > your changes do not take that into account.
> > 
> > For CookieJarGtk, we have the choice between 
> > adding the proper #if USE guards or split it into two files.
> > I do not really think we can share code in this case so I would vote for
> > splitting the files and moving them in their own directory.
> 
> As I said it was not meant to be a good solution or anything, just a quick hack
> to actually test the code. So you think a proper solution would be to add
> CookieJarSoup or something like that?
> 

Exactly. As the code is going to be really different, it is better to remove the current CookieJarGtk and create CookieJarCurl and CookieJarSoup (it may also be a good idea to move them to their platform/network directory).

[Dan Winship]

the cookie code is committed to libsoup svn now.

it may make sense to have configure.ac check for "libsoup-2.4 >= 2.23" to see if you have unstable libsoup, and define something in that case (which would be used to enable cookie support, etc), but if not, fall back to checking "libsoup-2.4 >= 2.4". just to make it easier for people to test with. Or maybe not...

[Xan Lopez]

Created attachment 20424 [details]
soup-cookies.patch

Ok, I added CookieJar{Soup,Curl} and removed the old CookieJarGtk.

I've bumped the soup version required to 2.23, I don't think we want people testing against (soon to be) old code.

[Alp Toker]

Landed in r31878.