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

Thu Feb 14 01:56:45 PST 2019

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 361981: Patch v8

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

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

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

API looks good to me now, but we need more unit tests.

> Source/WTF/wtf/glib/GRefPtr.h:243
> +template <> WTF_EXPORT_PRIVATE GMappedFile* refGPtr(GMappedFile*);
> +template <> WTF_EXPORT_PRIVATE void derefGPtr(GMappedFile*);

This requires a changelog entry in WTF.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:324
> +	   : identifier(contentRuleList->name().utf8()),
contentRuleList(contentRuleList), referenceCount(1)

Use a new line for every parameter. The contentRuleList should be moved with
WTFMove()

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:375
> + * Obtain the identifier passed to webkit_content_filter_manager_compile()

webkit_content_filter_manager_compile no longer exists. I would avoid using
function names here, because we might add more. I would save the identifier
used to save a filter in a #WebKitUserContentFilterStore, or something similar.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:383
> +const char*
> +webkit_user_content_filter_get_identifier(WebKitUserContentFilter*
userContentFilter)

This should be in the same line.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:389
> +WebKitUserContentFilter*
webkitUserContentFilterFromRuleList(RefPtr<API::ContentRuleList>&&
contentRuleList)

We normally use Create for private new functiuons like this.
webkitUserContentFilterCreate() in this case.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:63
> +static inline GError*
> +toGError(WebKitUserContentFilterError code, const std::error_code error)

One line here.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:85
> + * The path must point to a local filesystem.

And should exist, right?

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:93
> +    g_return_val_if_fail(storagePath != nullptr, nullptr);

Don't compare to nullptr

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:95
> +    store->priv->store = adoptRef(new
API::ContentRuleListStore(String::fromUTF8(storagePath), false));

Use FileSystem::stringFromFileSystemRepresentation() instead of
String::fromUTF8().

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:100
> +static void
> +webkitUserContentFilterStoreSaveBytes(GRefPtr<GTask>&& task, String&&
identifier, GRefPtr<GBytes>&& source)

One line here.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:152
> +    webkitUserContentFilterStoreSaveBytes(WTFMove(task),
String::fromUTF8(identifier), WTFMove(sourceBytes));

I think you can do GRefPtr<GBytes>(source) directly here instead of using a
local variable + move.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:201
> +    // Try mapping the file in memory first, and fall-back to reading the
contents as a fallback.

fall-back as a fallback? :-)

> Source/WebKit/UIProcess/API/glib/WebKitUserContentFilterStore.cpp:215
> +    struct TaskData {
> +	   String identifier;
> +    };

Use WEBKIT_DEFINE_ASYNC_DATA_STRUCT

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

We can't use this, it was added in glib 2.56, we should use g_file_read_async +
g_input_stream_read_bytes_async(). Maybe it's probably ok to limit this to
local files, receiving a path instead, to make it explicit.

> Tools/MiniBrowser/gtk/main.c:483
> +static void
> +filterSavedCallback(WebKitUserContentFilterStore *store, GAsyncResult
*result, FilterSaveData *data)

One line here.

> Tools/MiniBrowser/gtk/main.c:487
> +    g_assert_nonnull(store);
> +    g_assert_nonnull(result);
> +    g_assert_nonnull(data);

I would use g_assert() since this is not a unit test. Or even remove them,
since they won't be NULL.

> Tools/MiniBrowser/gtk/main.c:568
> +	   g_clear_pointer(&filtersPath, g_free);

filtersPath is not used again, so we don't need to set it to NULL, I think
g_free is enough here.

> Tools/MiniBrowser/gtk/main.c:570
> +	   webkit_user_content_filter_store_save_from_file(store,
"GTKMiniBrowserFilter", contentFilterFile, NULL, (GAsyncReadyCallback)
filterSavedCallback, &saveData);

(GAsyncReadyCallback) filterSavedCallback ->
(GAsyncReadyCallback)filterSavedCallback

> Tools/MiniBrowser/gtk/main.c:573
> +	   g_clear_object(&store);

g_object_unref().

> Tools/MiniBrowser/gtk/main.c:582
> +	   g_clear_pointer(&saveData.mainLoop, g_main_loop_unref);

g_main_loop_unref()

> Tools/MiniBrowser/gtk/main.c:584
> +	   g_clear_pointer(&error, g_error_free);

What is this error for?

> Tools/MiniBrowser/gtk/main.c:585
> +	   g_clear_object(&contentFilterFile);

g_object_unref()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:51
> +#if ENABLE(CONTENT_EXTENSIONS)

Now that we are exposing the API we should not allow to build with content
extensions disabled

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:457
> +    *tempDir = g_dir_make_tmp(nullptr, &error.outPtr());

Use Test::dataDirectory() for any temp file.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:479
> +    g_clear_pointer(&data.mainLoop, g_main_loop_unref);

g_main_loop_unref

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:480
> +    g_clear_object(&store);

g_object_unref

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:497
> +    GUniqueOutPtr<char> filtersDir;
> +    WebKitUserContentFilter* filter = getUserContentFilter(test->m_mainLoop,
&filtersDir.outPtr());

You are not using the filtersDir

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:504
> +    g_clear_pointer(&filter, webkit_user_content_filter_unref);

webkit_user_content_filter_unref

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:505
> +}

We should add cases for every new api entry, save_from_file(), load(),
remove(), fetch_ids, get_identifier(), remova_all(), etc. And we should also
add cases for the possible errors.