<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Update cookie manager API to properly work with ephemeral sessions"
href="https://bugs.webkit.org/show_bug.cgi?id=168230#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Update cookie manager API to properly work with ephemeral sessions"
href="https://bugs.webkit.org/show_bug.cgi?id=168230">bug 168230</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=168230#c2">comment #2</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=301345&action=diff" name="attach_301345" title="Patch">attachment 301345</a> <a href="attachment.cgi?id=301345&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=301345&action=review">https://bugs.webkit.org/attachment.cgi?id=301345&action=review</a>
>
> It looks mostly good, but I'd like to review this one again after you answer
> my questions about why you used the weak pointer, which I hope was just a
> mistake.
>
> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:217
> > + // FIXME: Add support for deleting cookies modified since the given timestamp. It should probably be added to libsoup.
> > + if (timestamp == std::chrono::system_clock::from_time_t(0))
> > + deleteAllCookies(session);
>
> Are you planning to fix this soon? I don't think we should proceed with
> these API changes if the API does not work properly. At least add a runtime
> warning using g_warning().</span >
No, I don't have time, I'll add the warning.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:40
> > + * The WebKitCookieManager defines how to setup and handle cookies.
>
> setup (noun) -> set up (verb)
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:43
> > * store cookies, with webkit_cookie_manager_set_persistent_storage(),
>
> Remove the comma
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:144
> > + g_object_add_weak_pointer(G_OBJECT(dataManager), reinterpret_cast<void**>(&manager->priv->dataManager));
>
> Why is the weak pointer needed? I would expect WebKitCookieManager to be
> owned by the WebKitWebsiteDataManager, so WebKitWebsiteDataManager would
> keep a ref to ensure the WebKitCookieManager is never destroyed first. Is
> there some really good reason you don't do that? I think it should be a
> normal raw pointer instead.</span >
The cookie manager can't be created by the user, it's always created and owned by the website data manager. However, nothing prevents the user from taking a ref to the returned cookie manager. It's a programmer error to use the cookie manager after the data manager is destroyed, that's why all methods are protected with g_return_if_fail(WEBKIT_IS_WEBSITE_DATA_MANAGER(manager->priv->dataManager)). I don't want to take a reference to the data manager. Maybe it isn't worth it just to handle a corner case, so I can simply remove the weak point and use a plan pointer. Users will get a crash sooner or layer in both cases I'm afraid.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:165
> > * By default, @cookie_manager doesn't store the cookies persistenly, so you need to call this
>
> Preexisting problem, but let's fix it now: persistenly -> persistently
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:176
> > + for (auto* processPool : webkitWebsiteDataManagerGetProcessPools(manager->priv->dataManager)) {
>
> This is a big problem. Either manager->priv->dataManager can be legitimately
> null here, or you should not have used a weak pointer.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:197
> > + g_return_if_fail(WEBKIT_IS_WEBSITE_DATA_MANAGER(manager->priv->dataManager));
>
> Ditto. If you're going to use the weak pointer, then the entire class needs
> to be prepared to deal with the WebKitWebsiteDataManager being null. There's
> no point to the weak pointer otherwise.</span >
It deals with that already, but using g_return macros.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:82
> > * webkit_web_context_get_security_manager() for that.
>
> Remove "for that": the sentence works better without them.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:698
> > WebKitCookieManager* webkit_web_context_get_cookie_manager(WebKitWebContext* context)
>
> I think you should document that this function returns the
> WebKitCookieManager of this context's WebKitWebsiteDataManager. I know it's
> an implementation detail, but otherwise I think it's kind of confusing why
> both WebKitWebContext and WebKitWebsiteDataManager have a
> WebKitCookieManager.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2286
> > + * Get the #WebKitWebsiteDataManager associated to @web_view. If @web_view is not ephemeral
>
> Add a comma after "ephemeral"
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2287
> > + * the returned #WebKitWebsiteDataManager will be the same as the #WebKitWebView:web-context one.
>
> "the #WebKitWebView:web-context one" doesn't sound great. I would write:
> "will be the same as the #WebKitWebsiteDataManager of @web_view's
> #WebKitWebContext."
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:103
> > + GRefPtr<WebKitCookieManager> cookieManager;
>
> Aha, so you do keep a ref! Good! Then you don't need the weak pointer in
> WebKitCookieManager, right? Surely the WebKitCookieManager cannot outlive
> the WebKitWebsiteDataManager due to this ref.</span >
Not surely, but I guess we should assume it, yes.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:104
> > + Vector<WebProcessPool*> processPools;
>
> Is it possible to use Vector<WebProcessPool&> to clarify that the process
> pools are not null?</span >
No, I don't think Vector can contain references.
<span class="quote">> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataRecord.cpp:54
> > +#if PLATFORM(GTK)
> > + if (hostName == "localhost")
> > + return hostName;
> > +#endif
>
> I think this should be an #else condition, not a GTK-specific condition.</span ></pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>