[webkit-reviews] review requested: [Bug 177932] [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager : [Attachment 325357] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  for review:
Bug 177932: [GTK] New API to add, retrieve and delete cookies via
WebKitCookieManager
https://bugs.webkit.org/show_bug.cgi?id=177932

Attachment 325357: Patch proposal

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




--- 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!