Bug 177932 – [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

As discussed during the Web Engines Hackfest, it would be great to have a way to add and retrieve (full) cookies into/out-of a running session via the WebKitCookieManager, since that would make it possible to improve the integration between the desktop and apps.

For instance, in Endless OS (a desktop based on GNOME) we'd like to be able to reuse the cookie stored by gnome-online-accounts so that you don't have to login again in your favourite online service from the browser (or from any app) if you already did it in g-o-a, for which this API would be very useful.

I'll be sending a mail to the mailing list in a few min to discuss the details of the proposed API itself, so filing this bug to track the actual work here

[Mario Sanchez Prada]

Mailing list discussion here: https://lists.webkit.org/pipermail/webkit-gtk/2017-October/003154.html

[Mario Sanchez Prada]

I've updated the name of this bug to reflect that I'm also adding a delete_cookie() method now, as mentioned in the mailing list:
https://lists.webkit.org/pipermail/webkit-gtk/2017-October/003155.html
https://lists.webkit.org/pipermail/webkit-gtk/2017-October/003157.html

[Mario Sanchez Prada]

Created attachment 323302 [details]
Patch proposal

See a patch proposal attached. The only change I did compared to my original proposal in the mailing list is that the functions add_cookie() and get_cookies() now receive a SoupURI* parameter instead of a const char*, to make sure that they are valid HTTP/HTTPs URLs (and also because it's easier and more natural to validate that those are valid URIs for HTTP/HTTPs right at the beginning of the functions).

Please review, thanks!

[Mario Sanchez Prada]

Created attachment 323310 [details]
Patch proposal

Let's try to fix the WPE bot (and fix some minor errors in the previous patch along the way)

[Adrian Perez]

Comment on attachment 323310 [details]
Patch proposal

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

Informal review: r- 

It would be r+ if there wasn't doubts about error handling (is it missing?
is it uneeded?). Please check the inline comments below regarding this.

Otherwise, the added API looks good to me, and you did a good job adding
the tests for the API. Thanks a lot for working on this patch!

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> + * request specified by @uri, which must be either and HTTP or an HTTP URL. In case the

Probably you meant “either an HTTP or an HTTPS URL”.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:280
> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);

What if the error is other than CallbackBase::Error::None? This is never
returning an error in that case e.g. using g_task_return_new_error(). I
would expect this to be something like:

  if (error == CallbackBase::Error::None) {
      g_task_return_boolen(task.get(), TRUE);
  } else {
      g_task_return_error(task.get(), toGError(error));
  }

If the current behavior is intended, you can remove the GError argument to
webkit_cookie_manager_add_cookie_finish() — read move below.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:294
> +gboolean webkit_cookie_manager_add_cookie_finish(WebKitCookieManager *manager, GAsyncResult *result, GError **error)

The GError argument may not be needed if the WebCore/WebKit error
will not be converted to a GError, as argumented above. Either
remove it here, or add the conversion to GError above as suggested.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:305
> + * @uri: the #SoupURI for which the list of cookies will be retrieved from

I think this should have the same parameter description as the
webkit_cookie_manager_add_cookie() function above, for consistency:

  @uri: the #SoupURI with the URI of the originating request

(Also, as a bonus this wording parses more easily in my head.)

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:311
> + * must be either and HTTP or an HTTP URL.

Ditto, probably you wanted “either an HTTP or an HTTPS URL” here as well.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:328
> +    processPools[0]->supplement<WebCookieManagerProxy>()->getCookies(sessionID, WebCore::URL(uri), [task = WTFMove(task)](const Vector<WebCore::Cookie>& cookies, CallbackBase::Error error) {

The “error” argument is never checked. If it's guaranteed to always
indicate a successful operation, then please add an assertion:

  ASSERT_EQ(error, CallbackBase::Error::None);

otherwise, please add the needed bits to convert the error codes to
GError values and to report them to the code using this API.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:395
> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);

Same comment as for webkit_cookie_manager_add_cookie(): How about converting
errors to GError, or removing the GError parameter to the _finish() function
(whichever fits in this case)?

[Mario Sanchez Prada]

Hey Adrián! Not sure what happened with the my mail filters for wk bugmail, but I didn't see your review until now (many thanks, btw!). I'm a bit busy at the moment with WK-unrelated matters, but I will be taking a look to your comments as soon as possible (hopefully this week). WebKit has changed quite a bit since the last time I touched it "seriously" and I need to wrap my head around a few things before I'm able to productively engage with your review, but I hope that will happen soon enough anyway :-)

In the meantime, if anyone has any other comments I'd appreciate them too, of course.

[Mario Sanchez Prada]

Comment on attachment 323310 [details]
Patch proposal

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

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
>> + * request specified by @uri, which must be either and HTTP or an HTTP URL. In case the
> 
> Probably you meant “either an HTTP or an HTTPS URL”.

Oops! Will fix that

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:280
>> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);
> 
> What if the error is other than CallbackBase::Error::None? This is never
> returning an error in that case e.g. using g_task_return_new_error(). I
> would expect this to be something like:
> 
>   if (error == CallbackBase::Error::None) {
>       g_task_return_boolen(task.get(), TRUE);
>   } else {
>       g_task_return_error(task.get(), toGError(error));
>   }
> 
> If the current behavior is intended, you can remove the GError argument to
> webkit_cookie_manager_add_cookie_finish() — read move below.

