[Webkit-unassigned] [Bug 168230] [GTK] Update cookie manager API to properly work with ephemeral sessions
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 13 17:59:45 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=168230
Michael Catanzaro <mcatanzaro at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #301345|review? |review-
Flags| |
--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 301345
--> https://bugs.webkit.org/attachment.cgi?id=301345
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=301345&action=review
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().
> 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.
> 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.
> 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.
> 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?
> 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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170214/434fc9f1/attachment-0001.html>
More information about the webkit-unassigned
mailing list