<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#c7">Comment # 7</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: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#c6">comment #6</a>)
<span class="quote">> 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.</span >
Thanks for looking at it.
<span class="quote">> 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 >
Memory cache is not the page cache is the memory cache, used to store any cacheable resources in memory, and disk cache is the HTTP disk cache used by the networking process.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:30
> > + * @Short_description: A security boundary for Websites
>
> I would rather say "websites" in lowercase.</span >
Ok.
<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
>
> Same.</span >
Ok.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:55
> > + CString host;
>
> 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 >
This is a common thing we do in the API just to make it more convenient to use. The host and protocol are stored in WebCore::SecurityOrigin as String, but all our APIs return always string in utf8 (like all other GNOME platform APIs). If we return the converted string directly, the returned value should be freed by the user. So, we cache the converted strings to be able to return a const char *, and we do that only on demand, so if you never call get_host we don't have a cached value for nothing.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:63
> > + WebKitSecurityOrigin* origin = g_slice_new(WebKitSecurityOrigin);
>
> 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/">https://git.gnome.org/browse/gnome-devel-docs/tree/programming-guidelines/C/</a>
> memory-management.page#n22</span >
Good point, didn't know that.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:77
> > + ASSERT(origin);
>
> 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 >
WebKitSecurityOrigin is a boxed type not a C++ class, we always use pointers with GObjects or boxed types. That ASSERT is equivalent to the g_return macros used in public methods, but in this case I used ASSERT because it's a private method.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:166
> > + * Returns: The host of the #WebKitSecurityOrigin.
>
> This needs a (nullable) annotation, or (allow-none), since you decided to
> return nullptr if the string is empty.</span >
I thought nullable/allow-none was assumed for char *, I think we don't use that in any of our APIs, we will have to update them all.
<span class="quote">> Did you see
> <a href="https://blogs.gnome.org/desrt/2014/05/27/allow-none-is-dead-long-live">https://blogs.gnome.org/desrt/2014/05/27/allow-none-is-dead-long-live</a>-
> nullable/ and continue to use (allow-none) for compatibility with old
> gobject-introspection?</span >
No.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:512
> > + origins = g_list_prepend(origins, webkitSecurityOriginCreate(origin.get()));
>
> And the same origin won't ever be in multiple records? (Same question
> applies to the case of the memory cache.)</span >
That could probably happen when you call fetchData with more than one type, but we always use fetchData with a single type.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:524
> > + * @error: return location for error or %NULL to ignore
>
> Shouldn't this be annotated (optional) or (allow-none)? Same for all the
> other error parameters in this file.</span >
I thought that GError ** was handled automatically too.
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:651
> > + * %WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS process model, an empty list will
>
> 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 >
I don't mind to use ;
<span class="quote">> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.h:94
> > + GError ** error);
>
> Extra space after the **</span >
Good catch.
<span class="quote">> > Tools/ChangeLog:9
> > + MiniBrowsore to show and handle Website data.
>
> Is MiniBrowsore a typo or a pun? I can't tell. :D</span >
:-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>