<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 cached data"
href="https://bugs.webkit.org/show_bug.cgi?id=146589#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add API to WebKitWebsiteDataManager to handle cached 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=257556&action=diff" name="attach_257556" title="Rebased patch">attachment 257556</a> <a href="attachment.cgi?id=257556&action=edit" title="Rebased patch">[details]</a></span>
Rebased patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=257556&action=review">https://bugs.webkit.org/attachment.cgi?id=257556&action=review</a>
Unofficial +1 on the API changes from me.
I think it would be helpful to add to the documentation of WebKitWebsiteDataManager a short description of the difference between the memory cache and the disk cache. (I presume the memory cache is the page cache, and the disk cache is the network cache?)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:30
> + * @Short_description: A security boundary for Websites</span >
I would rather say "websites" in lowercase.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:35
> + * by Websites. An origin consists of a host name, a protocol, and a port</span >
Same.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:55
> + CString host;</span >
I don't understand the value of duplicating protocol and host here, when they are already stored in the WebCore::SecurityOrigin. It's also a bit confusing that these are initialized in getters.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:63
> + WebKitSecurityOrigin* origin = g_slice_new(WebKitSecurityOrigin);</span >
I think we should stop using GSlice and just use plain new/delete instead. I base this off of a comment in the GNOME memory management programming guide: "we don’t want to encourage [use of g_slice_new]. GLib developers are moving towards deprecating it due to its performance slipping significantly behind the libc allocator’s on Linux in recent years." That is literally a comment, as in a commented-out section of the guide, so it doesn't show up on the actual website, but it's there to discourage people from adding guidance on GSlice: <a href="https://git.gnome.org/browse/gnome-devel-docs/tree/programming-guidelines/C/memory-management.page#n22">https://git.gnome.org/browse/gnome-devel-docs/tree/programming-guidelines/C/memory-management.page#n22</a>
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:77
> + ASSERT(origin);</span >
When you would write an assertion like this, I think it's better to pass by reference instead. I think we've been trying to avoid pass by pointer except in C API and when the pointer is expected to be null.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:166
> + * Returns: The host of the #WebKitSecurityOrigin.</span >
This needs a (nullable) annotation, or (allow-none), since you decided to return nullptr if the string is empty.
Did you see <a href="https://blogs.gnome.org/desrt/2014/05/27/allow-none-is-dead-long-live-nullable/">https://blogs.gnome.org/desrt/2014/05/27/allow-none-is-dead-long-live-nullable/</a> and continue to use (allow-none) for compatibility with old gobject-introspection?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:512
> + origins = g_list_prepend(origins, webkitSecurityOriginCreate(origin.get()));</span >
And the same origin won't ever be in multiple records? (Same question applies to the case of the memory cache.)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:524
> + * @error: return location for error or %NULL to ignore</span >
Shouldn't this be annotated (optional) or (allow-none)? Same for all the other error parameters in this file.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:651
> + * %WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS process model, an empty list will</span >
This is a comma splice; you can fix it by changing the comma to a semicolon, or by splitting it into two sentences if you don't like semicolons.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.h:94
> + GError ** error);</span >
Extra space after the **
<span class="quote">> Tools/ChangeLog:9
> + MiniBrowsore to show and handle Website data.</span >
Is MiniBrowsore a typo or a pun? I can't tell. :D
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:736
> + test->wait(1);</span >
:(</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>