[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

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