[Webkit-unassigned] [Bug 146589] [GTK] Add API to WebKitWebsiteDataManager to handle cached data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=146589

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #6)
> Comment on attachment 257556 [details]
> Rebased patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257556&action=review
> 
> Unofficial +1 on the API changes from me.

Thanks for looking at it.

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

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.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:30
> > + * @Short_description: A security boundary for Websites
> 
> I would rather say "websites" in lowercase.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:35
> > + * by Websites. An origin consists of a host name, a protocol, and a port
> 
> Same.

Ok.

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

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.

> > 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:
> https://git.gnome.org/browse/gnome-devel-docs/tree/programming-guidelines/C/
> memory-management.page#n22

Good point, didn't know that.

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

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.

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

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.

> Did you see
> https://blogs.gnome.org/desrt/2014/05/27/allow-none-is-dead-long-live-
> nullable/ and continue to use (allow-none) for compatibility with old
> gobject-introspection?

No.

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

That could probably happen when you call fetchData with more than one type, but we always use fetchData with a single type. 

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

I thought that GError ** was handled automatically too.

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

I don't mind to use ;

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.h:94
> > +                                                                       GError                  ** error);
> 
> Extra space after the **

Good catch.

> > Tools/ChangeLog:9
> > +        MiniBrowsore to show and handle Website data.
> 
> Is MiniBrowsore a typo or a pun? I can't tell. :D

:-D

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:736
> > +    test->wait(1);
> 
> :(

-- 
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/20150728/b0d3d1a4/attachment.html>