[webkit-reviews] review denied: [Bug 167941] [WPE][GTK] Enable support for CONTENT_EXTENSIONS : [Attachment 361851] Patch v7

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied 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 361851: Patch v7

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




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

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

We really need unit tests

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:54
> + */

Since: 2.24

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:72
> +static void
webkit_content_filter_manager_class_init(WebKitContentFilterManagerClass*
klass)

Remove klass

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:78
> + * @storage_path: path where data for compiled filters will be stored on
disk

(nullable) and add , or %NULL and document here or bellow in the body that
passing %NULL means using using the default store path. I would make this
mandatory, though.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:89
> +    manager->priv->store = path ? adoptRef(new
API::ContentRuleListStore(String::fromUTF8(path), false)) :
&API::ContentRuleListStore::defaultStore(false);

The default store depends on ContentRuleListStore::defaultStorePath() to be
implement but it isn't for GLib based ports. Use
FileSystem::stringFromFileSystemRepresentation() instead of String::fromUTF8().

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:97
> + * @json_source: a string containing the rule seet in JSON format

I would just use @source

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:120
> +    GRefPtr<GBytes> bytes = adoptGRef(g_bytes_new_static(jsonSource,
strlen(jsonSource)));
> +    webkit_content_filter_manager_compile(manager, identifier, bytes.get(),
cancellable, callback, userData);

I think it's easier enough for the caller to create a GBytes from a string to
not need to provide this API.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:144
> + * webkit_content_filter_manager_compile:

I think I would use add instead of compile. The fact that the source is
compiled is an implementation detail. Since we have remove and lookup, add
sounds better to me.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:153
> + * [WebKit content extesions JSON
format](https://webkit.org/blog/3476/content-blockers-first-look/).

WE should add a chapter in the docs to document the JSON format instead of
including a link to a blog post. I would be annoying, for example if you are
reading the docs in devhelp. I'm fine to do it in a follow up patch, but before
2.24 is out, please.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:173
> +    auto jsonString = String::fromUTF8(static_cast<const
char*>(g_bytes_get_data(jsonSource, nullptr)) ? : "",
g_bytes_get_size(jsonSource));

I think this would be easier to read if we first get the data and size with
g_bytes_get_data. We don't need to null-check the result, String accepts and
handles null. Another thing is, do we want to allow passing an empty source?

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:208
> + * @identifier: a filter identifier

Since add returns the WebKitUserContentFilter, I wonder if we should receive
the filter here instead of the id.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:291
> +	       g_task_return_pointer(task.get(),
webkitUserContentFilterFromRuleList(WTFMove(contentRuleList)),
reinterpret_cast<GDestroyNotify>(webkit_user_content_filter_unref));

This is weird. The method is called lookup(), but the pointer won't be the same
as the one returned by compile(). We should document it and probably rename
this method.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:308
> +WebKitUserContentFilter*
webkit_content_filter_manager_lookup_finish(WebKitContentFilterManager*
manager, GAsyncResult* result, GError** error)

Transfer full is also weird for a lookup().

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:343
> +	   result[identifiers.size()] = nullptr;

This is already nullptr, since you used g_new0.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:376
> + * @user_content_filter.

What's this?

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:386
> +    return userContentFilter->contentRuleList->name().utf8().data();

You can't do this, data() is a temporary. We should either cache the id in
WebKitUserContentFilter struct and return a const char* or use g_strdup() here
and return a newly allocated string. The former is usually preferred.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:322
> + * webkit_user_content_manager_add_filter:

Now I see why using add would be even more confusing in
WebKitContentFilterManager. I think we should make it clearer that
WebKitContentFilterManager is about storing in disk. Maybe Manager is not the
best word in this case and Store would fit better. If we keep the Manager name
then I would use store() instead of add() or compile() and retrieve() instead
of lookup(). I would also consider using
WebKitUserContentFilter{Manmager|Store} since it creates
WebKitUserContentFilters, it looks more consistent.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:331
> + * Filters need to be compiled from source or loaded from disk using a
> + * #WebKitContentFilterManager.

Since this receives a WebKitUserContentFilter object, there's no other way to
create such an object, no?

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:342
> +void webkit_user_content_manager_remove_filter(WebKitUserContentManager*
manager, WebKitUserContentFilter* filter)

This needs to be documented. I wonder why the build doesn't fail.

> Source/WebKit/UIProcess/API/gtk/WebKitError.h:147
> + * @WEBKIT_CONTENT_FILTER_ERROR_INVALID_JSON: The JSON source for a content
filter is invalid.

I would use INVALID_SOURCE

> Tools/MiniBrowser/gtk/main.c:116
> +    { "content-filter", 0, 0, G_OPTION_ARG_STRING, &contentFilter, "JSON
with content filtering rules", "FILE" },

G_OPTION_ARG_FILENAME

> Tools/MiniBrowser/gtk/main.c:568
> +	   if (!g_file_get_contents(contentFilter, &contentFilterJSON, NULL,
&error)) {

Use g_file_new_for_commandline_arg(), so that the passed file can be a relative
path. Then you could get the GBytes directly with g_file_read +
g_input_stream_read_bytes (g_file_load_bytes() would be better but requires
newer glib).

> Tools/MiniBrowser/gtk/main.c:571
> +	       return 1;

Do we want to make this fatal?

> Tools/MiniBrowser/gtk/main.c:575
> +	   filterManager = webkit_content_filter_manager_new(filtersPath);

This is leaked.

> Tools/MiniBrowser/gtk/main.c:584
> +	   compilationData.mainLoop = g_main_loop_new(NULL, FALSE);
> +	   g_main_loop_run(compilationData.mainLoop);
> +	   g_main_loop_unref(compilationData.mainLoop);
> +	   g_object_unref(filterManager);

Why are we doing this synchronously? If this is really useful we should provide
_sync() methods too.

> Tools/MiniBrowser/gtk/main.c:589
> +	       return 1;

Do we want to make this fatal?