[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

--- Comment #12 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 325431
  --> https://bugs.webkit.org/attachment.cgi?id=325431
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)

-- 
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/20171107/ef68e892/attachment-0001.html>