[Webkit-unassigned] [Bug 146589] [GTK] Add API to WebKitWebsiteDataManager to handle website data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=146589

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #299429|review?                     |review+
              Flags|                            |

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

> 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.

> 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.

> 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

> 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"?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:176
> +            websiteData->displayName = _("Local files");

You forgot to update POTFILES.in.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:188
> + * types actually found, not the types queried with webkit_website_data_manager_fetch().

"actually present"?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:521
> + * @user_data: (closure): the data to pass to callback function

to the callback function

> 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)

> 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"

> 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?

> 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.)

-- 
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/f31f02a3/attachment.html>