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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177932

Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #323310|0                           |1
        is obsolete|                            |
 Attachment #323310|review?                     |
              Flags|                            |
 Attachment #325357|                            |review?
              Flags|                            |

--- Comment #8 from Mario Sanchez Prada <mario at webkit.org> ---
Created attachment 325357

  --> https://bugs.webkit.org/attachment.cgi?id=325357&action=review

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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171030/eecd7182/attachment-0001.html>