[Webkit-unassigned] [Bug 177932] [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 8 07:05:25 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=177932
--- Comment #13 from Mario Sanchez Prada <mario at webkit.org> ---
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.
--
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/20171108/c1227373/attachment-0001.html>
More information about the webkit-unassigned
mailing list