<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #299429 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c19">Comment # 19</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:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>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 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.</span >
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 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().</span >
ge -> get
<span class="quote">> 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</span >
Leave a blank line between these paragraphs.
<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.</span >
Change the comma to a semicolon
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:175
> + if (websiteData->record.displayName == "Local documents on your computer")</span >
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 class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:176
> + websiteData->displayName = _("Local files");</span >
You forgot to update POTFILES.in.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteData.cpp:188
> + * types actually found, not the types queried with webkit_website_data_manager_fetch().</span >
"actually present"?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:521
> + * @user_data: (closure): the data to pass to callback function</span >
to the callback function
<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.</span >
sites -> websites (this is the only place you wrote "sites" and it's nice to use consistent terminology)
<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.</span >
This is a bit confusing. Let's say: "modified in the past @timespam microseconds"
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataPrivate.h:2
> + * Copyright (C) 2016 Igalia S.L.</span >
Happy new years!
<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));</span >
Why reloading?
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebsiteData.cpp:282
> + GList removeList = { itemList->data, nullptr, nullptr };</span >
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.)</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>