<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add API to WebKitWebsiteDataManager to handle website data"
href="https://bugs.webkit.org/show_bug.cgi?id=146589#c21">Comment # 21</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add API to WebKitWebsiteDataManager to handle website data"
href="https://bugs.webkit.org/show_bug.cgi?id=146589">bug 146589</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=146589#c19">comment #19</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=299429&action=diff" name="attach_299429" title="New patch">attachment 299429</a> <a href="attachment.cgi?id=299429&action=edit" title="New patch">[details]</a></span>
> New patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=299429&action=review">https://bugs.webkit.org/attachment.cgi?id=299429&action=review</a>
>
> 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?</span >
I thought about that but sounded redundant to me, this is data manager.
<span class="quote">> > 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.</span >
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.
<span class="quote">> > 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.</span >
Ok.
<span class="quote">> > 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</span >
Ok.
<span class="quote">> > 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"?</span >
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.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:176
> > + websiteData->displayName = _("Local files");
>
> You forgot to update POTFILES.in.</span >
As always :-P
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:188
> > + * types actually found, not the types queried with webkit_website_data_manager_fetch().
>
> "actually present"?</span >
Sure.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:521
> > + * @user_data: (closure): the data to pass to callback function
>
> to the callback function</span >
Ok.
<span class="quote">> > 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)</span >
Agree.
<span class="quote">> > 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"</span >
I avoided using microseconds detail, it's a GTimeSpan and not a guint64 for that reason.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataPrivate.h:2
> > + * Copyright (C) 2016 Igalia S.L.
>
> Happy new years!</span >
:-)
<span class="quote">> > 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?</span >
To update the data and check that whatever you removed is no longer listed.
<span class="quote">> > 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.)</span >
Then you would be heap allocating just to store a single item list. This is a trick for single item lists.</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>