[Webkit-unassigned] [Bug 146589] [GTK] Add API to WebKitWebsiteDataManager to handle website data
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 22 23:17:03 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=146589
--- Comment #21 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #19)
> Comment on attachment 299429 [details]
> New patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299429&action=review
>
> I like the API.
>
> I would slightly prefer slightly longer but more specific function names:
> webkit_website_data_manager_fetch_data, _remove_data, _clear_data. What do
> you think about that?
I thought about that but sounded redundant to me, this is data manager.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:38
> > + * A website is normally a set of URLs grouped by domain name using the public suffix list.
>
> Why is the public suffix list used? Keeping in mind that libsoup's public
> suffix list is often years out of date and that we should almost never be
> using it. It's my primary concern with this patch.
Because that's how the internal WebKit2 API works. My guess is that you normally want to remove the data for a website, no matter how many url forms that website has. It makes everything easier to handle.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:39
> > + * You can ge the website name, which is usually the domain, with webkit_website_data_get_name().
>
> ge -> get
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:42
> > + * A website can store different types of data in the client side. #WebKitWebsiteDataTypes is an enum containing
>
> Leave a blank line between these paragraphs.
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:43
> > + * all the possible data types, use webkit_website_data_get_types() to get the bitmask of data types.
>
> Change the comma to a semicolon
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:175
> > + if (websiteData->record.displayName == "Local documents on your computer")
>
> That's way too fragile; it's going to break whenever someone decides to
> change the display name. There has got to be a better way to check if the
> record represents a local file. How about checking if the protocol of the
> security origin is "file"?
Yes, I agree, I did it this way to avoid a GTK ifdef in displayNameForLocalFiles(). Fortunately unit tests will catch it if it's changed.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:176
> > + websiteData->displayName = _("Local files");
>
> You forgot to update POTFILES.in.
As always :-P
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:188
> > + * types actually found, not the types queried with webkit_website_data_manager_fetch().
>
> "actually present"?
Sure.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:521
> > + * @user_data: (closure): the data to pass to callback function
>
> to the callback function
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:579
> > + * Use webkit_website_data_manager_clear() if you want to remove the website data for all sites.
>
> sites -> websites (this is the only place you wrote "sites" and it's nice to
> use consistent terminology)
Agree.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:639
> > + * Asynchronously clear the website data of the given @types modified since the given @timespan.
>
> This is a bit confusing. Let's say: "modified in the past @timespam
> microseconds"
I avoided using microseconds detail, it's a GTimeSpan and not a guint64 for that reason.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataPrivate.h:2
> > + * Copyright (C) 2016 Igalia S.L.
>
> Happy new years!
:-)
> > Tools/MiniBrowser/gtk/main.c:272
> > + if (webkit_website_data_manager_remove_finish(manager, result, NULL))
> > + webkit_web_view_reload(webkit_uri_scheme_request_get_web_view(dataRequest->request));
>
> Why reloading?
To update the data and check that whatever you removed is no longer listed.
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebsiteData.cpp:282
> > + GList removeList = { itemList->data, nullptr, nullptr };
>
> I think GList removeList = g_list_prepend(nullptr, data) would be a much
> clearer way to write this. You might recall teaching me that recently. ;)
> (Note it's duplicated in several places below.)
Then you would be heap allocating just to store a single item list. This is a trick for single item lists.
--
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/20170123/c929787d/attachment-0001.html>
More information about the webkit-unassigned
mailing list