[webkit-reviews] review granted: [Bug 167941] [WPE][GTK] Enable support for CONTENT_EXTENSIONS : [Attachment 362081] Patch v9

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 15 02:28:30 PST 2019


Carlos Garcia Campos <cgarcia at igalia.com> has granted Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 167941: [WPE][GTK] Enable support for CONTENT_EXTENSIONS
https://bugs.webkit.org/show_bug.cgi?id=167941

Attachment 362081: Patch v9

https://bugs.webkit.org/attachment.cgi?id=362081&action=review




--- Comment #76 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 362081
  --> https://bugs.webkit.org/attachment.cgi?id=362081
Patch v9

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

cq- because we still need more unit tests

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:95
> +    WebKitUserContentFilterStore* store =
WEBKIT_USER_CONTENT_FILTER_STORE(g_object_new(WEBKIT_TYPE_USER_CONTENT_FILTER_S
TORE, nullptr));
> +    store->priv->store = adoptRef(new
API::ContentRuleListStore(FileSystem::stringFromFileSystemRepresentation(storag
ePath), false));

Sorry that I didn't notice this before, but we don't recommend to do this in
public new methods, because it doesn't work for bindings that use g_object_new
directly, instead of the new() public method. You should make the storage path
a read-only, construct-only property and create the API::ContentRuleListStore
in constructed().

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:220
> +    g_file_load_contents_async(file, cancellable, [](GObject* sourceObject,
GAsyncResult* result, void* userData) {

When I suggested to limit this to local files was because we couldn't use
load_bytes() and fallback code might be to complex, but since
g_file_load_contents_async() can handle any GFile I don't see any reason to
limit it local files. I'm sorry I didn't make it clearer in my previous review.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:228
> +

Remove this empty line.


More information about the webkit-reviews mailing list