[webkit-reviews] review denied: [Bug 125990] [GTK] Allow passing extra initialization data to web extensions : [Attachment 221541] Patch

[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 125990: [GTK] Allow passing extra initialization data to web extensions
https://bugs.webkit.org/show_bug.cgi?id=125990

Attachment 221541: Patch
https://bugs.webkit.org/attachment.cgi?id=221541&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221541&action=review


Looks good, I still have a few comments.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:809
> -    // We pass the additional web extensions directory to the injected
bundle as initialization user data.
> -   
context->priv->context->setInjectedBundleInitializationUserData(API::String::cr
eate(WebCore::filenameToString(directory)));
> +    context->priv->webExtensionsDirectory = directory;

Update also the documentation of this method to mention that
initialize-web-extensions signal is the perfect place to call it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:815
> + * webkit_web_context_set_web_extensions_initialization_user_data:
> + * @context: a #WebKitWebContext
> + *

userData parameter is missing in documentation. You should also explain briefly
what this function is for and how to use it, liking to the
initialize-web-extensions signal.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:821
> +    g_return_if_fail(WEBKIT_IS_WEB_CONTEXT(context));
> +

What about userData, do we allow passing NULL?, what happen in that case? We
should also document this behaviour.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
> +GRefPtr<GVariant> webkitWebContextInitializeWebExtensions(WebKitWebContext*
context)

We don't need to use GRefPtr here, we can return the pointer with the weak
reference that will be consumed by the caller.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.h:62
> + *

Add a tag See also: WebKitWebExtensionInitializeFunction and mention they are
mutually exclusive but this one takes precedence.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.h:63
> + * Since: 0.26

0.26? :-)

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:68
> +    ASSERT(userDataString);
> +    ASSERT(WKGetTypeID(userDataString) == WKStringGetTypeID());
> +
> +    CString dataString =
toImpl(static_cast<WKStringRef>(userDataString))->string().utf8();
> +    GRefPtr<GVariant> variant =
> +	   g_variant_parse(nullptr, dataString.data(), dataString.data() +
dataString.length(), nullptr, nullptr);
> +
> +    ASSERT(variant);
> +    ASSERT(g_variant_check_format_string(variant.get(), "(m&smv)", FALSE));
> +
> +    const char* directory = nullptr;
> +    GVariant* data = nullptr;
> +    g_variant_get(variant.get(), "(m&smv)", &directory, &data);

This could probably be moved to a helper function parseUserData or something
like that.

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:72
> +    if (directory)
> +	   webExtensionsDirectory = String::fromUTF8(directory);

Note that this was created in the UI process with WebCore::filenameToString(),
but now the utf8 string passed by the user is sent to the web process, so here
you should use WebCore::filenameToString() instead of String::fromUTF8()

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:79
> +    GRefPtr<GVariant> userData;
> +    if (data)
> +	   userData = data;

This is wrong. The data returned by g_variant_get() is a full reference that
you should adopt, but not here, right after g_variant_get() so that it's also
released by any previous early return. It's safe to set nullptr to a GrefPtr,
so you don't need the if either, you can simply: GRefPtr<GVariant> userData =
adoptGRef(data);

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:96
> +	   WebKitWebExtensionInitializeWithUserDataFunction
initializeWithUserDataFunction =
> +	      
module->functionPointer<WebKitWebExtensionInitializeWithUserDataFunction>("webk
it_web_extension_initialize_with_user_data");
>	   WebKitWebExtensionInitializeFunction initializeFunction =
>	      
module->functionPointer<WebKitWebExtensionInitializeFunction>("webkit_web_exten
sion_initialize");
> -	   if (!initializeFunction)
> +	   if (!initializeWithUserDataFunction && !initializeFunction)
>	       continue;

I think we can avoid looking for both symbols every time. Try to get the new
one, if not present fallback to the previous one.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:184
> +    GRefPtr<GVariant> userData = adoptGRef(g_variant_new("s",
webExtensionsUserData));

You should not adopt this one, since nobody is going to consume the floating
ref.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:231
> +	   g_dbus_method_invocation_return_value(invocation,
> +	       g_variant_new("(b)", g_str_equal(checkString,
initializationUserDataString.get())));

I think we should move this logic to the UI process instead. Rename the method
to GetInitializationUserData and check the string in the UI process like we do
with GetTitle, for example.