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

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro 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 360918: Patch v6

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




--- Comment #61 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 360918
  --> https://bugs.webkit.org/attachment.cgi?id=360918
Patch v6

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

All the async APIs should take cancellable parameters. Even if the internal
WebKit API isn't cancellable, you should at least fake cancellation by
returning G_IO_ERROR_CANCELLED in the finish() function if the user tried to
cancel it before the operation completed. It's just an antipattern to provide
an async API without cancellation.

I also think we should provide
webkit_content_filter_manager_compile_string_finish(), even though it won't
need to do anything other than call
webkit_content_filter_manager_compile_finish().

I'm glad there are lots of layout tests, but there should be API tests too. r-
for this. We don't rely on TestController's C API usage to test our WPE/GTK
APIs. Well, we can rely on that to test most of the functionality so we don't
need complex platform-specific tests, but we should still have basic checks to
make sure the WPE/GTK APIs work.

Carlos Garcia needs to approve it too.

> Source/WebKit/UIProcess/API/glib/WebKitContentFilterManager.cpp:66
> +    RefPtr<API::ContentRuleListStore> store { };

It doesn't need the { }

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:323
> +    _WebKitUserContentFilter(RefPtr<API::ContentRuleList> contentRuleList)

Why RefPtr<API::ContentRuleList> and not RefPtr<API::ContentRuleList>&&?

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

Why RefPtr<API::ContentRuleList> and not RefPtr<API::ContentRuleList>&&?

> Source/WebKit/UIProcess/API/gtk/WebKitAutocleanups.h:86
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC (WebKitUserContentFilter,
webkit_user_content_filter_unref)

You're missing an autocleanup for WebKitContentFilterManager.

> Source/WebKit/UIProcess/API/gtk/WebKitError.h:149
> +/**
> + * WebKitContentFilterError:
> + * @WEBKIT_CONTENT_FILTER_ERROR_INVALID_JSON: The JSON source for a content
filter is invalid.
> + * @WEBKIT_CONTENT_FILTER_ERROR_NOT_FOUND: The requested content filter
could not be found.
> + */

Since: 2.24

> Source/WebKit/UIProcess/API/wpe/WebKitError.h:134
> +/**
> + * WebKitContentFilterError:
> + * @WEBKIT_CONTENT_FILTER_ERROR_INVALID_JSON: The JSON source for a content
filter is invalid.
> + * @WEBKIT_CONTENT_FILTER_ERROR_NOT_FOUND: The requested content filter
could not be found.
> + */

Since: 2.24