Bug 82082 – [SOUP] Implement missing methods in CookieJarSoup

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

getRawCookies() and deleteCookie() are currently unimplemented. They are used by the web inspector to show the list of cookies and allow to delete them. This is nice to have but it's also important for WebKit2, since there's no API to get the list of cookies in the UI nor to remove a given cookie.

[Carlos Garcia Campos]

Created attachment 133546 [details]
Patch

I've also cleaned up the code a bit using DEFINE_STATIC_LOCAL for the global jar and GOwnPtr/GRefPtr everywhere.

[Martin Robinson]

Comment on attachment 133546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133546&action=review

Nice! Consider reverting the changes to deleteCookiesForHostname and deleteAllCookies. The old style seemed a more explicit about what was happening.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:128
> +        // FIXME we are currently passing false always for seesion because there's no API to know

Missing a ':' after FIXME.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:149
> +    CString cookieName = name.utf8();
> +

Extra newline here.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:165
> +        GOwnPtr<SoupCookie> cookie(static_cast<SoupCookie*>(item->data));

It's was actually a bit tricky for me to see how this worked now, with a GOwnPtr assigned the cookie late in its life. I'm not sure if I prefer this approach versus the old one.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:164
> -            setDefaultCookieJar(jar);
> +            setSoupCookieJar(jar);

I think your new name is far more appropriate.

[Gyuyoung Kim]

Comment on attachment 133546 [details]
Patch

Attachment 133546 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12117813

[Martin Robinson]

Please fix the EFL port before landing. :)

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 133546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133546&action=review
> 
> Nice! Consider reverting the changes to deleteCookiesForHostname and deleteAllCookies. The old style seemed a more explicit about what was happening.

well, not using smart pointers is always more explicit, yes.

> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:128
> > +        // FIXME we are currently passing false always for seesion because there's no API to know
> 
> Missing a ':' after FIXME.

Ok.

> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:149
> > +    CString cookieName = name.utf8();
> > +
> 
> Extra newline here.

I'll remove this one.

> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:165
> > +        GOwnPtr<SoupCookie> cookie(static_cast<SoupCookie*>(item->data));
> 
> It's was actually a bit tricky for me to see how this worked now, with a GOwnPtr assigned the cookie late in its life. I'm not sure if I prefer this approach versus the old one.

what's is tricky exactly? soup_cookie_jar_all_cookies() returns a transfer full GSList, so we need a GOwnPtr for the container and another one for every item.

[Carlos Garcia Campos]

(In reply to comment #4)
> Please fix the EFL port before landing. :)

Sure! It seems I missed a couple of s/Default/Soup/. Sorry :-(

[Martin Robinson]

(In reply to comment #5)

> what's is tricky exactly? soup_cookie_jar_all_cookies() returns a transfer full GSList, so we need a GOwnPtr for the container and another one for every item.

GOwnPtr is typically used for the entire lifetime of an object. In this case you are assigning a pre-existing objects to GOwnPtrs and they are freed almost as a side-effect of that.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> 
> > what's is tricky exactly? soup_cookie_jar_all_cookies() returns a transfer full GSList, so we need a GOwnPtr for the container and another one for every item.
> 
> GOwnPtr is typically used for the entire lifetime of an object. In this case you are assigning a pre-existing objects to GOwnPtrs and they are freed almost as a side-effect of that.

I don't get what you mean, GOwnPtr always frees the pointer as a side-effect for me. We usually do something like

GOwnPtr<char> foo(g_strdup("Foo"));
use_foo (bar, foo.get());

and that's supposed to be fine. 

This is the same, we have a method that returns a new allocated list, which transfers the full list, container and contents, so you are expected to free it, the same way g_strdup() returns a new allocated string that you should free it when done. 

soup_cookies_free() is convenient method to free both the container and the contents. It iterates the list calling soup_cookie_free() for every item (like our GOwnPtr is doing) and then calls g_slist_free() (like our GOwnPtr does). 

When an object returns a transfer full list, you typically use g_list_free_full() or g_list_foreach() + g_list_free() when you are not iterating the list, but when you need to iterate the list, you can free every item when iterating and then free the list to avoid iterating the list twice. This second case is what the patch does.

[Martin Robinson]

(In reply to comment #8)

> I don't get what you mean, GOwnPtr always frees the pointer as a side-effect for me. We usually do something like
> 
> GOwnPtr<char> foo(g_strdup("Foo"));
> use_foo (bar, foo.get());
> 
> and that's supposed to be fine. 
> 
> This is the same, we have a method that returns a new allocated list, which transfers the full list, container and contents, so you are expected to free it, the same way g_strdup() returns a new allocated string that you should free it when done. 

Note the "Own" in GOwnPtr. The way it's typically designed to work is that OwnPtr immediately contains a raw pointer after allocation. It owns it. The WTF version even has PassOwnPtr, which tries to enforce this more strongly. 

In this case GOwnPtr doesn't "own" the pointers in the GSList. Conceptually speaking, they are owned by the GSList itself. soup_cookies_free frees both the list and the individual cookies, because in a sense they are one complete unit of data. In your patch GOwnPtr is simply used as a mechanism to free the cookies that are owned by the list.

This is why it was confusing to me.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > I don't get what you mean, GOwnPtr always frees the pointer as a side-effect for me. We usually do something like
> > 
> > GOwnPtr<char> foo(g_strdup("Foo"));
> > use_foo (bar, foo.get());
> > 
> > and that's supposed to be fine. 
> > 
> > This is the same, we have a method that returns a new allocated list, which transfers the full list, container and contents, so you are expected to free it, the same way g_strdup() returns a new allocated string that you should free it when done. 
> 
> Note the "Own" in GOwnPtr. The way it's typically designed to work is that OwnPtr immediately contains a raw pointer after allocation. It owns it. The WTF version even has PassOwnPtr, which tries to enforce this more strongly. 

transfer container means you owns the container, transfer full means you own both the container and the items contained. 

> In this case GOwnPtr doesn't "own" the pointers in the GSList. Conceptually speaking, they are owned by the GSList itself. soup_cookies_free frees both the list and the individual cookies, because in a sense they are one complete unit of data. In your patch GOwnPtr is simply used as a mechanism to free the cookies that are owned by the list.

soup_cookies_free is just a convenient function to free both the list and the items with a single call.

> This is why it was confusing to me.

[Carlos Garcia Campos]

Committed r112234: <http://trac.webkit.org/changeset/112234>