[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 #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 257556
  --> https://bugs.webkit.org/attachment.cgi?id=257556
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257556&action=review

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:30
> + * @Short_description: A security boundary for Websites

I would rather say "websites" in lowercase.

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

Same.

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

> 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

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

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

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?

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

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

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

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

Extra space after the **

> Tools/ChangeLog:9
> +        MiniBrowsore to show and handle Website data.

Is MiniBrowsore a typo or a pun? I can't tell. :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/94d39c52/attachment.html>