Another "Ooops!", but this one is actually worse. TL;DR: I think you're right, I'll change it

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:294
>> +gboolean webkit_cookie_manager_add_cookie_finish(WebKitCookieManager *manager, GAsyncResult *result, GError **error)
> 
> The GError argument may not be needed if the WebCore/WebKit error
> will not be converted to a GError, as argumented above. Either
> remove it here, or add the conversion to GError above as suggested.

I think the GError should be filled, even if it's with a generic G_IO_ERROR_CANCELLED, in case something went wrong, so I'll keep it.

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:305
>> + * @uri: the #SoupURI for which the list of cookies will be retrieved from
> 
> I think this should have the same parameter description as the
> webkit_cookie_manager_add_cookie() function above, for consistency:
> 
>   @uri: the #SoupURI with the URI of the originating request
> 
> (Also, as a bonus this wording parses more easily in my head.)

Fair enough

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:311
>> + * must be either and HTTP or an HTTP URL.
> 
> Ditto, probably you wanted “either an HTTP or an HTTPS URL” here as well.

Yep

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:328
>> +    processPools[0]->supplement<WebCookieManagerProxy>()->getCookies(sessionID, WebCore::URL(uri), [task = WTFMove(task)](const Vector<WebCore::Cookie>& cookies, CallbackBase::Error error) {
> 
> The “error” argument is never checked. If it's guaranteed to always
> indicate a successful operation, then please add an assertion:
> 
>   ASSERT_EQ(error, CallbackBase::Error::None);
> 
> otherwise, please add the needed bits to convert the error codes to
> GError values and to report them to the code using this API.

I think I'll add the needed bits to return an GError in case it failed, for the same reasons explained above

>> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:395
>> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);
> 
> Same comment as for webkit_cookie_manager_add_cookie(): How about converting
> errors to GError, or removing the GError parameter to the _finish() function
> (whichever fits in this case)?

Providing a proper GError sounds good

[Mario Sanchez Prada]

Created attachment 325357 [details]
Patch proposal

See attached a re-worked version of my previous patch after considering the review comments from Adrián along with yet-one-more-change to the API add_cookie() API, whose signature is now as follows:

   void webkit_cookie_manager_add_cookie(WebKitCookieManager *manager, SoupCookie *cookie, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer userData)

You might have noticed that I removed the SoupURI* uri parameter, and the reason is because I realized that it was not only unnecessary but even wrong for this API: while it's important to use setCookies(Vector<Cookie> cookies, URL url, URL mainDocumentURL) when adding cookies as the result of the web engine loading resources from the web (so that it can be considered whether cookies come from 1st or 3rd parties), this API is meant to be used from other applications/components using the WebKit2GTK+ API to interact with the underlying cookie jar in a much more direct way, meaning that page loading stuff is not really relevant and thus passing URLs here besides the `domain` property that is already (mandatory) part of SoupCookie doesn't really buy us anything.

Actually, is not only that it doesn't buy us anything, but also that such URL would be plainly ignored by the soup layer when adding the cookie to the jar, since all the soup_cookie_jar_add_cookie() API takes is a reference to the Jar and to the SoupCookie, from which it extracts the domain to later on know which cookies it needs to send to which websites when loading them.

So, there's no URL-checking code in WebCore when adding a new cookie to the jar, how is it checked that a cookie being loaded from a random website can actually be added? Well, that's via soup_cookie_jar_set_accept_policy() -already exposed by the WebKitCookieManager API-, which allows deciding whether libsoup should add or not a set of cookies depending on whether that cookie came with an HTTP response that belongs to a 1st or a 3rd party. And to know that information, the soup-based network layer in WebCore takes care of calling soup_message_set_first_party() for the messages that come from the same domain than the one loaded in the main document, so that when soup_cookie_jar_add_cookie() is called later on, libsoup knows what to do.

Anyway, long way of saying that both the API and its internals can be simplified now and the only thing that needs to be passed to add_cookie() is, unsurprisingly, a SoupCookie with the name, domain, path and value properties properly initialized, and WebKit will take care of the rest when adding it to the underlying storage. For that reason, I also moved from using setCookies(Vector<Cookie> cookies, URL url, URL mainDocumentURL) to using setCookie(Cookie* cookie) instead -and implementing it-, since that's really all we need.

Last, I've also updated/extended the unit tests to make sure that I tested everything I could think of, and I think it's now even in better shape than before.

Please review again, thanks!

[Mario Sanchez Prada]

Created attachment 325359 [details]
Patch proposal

Spotted a missing line and a couple of confusing comments, new patch attached

[Mario Sanchez Prada]

Created attachment 325431 [details]
Patch Proposal

Rebased patch on top of latest changes in trunk, which broke the WPE build with this fix landed (see bug 178964)

[Leo Ufimtsev]

We are in need of this api, looking forward to testing/using it.
(We = Eclipse's SWT on Linux, the Browser widget uses webkit2gtk underneath).

[Michael Catanzaro]

Comment on attachment 325431 [details]
Patch Proposal

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

Sorry for the delay getting a review.

On the one hand, I think this really should block on bug #175265, because I would hate to add new API that won't work properly in an important case. On the other hand... that bug is kinda stalled, and it would be nice to land this. So I'm going to say it looks good. r=me.

Technically, since you're a GTK reviewer yourself, you *could* land it with just my approval (as you are allowed to count yourself as the second API reviewer). But I'm sure Carlos Garcia will want to review the new API, so I'll leave the r? for him.

> Source/WebCore/platform/network/soup/CookieSoup.cpp:34
> +Cookie::Cookie(SoupCookie *cookie)

The * hangs on the type, not the variable name.

That should be the case in the header file as well, but I see the surrounding code does not respect our coding style in the header.

> Source/WebCore/platform/network/soup/CookieSoup.cpp:57
> +Cookie::operator SoupCookie *() const

This is too risky IMO. We can't have implicit conversion operators returning newly-allocated memory: that's a recipe for accidental leaks. You can achieve almost the same result by adding a static toSoupCookie() method, with the benefit that it has to be called explicitly.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:-303
> -static SoupCookie* toSoupCookie(const Cookie& cookie)

Oh... there was already a toSoupCookie method. So I would move this to Cookie.[cpp,h], where it can replace the implicit conversion.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:297
> -        soup_cookie_jar_add_cookie(cookieStorage(), toSoupCookie(cookie));
> +        soup_cookie_jar_add_cookie(cookieStorage(), cookie);

Yeah, this is a great example of why the implicit conversion is not a great idea. In this case, the code is correct, as you really do want to transfer ownership of the newly-constructed SoupCookie here. But that's a really hidden way of constructing a new object and transferring ownership. Developers are inevitably going to get confused and mess this up in the future. It will probably be me, if I'm the next person to touch it. ;)

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:326
> +    GUniquePtr<SoupCookie> targetCookie(cookie);
> +    GUniquePtr<GSList> cookiesList(soup_cookie_jar_get_cookie_list(cookieStorage(), uri.get(), TRUE));
> +    for (GSList* item = cookiesList.get(); item; item = g_slist_next(item)) {
> +        GUniquePtr<SoupCookie> soupCookie(static_cast<SoupCookie*>(item->data));
> +
> +        // Use (name, domain, path) as the primary key to identify the cookie.
> +        if (g_strcmp0(soup_cookie_get_name(soupCookie.get()), soup_cookie_get_name(targetCookie.get()))
> +            || g_strcmp0(soup_cookie_get_domain(soupCookie.get()), soup_cookie_get_domain(targetCookie.get()))
> +            || g_strcmp0(soup_cookie_get_path(soupCookie.get()), soup_cookie_get_path(targetCookie.get())))
> +            continue;
> +
> +        soup_cookie_jar_delete_cookie(cookieStorage(), soupCookie.get());
> +        break;

This is inefficient. Imagine deleting a list of cookies... it should be O(n) but now it's O(n^2) because you have to loop through roughly half the list each time. Fortunately, I don't think we need to store the SoupCookies in some other data structure to make this work, because soup_cookie_jar_delete_cookie() doesn't work by comparing pointer values, it works by checking the domain, name, value, and path of the cookie. So I think you don't need to get the SoupCookie from the cookie jar: you can just construct a new one yourself, and it should work. So you shouldn't need this loop at all.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:257
> +    Vector<WebCore::Cookie> cookies;
> +    cookies.append(WebCore::Cookie(cookie));

This is unused.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:265
> +    // of the process we access to them from, so just use the first process pool.

"we access them from"

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:271
> +            // This can only happen in cases where the web process is not available,
> +            // consider the operation "cancelled" from the point of view of the client.
> +            g_task_return_new_error(task.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled"));
> +            return;

Ah, maybe this really should block on bug #175265, so we don't have to add an unnecessary error here.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:387
> +    // We only care about the name, domain and path, which is what will be used
> +    // to identify the cookie in SoupCookieJar held by the other process. However,
> +    // we still need to create the Cookie with a non-empty value not to get discarded
> +    // on its way to the other process, so we just pass the name again there.

Good comment!

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:65
> +        , m_cookies(0)
> +        , m_cookieAdded(false)
>          , m_cookiesChanged(false)
> +        , m_cookieDeleted(false)

Use inline initialization for the new data members. Bonus points if you convert the entire initializer list to inline initialization. (It's project style to do small cleanups like that regularly in larger patches, rather than in separate patches as is common in GNOME.)

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:315
> +    // Still one cookie, since (name, domain, path) are the same than the already existing

"same as"

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:336
> +    // We have now 2 cookies that would apply to the passed URL, one is the cookie initially

Best to write out "two" in English (here and below)

[Mario Sanchez Prada]

Thanks for the review Michael. TL;DR: I agree with your comments and will adapt the code accordingly ASAP but, before that, I'd love to get a second review from Carlos since, as you suggest, he will probably want to do it anyway (and also because of some doubts with the implicit conversion).

More comments below...

(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 325431 [details]
> Patch Proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325431&action=review
> 
> Sorry for the delay getting a review.

No problem, it also took me some time to react to the first one from Adrián, so I won't be the one complaining about this taking too long :-)

> On the one hand, I think this really should block on bug #175265, because I
> would hate to add new API that won't work properly in an important case. On
> the other hand... that bug is kinda stalled, and it would be nice to land
> this. So I'm going to say it looks good. r=me.

Thanks, I personally think it's feasible to move on, but I see your point too.

> Technically, since you're a GTK reviewer yourself, you *could* land it with
> just my approval (as you are allowed to count yourself as the second API
> reviewer). But I'm sure Carlos Garcia will want to review the new API, so
> I'll leave the r? for him.

Even if I'm a WebKit reviewer, it's been a while (as you know) since I contributed regularly to the codebase and even back when I was more active the main area I reviewed patches about was the accessibility layer so, personally, I would love to have a second (or third) reviewer taking a look if possible. 

> > Source/WebCore/platform/network/soup/CookieSoup.cpp:34
> > +Cookie::Cookie(SoupCookie *cookie)
> 
> The * hangs on the type, not the variable name.

Oops! I'm surprised the check-webkit-style didn't catch that one, will fix it.

> That should be the case in the header file as well, but I see the
> surrounding code does not respect our coding style in the header.
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:57
> > +Cookie::operator SoupCookie *() const
> 
> This is too risky IMO. We can't have implicit conversion operators returning
> newly-allocated memory: that's a recipe for accidental leaks. You can
> achieve almost the same result by adding a static toSoupCookie() method,
> with the benefit that it has to be called explicitly.

This was actually a suggestion made by Carlos in the hackfest, although it's entirely possible I misunderstood him. That said, I don't mind changing the code as you suggest, but I think I'll wait for his review first, just in case we are missing something.

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:-303
> > -static SoupCookie* toSoupCookie(const Cookie& cookie)
> 
> Oh... there was already a toSoupCookie method. So I would move this to
> Cookie.[cpp,h], where it can replace the implicit conversion.
> 
> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:297
> > -        soup_cookie_jar_add_cookie(cookieStorage(), toSoupCookie(cookie));
> > +        soup_cookie_jar_add_cookie(cookieStorage(), cookie);
> 
> Yeah, this is a great example of why the implicit conversion is not a great
> idea. In this case, the code is correct, as you really do want to transfer
> ownership of the newly-constructed SoupCookie here. But that's a really
> hidden way of constructing a new object and transferring ownership.
> Developers are inevitably going to get confused and mess this up in the
> future. It will probably be me, if I'm the next person to touch it. ;)

I agree it's a bit obscure, yes. You'r convincing me by the moment!

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:326
> > +    GUniquePtr<SoupCookie> targetCookie(cookie);
> > +    GUniquePtr<GSList> cookiesList(soup_cookie_jar_get_cookie_list(cookieStorage(), uri.get(), TRUE));
> > +    for (GSList* item = cookiesList.get(); item; item = g_slist_next(item)) {
> > +        GUniquePtr<SoupCookie> soupCookie(static_cast<SoupCookie*>(item->data));
> > +
> > +        // Use (name, domain, path) as the primary key to identify the cookie.
> > +        if (g_strcmp0(soup_cookie_get_name(soupCookie.get()), soup_cookie_get_name(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_domain(soupCookie.get()), soup_cookie_get_domain(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_path(soupCookie.get()), soup_cookie_get_path(targetCookie.get())))
> > +            continue;
> > +
> > +        soup_cookie_jar_delete_cookie(cookieStorage(), soupCookie.get());
> > +        break;
> 
> This is inefficient. Imagine deleting a list of cookies... it should be O(n)
> but now it's O(n^2) because you have to loop through roughly half the list
> each time. Fortunately, I don't think we need to store the SoupCookies in
> some other data structure to make this work, because
> soup_cookie_jar_delete_cookie() doesn't work by comparing pointer values, it
> works by checking the domain, name, value, and path of the cookie. So I
> think you don't need to get the SoupCookie from the cookie jar: you can just
> construct a new one yourself, and it should work. So you shouldn't need this
> loop at all.

Ah! That's a good point, I was too lazy to check libsoup's code and didn't know that internally that's how _delete_cookie() worked. If that's the case, then I agree simply constructing a dummy SoupCookie with the right values for name, domain and path should be better.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:257
> > +    Vector<WebCore::Cookie> cookies;
> > +    cookies.append(WebCore::Cookie(cookie));
> 
> This is unused.

I can't believe this slipped (from a previous version of the patch). Will remove it.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:265
> > +    // of the process we access to them from, so just use the first process pool.
> 
> "we access them from"

Almost 5 years living in the UK for this (and it's not like it's the only offender)... :-) will fix it too.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:271
> > +            // This can only happen in cases where the web process is not available,
> > +            // consider the operation "cancelled" from the point of view of the client.
> > +            g_task_return_new_error(task.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled"));
> > +            return;
> 
> Ah, maybe this really should block on bug #175265, so we don't have to add
> an unnecessary error here.

Hmmm.. you sure? I feel like returning this error here, even it's kind of a generic catch-all kind of error is not a bad idea. Future is uncertain, as they say

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:387
> > +    // We only care about the name, domain and path, which is what will be used
> > +    // to identify the cookie in SoupCookieJar held by the other process. However,
> > +    // we still need to create the Cookie with a non-empty value not to get discarded
> > +    // on its way to the other process, so we just pass the name again there.
> 
> Good comment!

Thanks, my verbosity put towards a good cause to prevent confusion.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:65
> > +        , m_cookies(0)
> > +        , m_cookieAdded(false)
> >          , m_cookiesChanged(false)
> > +        , m_cookieDeleted(false)
> 
> Use inline initialization for the new data members. Bonus points if you
> convert the entire initializer list to inline initialization. (It's project
> style to do small cleanups like that regularly in larger patches, rather
> than in separate patches as is common in GNOME.)

Sure, but to avoid me misunderstanding things again... would you mind posting a pointer to some other test that's already looking the way you suggest?

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:315
> > +    // Still one cookie, since (name, domain, path) are the same than the already existing
> 
> "same as"

Almost five years in the UK... 5 years! (PS: I'll fix it)

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:336
> > +    // We have now 2 cookies that would apply to the passed URL, one is the cookie initially
> 
> Best to write out "two" in English (here and below)

Ok, will change it.

[Michael Catanzaro]

(In reply to Mario Sanchez Prada from comment #13)
> > > Source/WebCore/platform/network/soup/CookieSoup.cpp:57
> > > +Cookie::operator SoupCookie *() const
> > 
> > This is too risky IMO. We can't have implicit conversion operators returning
> > newly-allocated memory: that's a recipe for accidental leaks. You can
> > achieve almost the same result by adding a static toSoupCookie() method,
> > with the benefit that it has to be called explicitly.
> 
> This was actually a suggestion made by Carlos in the hackfest, although it's
> entirely possible I misunderstood him. That said, I don't mind changing the
> code as you suggest, but I think I'll wait for his review first, just in
> case we are missing something.

Sometimes what seems to be a good idea is not, in fact, a good idea. Consider a few of the many ways this can go badly:

void someFunction(SoupCookie* transferNone);
WebCore::Cookie cookie;
someFunction(&cookie); // hidden leak

or:

WebCore::Cookie cookie;
auto* cookie = static_cast<SoupCookie*>(&cookie); // hidden leak

In both cases, it's pretty surprising that an allocation occurs at all.

> I agree it's a bit obscure, yes. You'r convincing me by the moment!

Good. ;)
 
> > > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:271
> > > +            // This can only happen in cases where the web process is not available,
> > > +            // consider the operation "cancelled" from the point of view of the client.
> > > +            g_task_return_new_error(task.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled"));
> > > +            return;
> > 
> > Ah, maybe this really should block on bug #175265, so we don't have to add
> > an unnecessary error here.
> 
> Hmmm.. you sure? I feel like returning this error here, even it's kind of a
> generic catch-all kind of error is not a bad idea. Future is uncertain, as
> they say

Well, it's certainly a good idea to return the error until we have that bug fixed. Fixing that bug entails ensuring these functions work without a web process, so your patch actually complicates that work.
 
> > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:65
> > > +        , m_cookies(0)
> > > +        , m_cookieAdded(false)
> > >          , m_cookiesChanged(false)
> > > +        , m_cookieDeleted(false)
> > 
> > Use inline initialization for the new data members. Bonus points if you
> > convert the entire initializer list to inline initialization. (It's project
> > style to do small cleanups like that regularly in larger patches, rather
> > than in separate patches as is common in GNOME.)
> 
> Sure, but to avoid me misunderstanding things again... would you mind
> posting a pointer to some other test that's already looking the way you
> suggest?

Initialize them in the class declaration like this:

    WebKitCookieManager* m_cookieManager { nullptr };
    WebKitCookieAcceptPolicy m_acceptPolicy { WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY };
    char** m_domains { nullptr };
    bool m_cookiesChanged { false };
    bool m_finishLoopWhenCookiesChange { false };
    GUniquePtr<char> m_cookiesTextFile;
    GUniquePtr<char> m_cookiesSQLiteFile;

[Mario Sanchez Prada]

Created attachment 326469 [details]
Patch proposal

See attached a patch proposal where I think I've addressed Michael's concerns

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

(In reply to Mario Sanchez Prada from comment #15)
> Created attachment 326469 [details]
> Patch proposal
> 
> See attached a patch proposal where I think I've addressed Michael's concerns

Yes, thanks!

[Carlos Garcia Campos]

Comment on attachment 326469 [details]
Patch proposal

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

> Source/WebCore/platform/network/soup/CookieSoup.cpp:43
> +Cookie::Cookie(SoupCookie* cookie)
> +{
> +    name = String::fromUTF8(cookie->name);
> +    value = String::fromUTF8(cookie->value);
> +    domain = String::fromUTF8(cookie->domain);
> +    path = String::fromUTF8(cookie->path);
> +    expires = cookie->expires ? static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0;
> +    httpOnly = cookie->http_only;
> +    secure = cookie->secure;
> +    session = !cookie->expires;

Use the member initialization instead

Cookie::Cookie(SoupCookie* cookie)
    : name(String::fromUTF8(cookie->name))
    , value(String::fromUTF8(cookie->value))
....

> Source/WebCore/platform/network/soup/CookieSoup.cpp:60
> +    if (name.isNull() || value.isNull() || domain.isNull() || path.isNull())
> +        return nullptr;

Could we use isNull()?

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> + * Asynchronously add a #SoupCookie to the underlying storage, which is not persistent
> + * unless you explicitly call webkit_cookie_manager_set_persistent_storage().

Asynchronously add @cookie to @cookie_manager ? I don't think we need to clarify the persistance here

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:250
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:284
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:296
> + * @uri: the #SoupURI for the domain of the cookies to be retrieved

We don't expose SoupURI in our API, better use char* and create the soupURI internally if needed.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:302
> + * Asynchronously get a list of #SoupCookie from @cookie_manager associated with @uri, which
> + * must be either and HTTP or an HTTPS URL.

and -> an

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:306
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:327
> +        GList* cookies_list = nullptr;

cookies_list -> cookieList

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:346
> + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances
> + *    or %NULL in case of error.

If NULL means error, how can we return an empty list?

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:347
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:361
> + * @name: the name the cookie to be deleted
> + * @domain: the domain of the cookie to be deleted
> + * @path: the path of the cookie to be deleted

It's weird that this doesn't receive a SoupCookie, maybe we can document that name, domain and patch will be used to match the cookie.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:371
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:412
> + * Returns: %TRUE if the cookie was added or %FALSE in case of error.

added -> deleted

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:413
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/wpe/WebKitCookieManager.h:131
> +WEBKIT_API GList*

GList* -> GList *

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:122
> +        test->m_cookieAdded = added;

Does this make sense? we are asserting before if there's an error which is the only case where the cookie isn't added no?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:148
> +        g_list_free_full(m_cookies, reinterpret_cast<GDestroyNotify>(soup_cookie_free));

You should free the cookies in the destructor too.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:149
> +        m_cookies = 0;

nullptr

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:287
> +    test->setAcceptPolicy(WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY);

No third party is already the default.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:332
> +    g_assert_nonnull(foundCookies);
> +    g_assert_cmpint(g_list_length(foundCookies), ==, 2);

g_list_length returns 0 in case list is null, so we don't need the previous assert that isn't used in previous calls to getCookies() either.

[Michael Catanzaro]

Comment on attachment 326469 [details]
Patch proposal

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

> Source/WebCore/platform/Cookie.h:71
> +    Cookie(SoupCookie*);

Also, though I think this conversion is OK, it'd probably be less confusing to make it explicit. I usually make single-parameter constructors explicit by default.

[Mario Sanchez Prada]

Created attachment 327321 [details]
Patch proposal

Thanks for the review Carlos. I see you marked the patch as r+ but, since you suggested quite some changes and I'm adding one more on top (see the end of this comment), I'm attaching yet one more version of the patch where I addressed all your comments and I'm now asking for one more final review, hope you don't mind.

Now see my comments below...

(In reply to Carlos Garcia Campos from comment #18)
> Comment on attachment 326469 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326469&action=review
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:43
> > +Cookie::Cookie(SoupCookie* cookie)
> > +{
> > +    name = String::fromUTF8(cookie->name);
> > +    value = String::fromUTF8(cookie->value);
> > +    domain = String::fromUTF8(cookie->domain);
> > +    path = String::fromUTF8(cookie->path);
> > +    expires = cookie->expires ? static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0;
> > +    httpOnly = cookie->http_only;
> > +    secure = cookie->secure;
> > +    session = !cookie->expires;
> 
> Use the member initialization instead

Fixed.

> Cookie::Cookie(SoupCookie* cookie)
>     : name(String::fromUTF8(cookie->name))
>     , value(String::fromUTF8(cookie->value))
> ....
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:60
> > +    if (name.isNull() || value.isNull() || domain.isNull() || path.isNull())
> > +        return nullptr;
> 
> Could we use isNull()?

Not really, since Cookie::isNull() returns `false` only when *all* the internal attributes (name, domain, path, value, expired...) are nullptr/0/false, and in here I want to return nullptr if *either* of these four things are null, since otherwise I could not create a SoupCookie object.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> > + * Asynchronously add a #SoupCookie to the underlying storage, which is not persistent
> > + * unless you explicitly call webkit_cookie_manager_set_persistent_storage().
> 
> Asynchronously add @cookie to @cookie_manager ? I don't think we need to
> clarify the persistance here

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:250
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:284
> > + */
> 
> Since: 2.20

Fixed.
 
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:296
> > + * @uri: the #SoupURI for the domain of the cookies to be retrieved
> 
> We don't expose SoupURI in our API, better use char* and create the soupURI
> internally if needed.

Fixed.
 
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:302
> > + * Asynchronously get a list of #SoupCookie from @cookie_manager associated with @uri, which
> > + * must be either and HTTP or an HTTPS URL.
> 
> and -> an

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:306
> > + */
> 
> Since: 2.20

Fixed.
 
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:327
> > +        GList* cookies_list = nullptr;
> 
> cookies_list -> cookieList

Fixed.
 
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:346
> > + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances
> > + *    or %NULL in case of error.
> 
> If NULL means error, how can we return an empty list?

Oops! Documentation bug, fixed!

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:347
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:361
> > + * @name: the name the cookie to be deleted
> > + * @domain: the domain of the cookie to be deleted
> > + * @path: the path of the cookie to be deleted
> 
> It's weird that this doesn't receive a SoupCookie, maybe we can document
> that name, domain and patch will be used to match the cookie.

If you think that the triplet {name, domain, path} is what acts as a primary key to identify the cookie, then it's not that weird. Besides, it's particularly convenient since soup_cookie_new() doesn't allow you to pass a null value, which would force you to construct a SoupCookie with some fake/dummy value here just to delete it, which is quite weird IMHO.

For that reason, I think that simply passing the three strings that are needed to identify the cookie to be deleted is simpler, cleaner and more convenient, so I've re-written a bit the documentation to try to make it more clear now:

"""
 Asynchronously delete a #SoupCookie from the current session, that will be uniquely                                
 identified in the internal cookie jar by its name, domain and path.  
"""
  
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:371
> > + */
> 
> Since: 2.20

Fixed.
 
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:412
> > + * Returns: %TRUE if the cookie was added or %FALSE in case of error.
> 
> added -> deleted

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:413
> > + */
> 
> Since: 2.20

Fixed.
 
> > Source/WebKit/UIProcess/API/wpe/WebKitCookieManager.h:131
> > +WEBKIT_API GList*
> 
> GList* -> GList *

Fixed.
 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:122
> > +        test->m_cookieAdded = added;
> 
> Does this make sense? we are asserting before if there's an error which is
> the only case where the cookie isn't added no?

You're right, it's not needed. And the same applies to m_cookieDeleted. Fixed (by removing both things).
 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:148
> > +        g_list_free_full(m_cookies, reinterpret_cast<GDestroyNotify>(soup_cookie_free));
> 
> You should free the cookies in the destructor too.

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:149
> > +        m_cookies = 0;
> 
> nullptr

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:287
> > +    test->setAcceptPolicy(WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY);
> 
> No third party is already the default.

Fixed (by removing this line).
 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:332
> > +    g_assert_nonnull(foundCookies);
> > +    g_assert_cmpint(g_list_length(foundCookies), ==, 2);
> 
> g_list_length returns 0 in case list is null, so we don't need the previous
> assert that isn't used in previous calls to getCookies() either.

Fixed (by removing the unnecessary asserts).


(In reply to Michael Catanzaro from comment #19)
> Comment on attachment 326469 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326469&action=review
> 
> > Source/WebCore/platform/Cookie.h:71
> > +    Cookie(SoupCookie*);
> 
> Also, though I think this conversion is OK, it'd probably be less confusing
> to make it explicit. I usually make single-parameter constructors explicit
> by default.

Fixed.


Last, I realized (thanks to one of the unit tests I had added, that was failing!) that I needed to change the implementation of NetworkStorageSession::deleteCookie() back to how I had it in my initial proposal, before this review from Michael:

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:326
> > +    GUniquePtr<SoupCookie> targetCookie(cookie);
> > +    GUniquePtr<GSList> cookiesList(soup_cookie_jar_get_cookie_list(cookieStorage(), uri.get(), TRUE));
> > +    for (GSList* item = cookiesList.get(); item; item = g_slist_next(item)) {
> > +        GUniquePtr<SoupCookie> soupCookie(static_cast<SoupCookie*>(item->data));
> > +
> > +        // Use (name, domain, path) as the primary key to identify the cookie.
> > +        if (g_strcmp0(soup_cookie_get_name(soupCookie.get()), soup_cookie_get_name(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_domain(soupCookie.get()), soup_cookie_get_domain(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_path(soupCookie.get()), soup_cookie_get_path(targetCookie.get())))
> > +            continue;
> > +
> > +        soup_cookie_jar_delete_cookie(cookieStorage(), soupCookie.get());
> > +        break;
> 
> This is inefficient. Imagine deleting a list of cookies... it should be O(n)
> but now it's O(n^2) because you have to loop through roughly half the list
> each time. Fortunately, I don't think we need to store the SoupCookies in
> some other data structure to make this work, because
> soup_cookie_jar_delete_cookie() doesn't work by comparing pointer values, it
> works by checking the domain, name, value, and path of the cookie. So I
> think you don't need to get the SoupCookie from the cookie jar: you can just
> construct a new one yourself, and it should work. So you shouldn't need this
> loop at all.

Following this comment, I replaced all that code with a simple call to soup_cookie_jar_delete_cookie(targetCookie.get()), but the problem is that such a thing won't work because what  soup_cookie_jar_delete_cookie() does internally is to (1) retrieve a list of cookies for a given domain -out of a hash table indexed by domain- and then (2) iterate through that list using soup_cookie_equal() to decide which cookie to delete. And since soup_cookie_equal() uses **name, path & value** (curiously enough, it doesn't use the domain) to match the cookie in the jar with the one that needs to be deleted, this approach won't work with the API we are proposing because what we use to match the cookie is name, domain & path, but not the value (since we might not know it after all if all we want is to delete a cookie identifying it by what it constitutes the primary key).

So, this seems to me to be a good solution after all even if it's not ideal. I could create a hash table here to keep a parallel structure that we could check to use the actual SoupCookie we want to delete without having to do this loop but in my mind that's a bit overkill for a few reasons:
  1. It would require initializing that hash table when creating the cookie storage by adding every single cookie in the jar to it (which would consume memory), just in case we want to delete a cookie through this new API afterwards.
  2. It's not really true that this is O(n^2) where n == number of cookies stored: we call soup_cookie_jar_get_cookie_list() to retrieve a list of cookies *for the given domain*, and that's the list we iterate over, which doesn't necessarily have to be too big (or not as big as if it was iterating through ALL the cookies stored), so this is a fairly relative problem.
  3. Deleting a cookie is not a common use case (otherwise this API would have existed already), so I think I'd optimize for not adding extra artifacts here (such the proposed hash table) just to make this a bit more efficient at the expense of always using more memory, specially when the performance impact doesn't have to be that bad since it's O(n^2) where n == list of cookies for a given domain, not all of them.

So, for these reasons I simply brought the original code I had in place back for now, which gets the unit tests passing now again.

Now, another alternative I can think of that would remove this "burden" from WebKit2GTK itself (by requiring some extra work from the client) would be to actually have webkit_cookie_manager_delete_cookie() receive a SoupCookie* instead of the {name, domain, path} triplet. By doing that it would no longer be possible to conveniently remove a cookie by simple specifying those three values (ignoring the value), as it would require that the app first had retrieved the cookie that it wants to delete from the cookie storage. The advantage, however, would be that there would be no need to do this loop in NetworkStorageSessionSoup.cpp as simply calling soup_cookie_jar_delete_cookie() with the cookie passed would work, since this time soup_cookie_equal() would be able to match the right cookie as the value attribute would be set.

Carlos, Michael... what is your preference?:

  a. Keep the API as it is in the current patch (with webkit_cookie_manager_delete_cookie(name, domain, path)) + the funny loop in WebCore to match the cookie to be deleted
  b. Change the API so that webkit_cookie_manager_delete_cookie() receives a SoupCookie, simplifying the logic in WebKit + forcing apps to specify a full cookie (including "value") to delete it

I personally don't have any strong preference, as I think both could work fine, but I want to stress that I have no problem in changing from (a) to (b) if that's what looks better to you.

[Michael Catanzaro]

My slight preference would be (a) since it means less work for applications.

[Carlos Garcia Campos]

It's difficult to decide without knowing the main use case of this API. This is one of the reasons why we decided not to add new API to WebKit2 without a real use case, an application needing that new API and hopefully with a patch ready. If the normal use case is getting the list the list of cookies, store it somewhere and then delete a particular cookie, then the most convenient API is receiving a SoupCookie and of course, compare also de value. How is the user supposed to know the domain, name and path without getting the list of cookies before? In this use case I don't expect the user to create a cookie to delete, but use an existing one. If this is not the use case, then of course it's more convenient to pass the triplet, but I would like to understand the use case.

[Mario Sanchez Prada]

(In reply to Carlos Garcia Campos from comment #22)
> It's difficult to decide without knowing the main use case of this API. This
> is one of the reasons why we decided not to add new API to WebKit2 without a
> real use case, an application needing that new API and hopefully with a
> patch ready. If the normal use case is getting the list the list of cookies,
> store it somewhere and then delete a particular cookie, then the most
> convenient API is receiving a SoupCookie and of course, compare also de
> value. How is the user supposed to know the domain, name and path without
> getting the list of cookies before? In this use case I don't expect the user
> to create a cookie to delete, but use an existing one.

This is a really strong point and one that made me now lean towards (b) instead, since it's true it's hard to know domain, name and path for a cookie without having retrieved it first. Actually, domain and path you might be able to "guess/generate" them, but there's no way you can know the name to do the match without having retrieved the cookie first.

Besides, as Carlos pointed out to me on IRC, libsoup receives a full SoupCookie as a parameter to delete it from the jar, which is another hint on that perhaps that's the right thing to do.

> If this is not the
> use case, then of course it's more convenient to pass the triplet, but I
> would like to understand the use case.

You kind of convinced me that (b) is the way to go now, but before going all ahead and change the patch now I'll talk to Juan Pablo (or get him to reply here too, if possible) who is the person from Endless' apps team who came up with this requirements, so he might know better what would be best from the application's side.

I think, though, that the API he really needed was add() and get(), as we proposed delete() too just for the API to be complete IIRC, so we might not get more light on that from his side (in which case we might consider simply not adding it, although the API would be a bit unbalanced in that case).

Last, perhaps Leo Ufimtsev might have some opinion on the topic too, since he expressed his interest in this API as well in https://bugs.webkit.org/show_bug.cgi?id=177932#c11.

[Juan Pablo Ugarte]

(In reply to Mario Sanchez Prada from comment #23)
[...] 
> I think, though, that the API he really needed was add() and get(), as we
> proposed delete() too just for the API to be complete IIRC, so we might not
> get more light on that from his side (in which case we might consider simply
> not adding it, although the API would be a bit unbalanced in that case).

Right, my use case only require add() and get() but we tough it would be better to have delete() for API completeness.

I agree is better to maintain consistency with libsoup API and make delete() take a SoupCookie *, specially if its makes the implementation easier.

[Mario Sanchez Prada]

Created attachment 327360 [details]
Patch proposal

(In reply to Juan Pablo Ugarte from comment #24)
> (In reply to Mario Sanchez Prada from comment #23)
> [...] 
> > I think, though, that the API he really needed was add() and get(), as we
> > proposed delete() too just for the API to be complete IIRC, so we might not
> > get more light on that from his side (in which case we might consider simply
> > not adding it, although the API would be a bit unbalanced in that case).
> 
> Right, my use case only require add() and get() but we tough it would be
> better to have delete() for API completeness.
> 
> I agree is better to maintain consistency with libsoup API and make delete()
> take a SoupCookie *, specially if its makes the implementation easier.

Thanks. So, considering that Michael said he _slightly_ preferred (a) if that means less work for apps but that's unclear to be the case since you need the name anyway, I think we might be all onboard then with (b), so here is a new patch. Hopefully the last one!

Please review, thanks!

[Michael Catanzaro]

Yeah, you've convinced me that (b) is best.

[Carlos Garcia Campos]

Comment on attachment 327360 [details]
Patch proposal

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

> Source/WebCore/platform/network/soup/CookieSoup.cpp:39
> +    , expires(0)

Would it be possible to initialize this here?

, expires(cookie->expires ? static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0)

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:350
> + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances.

It's not a GSList, but a GList. Even if transfer full is used, I would still clarify that user should free the list and its contents.

[Mario Sanchez Prada]

(In reply to Carlos Garcia Campos from comment #27)
> Comment on attachment 327360 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327360&action=review
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:39
> > +    , expires(0)
> 
> Would it be possible to initialize this here?
> 
> , expires(cookie->expires ?
> static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0)

Sure, actually that was my first approach but I changed it because I was unsure that was the right style. Will change it back before landing.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:350
> > + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances.
> 
> It's not a GSList, but a GList. Even if transfer full is used, I would still
> clarify that user should free the list and its contents.

Right, I'll fix that too before landing too.

Thanks!

[Mario Sanchez Prada]

Committed r225054: <https://trac.webkit.org/changeset/225054>