After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 522125 - add support for cookies
add support for cookies
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
2.4.x
Other Linux
: Normal normal
: GNOME 2.24
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2008-03-12 23:29 UTC by Dan Winship
Modified: 2008-11-04 21:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[1/4] Use updated domain to lookup cookies. (629 bytes, patch)
2008-03-15 23:43 UTC, Xan Lopez
committed Details | Review
[2/4] Use g_slist_concat to concatenate files, no g_slist_append (549 bytes, patch)
2008-03-15 23:43 UTC, Xan Lopez
committed Details | Review
[3/4] Do not free memory when we want to return it. (681 bytes, patch)
2008-03-15 23:43 UTC, Xan Lopez
committed Details | Review
[4/4] Avoid inifinite loopes while adding cookies to headers. (882 bytes, patch)
2008-03-15 23:43 UTC, Xan Lopez
committed Details | Review
[PATCH] Add cookie-jar-text files to Makefile.am and soup.h (972 bytes, patch)
2008-10-27 21:09 UTC, Xan Lopez
committed Details | Review
[PATCH] Export set_cookie as soup_cookie_jar_set_cookie_internal. (2.90 KB, patch)
2008-10-27 21:09 UTC, Xan Lopez
committed Details | Review
[PATCH] Add soup_cookie_jar_all_cookies. (1.55 KB, patch)
2008-10-27 21:09 UTC, Xan Lopez
committed Details | Review
[PATCH] Protect against NULL dates. (364 bytes, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] Use G_{BEGIN,END}_DECLS in the header. (825 bytes, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] Create boxed type for SoupCookie. (5.60 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] Add 'changed' signal. (3.21 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie: free expire date when freeing cookie. (728 bytes, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: rename set_internal to add. (2.65 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie: add soup_cookie_equal. (1.06 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: add soup_cookie_jar_delete function. (2.78 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: add docs for soup_cookie_jar_add_cookie. (1.09 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar-text: react to 'changed' signal. (6.34 KB, patch)
2008-10-27 21:10 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar-text: plug mem leak. (621 bytes, patch)
2008-10-27 22:20 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie: make get_type function thread safe. (824 bytes, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie: document and add to the header soup_cookie_header. (1.39 KB, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: update internal state before emitting 'changed' signal. (1.05 KB, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar-text: parse_cookie can return NULL, protect against that. (1.07 KB, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: delete expired cookies ASAP. (2.14 KB, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: mention memory management of cookies for add function in docs. (639 bytes, patch)
2008-10-28 10:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie: remove 'const' from copy function parameter. (1.58 KB, patch)
2008-10-28 10:53 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: remove get_cookies_for_domain, it's a bit pointless now. (1008 bytes, patch)
2008-10-28 18:55 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: delete cookies from jar on lookup too. (2.00 KB, patch)
2008-10-28 18:56 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar-text: put continue on its own line. (516 bytes, patch)
2008-10-28 18:56 UTC, Xan Lopez
committed Details | Review
Whole patchset against master. (234.64 KB, patch)
2008-10-28 19:06 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: stop using the save vmethod. (3.94 KB, patch)
2008-10-29 09:09 UTC, Xan Lopez
committed Details | Review
[PATCH] soup-cookie-jar: copy cookies for soup_cookie_jar_all_cookies. (876 bytes, patch)
2008-10-31 08:32 UTC, Xan Lopez
committed Details | Review

Description Dan Winship 2008-03-12 23:29:03 UTC
we need support for sending and receiving cookies

initial work for this can be seen on the "cookies" branch of
http://gnome.org/~danw/libsoup.git
Comment 1 Xan Lopez 2008-03-15 14:02:12 UTC
Hi,
I've been playing a bit with this now, trying to integrate it with webkit.

Some comments:
- The branch is strangely broken (all structs for the types are missing the initial _).
- Seems the way you envision the API is attaching the jar to the session and from there everything will happen automagically, right? It's a neat idea, but WebKit has an explicit setCookies() method to implement, so it would be more straightforward if we made public the set_cookie function as soup_cookie_jar_set_cookie (SoupCookieJar *jar, SoupCookie *cookie). Maybe we can just ignore that and it our own way, I'll try to investigate whether this is a problem.
Comment 2 Dan Winship 2008-03-15 17:12:01 UTC
(In reply to comment #1)
> - The branch is strangely broken (all structs for the types are missing the
> initial _).

Ah... yeah, I just grabbed the changed files from my cookie hacking working directory and copied them on top of a branch off head, without fixing anything for changes since then. (the "_"s on types were added very shortly before 2.4.0.). Fixed now.

> - Seems the way you envision the API is attaching the jar to the session and
> from there everything will happen automagically, right?

Right.

> It's a neat idea, but
> WebKit has an explicit setCookies() method to implement, so it would be more
> straightforward if we made public the set_cookie function as
> soup_cookie_jar_set_cookie (SoupCookieJar *jar, SoupCookie *cookie).

OK, the idea was that there would end up being multiple subclasses of SoupCookieJar. Like probably one in libsoup itself that would work with a traditional Mozilla-style cookies.txt file, and another somewhere else that supports the new Firefox 3 cookies.sqlite, for the online-desktop stuff. And then WebKit could implement its own cookie jar, which I guess wouldn't need any of the logic for figuring out when to send which cookies, because something else in WebKit is already figuring that out? Or is setCookies only used from javascript or something?

Anyway, whatever methods you guys need, we can adjust SoupCookieJar accordingly.
Comment 3 Xan Lopez 2008-03-15 20:32:11 UTC
I think you want something like this to avoid fun infinite loops btw :)

@@ -748,10 +748,11 @@ soup_cookies_to_response (GSList *cookies, SoupMessage *msg)

        header = g_string_new (NULL);
        while (cookies) {
                serialize_cookie (cookies->data, header, TRUE);
                soup_message_headers_append (msg->response_headers,
                                             "Set-Cookie", header->str);
                g_string_truncate (header, 0);
+               cookies = cookies->next;
        }
        g_string_free (header, TRUE);
 }
@@ -774,8 +775,10 @@ soup_cookies_to_request (GSList *cookies, SoupMessage *msg)

        header = g_string_new (soup_message_headers_get (msg->request_headers,
                                                         "Cookie"));
-       while (cookies)
-               serialize_cookie (cookies->data, header, FALSE);
+       while (cookies) {
+               soup_cookie_serialize_cookie (cookies->data, header, FALSE);
+               cookies = cookies->next;
+       }
        soup_message_headers_replace (msg->request_headers,
                                      "Cookie", header->str);
Comment 4 Xan Lopez 2008-03-15 23:42:47 UTC
Hi, I just got http cookies working with the soup backend, only needed to fix a few bugs in soup and attach the jar to the existing session. I'm attaching the patches here so you can review them, I can merge them into one big patch with ChangeLog to commit.

I'll look into cookies set from JS now, I think I only need to export the set_cookie functionality for this...
Comment 5 Xan Lopez 2008-03-15 23:43:31 UTC
Created attachment 107363 [details] [review]
[1/4] Use updated domain to lookup cookies.

 libsoup/soup-cookie-jar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Comment 6 Xan Lopez 2008-03-15 23:43:34 UTC
Created attachment 107364 [details] [review]
[2/4] Use g_slist_concat to concatenate files, no g_slist_append

 libsoup/soup-cookie-jar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Comment 7 Xan Lopez 2008-03-15 23:43:37 UTC
Created attachment 107365 [details] [review]
[3/4] Do not free memory when we want to return it.

 libsoup/soup-cookie.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Comment 8 Xan Lopez 2008-03-15 23:43:39 UTC
Created attachment 107366 [details] [review]
[4/4] Avoid inifinite loopes while adding cookies to headers.

 libsoup/soup-cookie.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Comment 9 Dan Winship 2008-03-16 00:19:55 UTC
These all look good. I want to branch libsoup before committing anything big like this, but feel free to commit to a git repo somewhere and I can pull it into mine. (Or maybe I can tweak the permissions on mine so you can push there too?)
Comment 10 Xan Lopez 2008-03-16 10:40:57 UTC
Ok, I've put my stuff in git://people.freedesktop.org/~xan/libsoup
Comment 11 Xan Lopez 2008-03-18 18:46:09 UTC
I've now pushed in my branch the needed API to implement the CookieJar class in WebKit. The bug is http://bugs.webkit.org/show_bug.cgi?id=17917
Comment 12 Dan Winship 2008-03-19 00:41:34 UTC
ok

get_cookies_for_domain() is not at all the method that WebKit::cookies() wants to be using, so just make that static again. You'll need a new method. Something like:

    char *soup_cookie_jar_get_cookies (SoupCookieJar *jar, SoupURI *uri,
                                       gboolean for_http);

which will use code extracted from soup_cookie_jar_add_cookie_header(). However, soup_cookie_jar_add_cookie_header() is currently unfinished; it needs to filter its list of cookies based on various things (path, ssl, expiredness, etc). update_for_cookies() is the beginning of such a filter. (I don't remember why it's called update_for_cookies() though... that doesn't seem to make any sense.) soup_cookie_applies_to_uri() is *also* such a filter. Probably it's best to just kill update_for_cookies and use soup_cookie_applies_to_uri() instead, although you have to check http_only too in addition to calling soup_cookie_applies_to_uri()... (That's what the "for_http" parameter is for; soup_cookie_jar_add_cookie_header() will pass
TRUE, telling it to include http_only cookies, while WebKit::cookies() will pass FALSE, telling it to omit them.)


Since soup_cookie_jar_get_cookies() will need to return a string, it can't use soup_cookies_to_request(). I don't like making serialize_cookie() into a public method though, with its confusing boolean flag and all. Instead, I'd make it static again and add

    char *soup_cookies_to_cookie_header(GSList *cookies);

that will do the serializing and concatenating for you. (Ie, basically soup_cookies_to_request, except without the get_header and set_header parts.) 


One more bug to fix: the call to domain_matches() in parse_one_cookie() is currently ifdeffed out, because the cookie-parsing test is lame and passes the same URI for every cookie, and so the test doesn't pass if SoupCookie is actually checking domains. But that's a bug in the test program, so you should just remove the #if 0, and we can come up with a better test program later.
Comment 13 Dan Winship 2008-03-19 00:45:44 UTC
Actually, if we're going to have a string-based soup_cookie_jar_get_cookies(), why not have soup_cookie_jar_set_cookie() take a string as well. Then you don't need to deal with SoupCookie directly at all in WebKit.
Comment 14 Dan Winship 2008-04-09 02:40:19 UTC
This is all committed to trunk now.

We still need:

    - support for persistent cookies
    - testing
    - ...?
Comment 15 Xan Lopez 2008-10-27 21:07:21 UTC
OK, I'll put here a few patches finishing the text storage for cookies in the 'cookies' branch.

Some random points:

- As mentioned on IRC, it probably makes sense to move the read_only and load stuff in SoupCookieJarText to the base class, as all subclasses are likely to do the same thing.

- It's very possible that the cookie management in the text file is crack :)

- I've got a weird crash from time to time in webkit where the cookies passed in the 'changed' signal are corrupted. Tried to run the thing under memcheck, no errors reported...

Anyway, comments welcome, but take this as a first draft.

If you want to actually test the stuff I can also attach the epiphany patch and the *very* crappy webkit hack.
Comment 16 Xan Lopez 2008-10-27 21:09:49 UTC
Created attachment 121464 [details] [review]
[PATCH] Add cookie-jar-text files to Makefile.am and soup.h

 libsoup/Makefile.am |    2 ++
 libsoup/soup.h      |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)
Comment 17 Xan Lopez 2008-10-27 21:09:53 UTC
Created attachment 121465 [details] [review]
[PATCH] Export set_cookie as soup_cookie_jar_set_cookie_internal.


Mostly useful for SoupCookieJar subclasses.
---
 libsoup/soup-cookie-jar-text.c |    2 +-
 libsoup/soup-cookie-jar.c      |   10 +++++-----
 libsoup/soup-cookie-jar.h      |   23 +++++++++++------------
 3 files changed, 17 insertions(+), 18 deletions(-)
Comment 18 Xan Lopez 2008-10-27 21:09:56 UTC
Created attachment 121466 [details] [review]
[PATCH] Add soup_cookie_jar_all_cookies.


Returns all cookies in the jar in a GSList.
---
 libsoup/soup-cookie-jar.c |   30 ++++++++++++++++++++++++++++++
 libsoup/soup-cookie-jar.h |    1 +
 2 files changed, 31 insertions(+), 0 deletions(-)
Comment 19 Xan Lopez 2008-10-27 21:10:00 UTC
Created attachment 121467 [details] [review]
[PATCH] Protect against NULL dates.

 libsoup/soup-date.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Comment 20 Xan Lopez 2008-10-27 21:10:04 UTC
Created attachment 121468 [details] [review]
[PATCH] Use G_{BEGIN,END}_DECLS in the header.

 libsoup/soup-cookie-jar.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Comment 21 Xan Lopez 2008-10-27 21:10:09 UTC
Created attachment 121469 [details] [review]
[PATCH] Create boxed type for SoupCookie.

 libsoup/soup-cookie.c |   32 +++++++++++++++++++
 libsoup/soup-cookie.h |   83 +++++++++++++++++++++++--------------------------
 2 files changed, 71 insertions(+), 44 deletions(-)
Comment 22 Xan Lopez 2008-10-27 21:10:14 UTC
Created attachment 121470 [details] [review]
[PATCH] Add 'changed' signal.


The signature is void (*changed) (SoupCookieJar *jar,
                                  SoupCookie *old_cookie,
                                  SoupCookie *new_cookie);
Emitted when a cookie is:

- added (old_cookie = NULL, new_cookie != NULL)
- changed (old_cookie != NULL, new_cookie != NULL)
- deleted (old_cookie != NULL, new_cookie = NULL)

We emit added and changed cases for now, as there isn't yet a way to
delete cookies from the jar.
---
 libsoup/soup-cookie-jar.c |   31 +++++++++++++++++++++++++++++--
 libsoup/soup-cookie-jar.h |    8 ++++++--
 libsoup/soup-marshal.list |    1 +
 3 files changed, 36 insertions(+), 4 deletions(-)
Comment 23 Xan Lopez 2008-10-27 21:10:17 UTC
Created attachment 121471 [details] [review]
[PATCH] soup-cookie: free expire date when freeing cookie.

 libsoup/soup-cookie.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Comment 24 Xan Lopez 2008-10-27 21:10:20 UTC
Created attachment 121472 [details] [review]
[PATCH] soup-cookie-jar: rename set_internal to add.

 libsoup/soup-cookie-jar.c |    6 +++---
 libsoup/soup-cookie-jar.h |   24 ++++++++++++------------
 2 files changed, 15 insertions(+), 15 deletions(-)
Comment 25 Xan Lopez 2008-10-27 21:10:23 UTC
Created attachment 121473 [details] [review]
[PATCH] soup-cookie: add soup_cookie_equal.


Compares path, name and value fields. Not sure if it makes sense to
compare anything else.
---
 libsoup/soup-cookie.c |    9 +++++++++
 libsoup/soup-cookie.h |    2 ++
 2 files changed, 11 insertions(+), 0 deletions(-)
Comment 26 Xan Lopez 2008-10-27 21:10:26 UTC
Created attachment 121474 [details] [review]
[PATCH] soup-cookie-jar: add soup_cookie_jar_delete function.

 libsoup/soup-cookie-jar.c |   41 +++++++++++++++++++++++++++++++++++++++++
 libsoup/soup-cookie-jar.h |   26 ++++++++++++++------------
 2 files changed, 55 insertions(+), 12 deletions(-)
Comment 27 Xan Lopez 2008-10-27 21:10:29 UTC
Created attachment 121475 [details] [review]
[PATCH] soup-cookie-jar: add docs for soup_cookie_jar_add_cookie.

 libsoup/soup-cookie-jar.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
Comment 28 Xan Lopez 2008-10-27 21:10:32 UTC
Created attachment 121476 [details] [review]
[PATCH] soup-cookie-jar-text: react to 'changed' signal.


Manage persistent storage of cookies by installing a handler for the
'changed' signal.

Also refactor and simplify some of the code.
---
 libsoup/soup-cookie-jar-text.c |  186 +++++++++++++++++++++++++++++----------
 1 files changed, 138 insertions(+), 48 deletions(-)
Comment 29 Dan Winship 2008-10-27 22:11:07 UTC
OK, this mostly looks good. I've already committed the non-cookie-jar-related bug fixes.

(In reply to comment #21)
> Created an attachment (id=121469) [edit]
> [PATCH] Create boxed type for SoupCookie.

The get_type method needs to use fully-thread-safe g_once version. Look at soup_date_get_type() for an example.

soup_cookie_copy should be either static or gtk-doc'ed. If you keep it in the public API, remove the "const", for consistency with everywhere else where we don't specify that struct types are const.

(In reply to comment #22)
> We emit added and changed cases for now, as there isn't yet a way to
> delete cookies from the jar.

They get deleted automatically when it notices that they've expired. Currently this only happens at lookup time, as mentioned by the comment in set_cookie_internal, but probably it should be changed so that it checks the expiration date there, and does the removal (and corresponding removal-type CHANGED signal) then. (Note that this is actually how web sites ask the browser to delete a cookie; by sending it a new already-expired one.)

> +			g_signal_emit (jar, signals[CHANGED], 0,
> +				       old_cookie, cookie);
>  			soup_cookie_free (old_cookie);
>  			oc->data = cookie;

You should set oc->data *before* emitting the signal, to ensure a consistent state if the caller calls some other SoupCookieJar method from the signal handler. (Possibly related to your crash?)

(In reply to comment #24)
> Created an attachment (id=121472) [edit]
> [PATCH] soup-cookie-jar: rename set_internal to add.

Actually, the odd ref-stealing semantics suggest "soup_cookie_jar_take_cookie()" might be a better name. Either way, the docs need to mention the fact that the cookie is stolen or freed. (Also possibly related to your crash?)

(In reply to comment #26)
> Created an attachment (id=121474) [edit]
> [PATCH] soup-cookie-jar: add soup_cookie_jar_delete function.

This has the same problem with the CHANGED signal as above; it needs to remove the cookies from its internal state first, and then emit all the CHANGED signals afterward.

(In reply to comment #28)
> Created an attachment (id=121476) [edit]
> [PATCH] soup-cookie-jar-text: react to 'changed' signal.

This will end up rewriting the cookies.txt file N times in a row if the server sends back N cookies. It would be better to set a timeout and write out the changed cookies.txt from there. Except that the app might be using a SoupSessionSync and there isn't any main loop... feh. I need to think about this.
Comment 30 Xan Lopez 2008-10-27 22:20:36 UTC
Created attachment 121482 [details] [review]
[PATCH] soup-cookie-jar-text: plug mem leak.

 libsoup/soup-cookie-jar-text.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Comment 31 Xan Lopez 2008-10-28 10:05:57 UTC
OK, I've addressed all of your points except the last one. And yeah, in this case I was aware that we'll copy the whole file for each cookie deleted, but couldn't figure a better/easier way of doing it (as you mention), so I decided to postpone the optimization for now :)

With this patches I can browse for a long time without crashes, the flash plugin will usually crash the thing before any cookies problem. I'd say most crashes went away when I checked the return value of parse_cookie (oops...), but probably the weird memory corruptions were due to something else.
Comment 32 Xan Lopez 2008-10-28 10:06:37 UTC
Created attachment 121502 [details] [review]
[PATCH] soup-cookie: make get_type function thread safe.

 libsoup/soup-cookie.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
Comment 33 Xan Lopez 2008-10-28 10:06:41 UTC
Created attachment 121503 [details] [review]
[PATCH] soup-cookie: document and add to the header soup_cookie_header.

 libsoup/soup-cookie.c |    9 +++++++++
 libsoup/soup-cookie.h |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)
Comment 34 Xan Lopez 2008-10-28 10:06:44 UTC
Created attachment 121504 [details] [review]
[PATCH] soup-cookie-jar: update internal state before emitting 'changed' signal.


Otherwise users connected to the signal mind find the jar in an
inconsistent state.
---
 libsoup/soup-cookie-jar.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Comment 35 Xan Lopez 2008-10-28 10:06:48 UTC
Created attachment 121505 [details] [review]
[PATCH] soup-cookie-jar-text: parse_cookie can return NULL, protect against that.

 libsoup/soup-cookie-jar-text.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Comment 36 Xan Lopez 2008-10-28 10:06:52 UTC
Created attachment 121506 [details] [review]
[PATCH] soup-cookie-jar: delete expired cookies ASAP.


Do it directly in add_cookie instead of waiting until the lookup phase.
---
 libsoup/soup-cookie-jar.c |   48 ++++++++++++++++++++------------------------
 1 files changed, 22 insertions(+), 26 deletions(-)
Comment 37 Xan Lopez 2008-10-28 10:06:55 UTC
Created attachment 121507 [details] [review]
[PATCH] soup-cookie-jar: mention memory management of cookies for add function in docs.

 libsoup/soup-cookie-jar.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Comment 38 Xan Lopez 2008-10-28 10:53:19 UTC
Created attachment 121511 [details] [review]
[PATCH] soup-cookie: remove 'const' from copy function parameter.


Also remove duplicated declaration from header.
---
 libsoup/soup-cookie.c |    2 +-
 libsoup/soup-cookie.h |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)
Comment 39 Dan Winship 2008-10-28 13:12:14 UTC
> +			if (!c) continue;

please split that into two lines

(In reply to comment #36)
> Created an attachment (id=121506) [edit]
> [PATCH] soup-cookie-jar: delete expired cookies ASAP.

er, sorry, it should delete them both at set time AND at lookup time, so that we don't send a cookie if it expires between requests.

can you either push your changes somewhere or else attach a single patch with all the differences so I can look at the big picture more easily?
Comment 40 Xan Lopez 2008-10-28 13:23:19 UTC
> (In reply to comment #36)
> > Created an attachment (id=121506) [edit]
> > [PATCH] soup-cookie-jar: delete expired cookies ASAP.
> 
> er, sorry, it should delete them both at set time AND at lookup time, so that
> we don't send a cookie if it expires between requests.
> 

Mmm, right, I think I just moved the code around in the set case actually. What needs to be done is to remove the cookie from the jar in soup_cookie_jar_get_cookies when the cookie has expired, right?

> can you either push your changes somewhere or else attach a single patch with
> all the differences so I can look at the big picture more easily?
>

OK, I'll attach jar.c and jar-text.c. 

Comment 41 Dan Winship 2008-10-28 17:21:19 UTC
> Mmm, right, I think I just moved the code around in the set case actually. What
> needs to be done is to remove the cookie from the jar in
> soup_cookie_jar_get_cookies when the cookie has expired, right?

yeah, just put back the code you removed. and make it emit the signal(s) at the end.
Comment 42 Xan Lopez 2008-10-28 18:55:59 UTC
Created attachment 121529 [details] [review]
[PATCH] soup-cookie-jar: remove get_cookies_for_domain, it's a bit pointless now.

 libsoup/soup-cookie-jar.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)
Comment 43 Xan Lopez 2008-10-28 18:56:03 UTC
Created attachment 121530 [details] [review]
[PATCH] soup-cookie-jar: delete cookies from jar on lookup too.

 libsoup/soup-cookie-jar.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)
Comment 44 Xan Lopez 2008-10-28 18:56:06 UTC
Created attachment 121531 [details] [review]
[PATCH] soup-cookie-jar-text: put continue on its own line.

 libsoup/soup-cookie-jar-text.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Comment 45 Xan Lopez 2008-10-28 18:57:29 UTC
OK, as discussed on IRC:

- No need to revert the patch about deleting cookies on add: it's more clear that way and get_cookies_for_domain is only used by add (removed the function in a patch now that it's a one-liner).

- New patch to delete cookies on lookup too (soup_cookie_jar_get_cookies).
Comment 46 Xan Lopez 2008-10-28 19:06:13 UTC
Created attachment 121533 [details] [review]
Whole patchset against master.
Comment 47 Xan Lopez 2008-10-29 09:09:14 UTC
Created attachment 121567 [details] [review]
[PATCH] soup-cookie-jar: stop using the save vmethod.


We don't need anything else than the 'changed' signal.

Can't remove it completely because it was shipped in a stable version
already.
---
 libsoup/soup-cookie-jar-text.c |   62 +++++++++-------------------------------
 libsoup/soup-cookie-jar.c      |    3 +-
 libsoup/soup-cookie-jar.h      |    1 +
 3 files changed, 16 insertions(+), 50 deletions(-)
Comment 48 Dan Winship 2008-10-29 19:47:48 UTC
all patches are now pushed to the cookies branch in my repo, hopefully in such a way that you can pull wihtout causing a zillion conflicts
Comment 49 Dan Winship 2008-10-29 22:02:47 UTC
still to be done:

    - pull in Diego's sqlite branch, fix that for the various changes.
      (I don't want to merge this to trunk until we're convinced that
      SoupCookieJar will work well for both Text and Sqlite, even if we
      don't push the sqlite backend to trunk right away.)

        - This will require libsoup-gnome. The way it's going to work
          is that configure.in will have a --without-gnome flag for
          people to build just libsoup.la/libsoup-2.4.pc, but the default
          will be to build both libsoup.la and libsoup-gnome.la (and
          corresponding .pc files). There may also be a --without-sqlite
          if you want to build libsoup-gnome, but without sqlite.
          (Obviously that won't make sense until there are other features
          in libsoup-gnome :-)

          I'll probably do the initial libsoup-gnome stuff on a separate
          branch, and then merge it into cookies, keyring, and proxy.

    - fix the cookies regression test

    - fixes to some of the new stuff:

        - I think soup_cookie_equal() should check name and domain and
          nothing else; essentially, that the two cookies could not both
          be in the cookie jar at the same time, because they're the
          same cookie, just possibly with different values/expiration
          times. But then _equal() isn't a good name for it. I'm thinking
          soup_cookie_equivalent() maybe. Anyway, all of the current uses
          of it would still be fine with the new semantics.

        - soup_cookie_jar_all_cookies() is slightly tricky in that if you
          make any SoupCookieJar calls after getting back the list of
          cookies, some of the cookies might be freed. So it probably needs
          to copy each of the cookies. Not that big a deal since it's
          really only needed by the "View Cookies" dialog, so the extra
          copying time shouldn't hurt anything.

        - more?

    - make sure the docs are all up-to-date at the end
Comment 50 Christian Dywan 2008-10-29 22:39:20 UTC
(In reply to comment #49)
> still to be done:
> 
>     - pull in Diego's sqlite branch, fix that for the various changes.
>       (I don't want to merge this to trunk until we're convinced that
>       SoupCookieJar will work well for both Text and Sqlite, even if we
>       don't push the sqlite backend to trunk right away.)
> 
>         - This will require libsoup-gnome. The way it's going to work
>           is that configure.in will have a --without-gnome flag for
>           people to build just libsoup.la/libsoup-2.4.pc, but the default
>           will be to build both libsoup.la and libsoup-gnome.la (and
>           corresponding .pc files). There may also be a --without-sqlite
>           if you want to build libsoup-gnome, but without sqlite.
>           (Obviously that won't make sense until there are other features
>           in libsoup-gnome :-)
> 
>           I'll probably do the initial libsoup-gnome stuff on a separate
>           branch, and then merge it into cookies, keyring, and proxy.

Can you please point out roughly what libsoup-gnome is meant to be? For instance, I read your comment as sqlite support being part of that, albeit I don't think sqlite is platform specific at all. If you actually meant libsoup-gnome to be something that actually specifies a set of features particularly useful for Gnome that of course makes sense as long as the specific features are indeed usable separately.

> 
>     - fix the cookies regression test
> 
>     - fixes to some of the new stuff:
> 
>         - I think soup_cookie_equal() should check name and domain and
>           nothing else; essentially, that the two cookies could not both
>           be in the cookie jar at the same time, because they're the
>           same cookie, just possibly with different values/expiration
>           times. But then _equal() isn't a good name for it. I'm thinking
>           soup_cookie_equivalent() maybe. Anyway, all of the current uses
>           of it would still be fine with the new semantics.

Incidentally to me "equal" and "equivalent" is exactly the same. For what I want, "equal" to my understanding implies that if cookie1 equals cookie2, inserting both one after another into the same jar means that the second one replaces the previous one. So _equal should yield TRUE if that is the case.
Comment 51 Dan Winship 2008-10-30 02:00:59 UTC
(In reply to comment #50)
> Can you please point out roughly what libsoup-gnome is meant to be?

"Things that can't be done with just the libraries libsoup currently depends on (glib, libxml2, gnutls), but can be done with other libraries that are GNOME dependencies."

There are people using libsoup who don't want to add further GNOME-ish dependencies, so new GNOME-ish dependencies go into libsoup-gnome.

> For
> instance, I read your comment as sqlite support being part of that, albeit I
> don't think sqlite is platform specific at all. If you actually meant
> libsoup-gnome to be something that actually specifies a set of features
> particularly useful for Gnome that of course makes sense as long as the
> specific features are indeed usable separately.

Well, in general they will all be built on top of abstract classes in libsoup proper. So libsoup will have SoupCookieJar, SoupPasswordManager, and SoupProxyManager, and libsoup-gnome will have SoupCookieJarSqlite, SoupPasswordManagerGNOME (using gnome-keyring), and SoupProxyManagerLibproxy (using libproxy, assuming that gets accepted as a dependency). So people who don't want to link to libsoup-gnome can still build their own implementations of the features.

I don't think it's reasonable to allow libsoup to be optionally compiled with or without each possible dependency.

FWIW I was assuming WebKit would link to libsoup-gnome (so it can integrate with gnome-keyring, gconf proxies, etc). But if it doesn't want to, it can still either (a) dlopen it, or (b) allow the application to create the SoupSession itself, so that, eg, epiphany can pass it a session making use of all the libsoup-gnome features.

> Incidentally to me "equal" and "equivalent" is exactly the same. For what I
> want, "equal" to my understanding implies that if cookie1 equals cookie2,
> inserting both one after another into the same jar means that the second one
> replaces the previous one. So _equal should yield TRUE if that is the case.

"equal" to me suggests that they have the same *value*. If "equivalent" also suggests that, then it might not be the best name.
Comment 52 Christian Dywan 2008-10-30 10:04:23 UTC
(In reply to comment #51)
> (In reply to comment #50)
> > Can you please point out roughly what libsoup-gnome is meant to be?
> 
> "Things that can't be done with just the libraries libsoup currently depends on
> (glib, libxml2, gnutls), but can be done with other libraries that are GNOME
> dependencies."
> 
> There are people using libsoup who don't want to add further GNOME-ish
> dependencies, so new GNOME-ish dependencies go into libsoup-gnome.
> 
> > For
> > instance, I read your comment as sqlite support being part of that, albeit I
> > don't think sqlite is platform specific at all. If you actually meant
> > libsoup-gnome to be something that actually specifies a set of features
> > particularly useful for Gnome that of course makes sense as long as the
> > specific features are indeed usable separately.
> 
> Well, in general they will all be built on top of abstract classes in libsoup
> proper. So libsoup will have SoupCookieJar, SoupPasswordManager, and
> SoupProxyManager, and libsoup-gnome will have SoupCookieJarSqlite,
> SoupPasswordManagerGNOME (using gnome-keyring), and SoupProxyManagerLibproxy
> (using libproxy, assuming that gets accepted as a dependency). So people who
> don't want to link to libsoup-gnome can still build their own implementations
> of the features.
> 
> I don't think it's reasonable to allow libsoup to be optionally compiled with
> or without each possible dependency.

I totally agree that GnomeKeyRing is only interesting to Gnome in particular, as for libproxy I don't even know that library yet. And I do also agree that we don't want to end up in an --enable-foo mess (like for example WebKitGtk, which hopefully is about to change soon). Yet sqlite is perfectly useful on any platform, and if it is even a seemless replacement for the text backend, it could just be the packager's choice or the user's choice to use it (provided it's a module).
I suppose we should hold on the thought until those features actually exist, though. Let's leave the tar to cool and become a street before attempting to paint the stripes on it. ^_~

> FWIW I was assuming WebKit would link to libsoup-gnome (so it can integrate
> with gnome-keyring, gconf proxies, etc). But if it doesn't want to, it can
> still either (a) dlopen it, or (b) allow the application to create the
> SoupSession itself, so that, eg, epiphany can pass it a session making use of
> all the libsoup-gnome features.

Indeed WebKit shouldn't build depend on the platform. "(a) dlopen" sounds preferrable since that would mean that any WebKit application could integrate properly without any extra effort. The same could even be done on other platforms if needed, a bit like GIO does it.

> > Incidentally to me "equal" and "equivalent" is exactly the same. For what I
> > want, "equal" to my understanding implies that if cookie1 equals cookie2,
> > inserting both one after another into the same jar means that the second one
> > replaces the previous one. So _equal should yield TRUE if that is the case.
> 
> "equal" to me suggests that they have the same *value*. If "equivalent" also
> suggests that, then it might not be the best name.

What about "match"? That's for instance terminology used in the context of regular expressions, which does not mean equality but *matching* defined criteria. Granted, in the case of the cookie jar, the criteria are immutable.
Comment 53 Xan Lopez 2008-10-31 08:29:26 UTC
I think soup_cookie_compare is much more neutral than equal/equivalent/match/whatever (neutral in a tricky way, because it just omits information, but still neutral).
Comment 54 Xan Lopez 2008-10-31 08:32:40 UTC
Created attachment 121719 [details] [review]
[PATCH] soup-cookie-jar: copy cookies for soup_cookie_jar_all_cookies.


The jar can be modified after the user gets the list of cookies, so
this is much safer.
---
 libsoup/soup-cookie-jar.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Comment 55 Dan Winship 2008-10-31 11:14:01 UTC
committed and pushed. still trying to decide on soup_cookie_equal().

I was trying to fix up the cookie regression test, and decided I'm probably not going to end up finishing it now (for 2.25.1), but I already found two bugs in soup-cookie.c:

    * A response from foo.com wasn't being allowed to set a cookie for
      domain=.foo.com

    * soup_cookie_applies_to_uri() wasn't actually checking paths,
      resulting in extra cookies being sent.
Comment 56 Dan Winship 2008-11-04 21:29:34 UTC
Merged 99% of the cookies branch to svn trunk for 2.25.1. Further cookie problems/feature requests should be filed as separate bugs.