[webkit-reviews] review denied: [Bug 125990] [GTK] Allow passing extra initialization data to web extensions : [Attachment 220378] 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 220378: Patch
https://bugs.webkit.org/attachment.cgi?id=220378&action=review

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


Thanks for the new patch. I think we already agreed on having a signal
WebKitWebContext::initialize-web-extensions and a method to set the init user
data webkit_web_context_set_web_extensions_initialization_user_data(). Then we
document that the best place to call both set_web_extensions_directory and
set_web_extensions_initialization_user_data is in the initialize-web-extensions
callback. If we want to avoid having to keep the user data around in the
context all the time, we could clear the data after the signal is emitted, and
document that set_web_extensions_initialization_user_data() should always be
called when initialize-web-extensions is emitted. This way we also avoid the
accumulator and the signal having to return a GVariant. Also in the web
extensions api, we agreed on adding a new initialization function instead of
webkit_web_extension_get_initialization_user_data(), something like
webkit_web_extension_initialize_with_user_data() that receives the GVariant as
a parameter. If this symbol is not present in the extension, we just fallback
to webkit_web_extension_initialize(). See comment #11.

> Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp:123
> +    GRefPtr<GVariant> extraData =
webkitWebContextWebExtensionsLoaded(WEBKIT_WEB_CONTEXT(clientInfo));

WebExtensionsLoaded is not accurate, this is called even before the web process
has been launched. Use the name of the signal that is going to be emitted:

webkitWebContextInitializeWebExtensions()

We could make that method build the final GVariant, so that we don't need more
private api here. It emits the signal and then builds the GVariant with the
extensions dir and user_data.

GRefPtr<GVariant> data =
webkitWebContextInitializeWebExtensions(WEBKIT_WEB_CONTEXT(clientInfo));
GOwnPtr<gchar> dataString(g_variant_print(data.get(), TRUE));
return static_cast<WKTypeRef>(WKStringCreateWithUTF8CString(dataString.get()));


The function would return a float reference, so you don't need to use adoptGRef
in this case.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensionsInitialization.cpp:45

> +    GRefPtr<GDBusProxy> proxy =
adoptGRef(bus->createProxy("org.webkit.gtk.WebExtensionTest",
> +	   "/org/webkit/gtk/WebExtensionTest" ,
"org.webkit.gtk.WebExtensionTest", test->m_mainLoop));
> +    GRefPtr<GVariant> result = adoptGRef(g_dbus_proxy_call_sync(
> +	   proxy.get(),
> +	   "GetExtraDataAsString",
> +	   nullptr,
> +	   G_DBUS_CALL_FLAGS_NONE,
> +	   -1, 0, 0));
> +    g_assert(result);

I think it would be a lot easier if you simply check the user data in the web
extension. Here you simply pass "'extra data'" and in the web extension you
assert if the user data is not "'extra data'"

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensionsInitialization.cpp:65

> +   
webkit_web_context_set_web_extensions_directory(webkit_web_context_get_default(
), WEBKIT_TEST_WEB_EXTENSIONS_DIR);
> +    initializeWebExtensionsSignalHandlerId =
g_signal_connect(webkit_web_context_get_default(),
> +	   "initialize-web-extensions",
G_CALLBACK(initializeWebExtensionsCallback), nullptr);

Create a class instead of adding this in beforeAll. Call
webkit_web_context_set_web_extensions_directory in the signal hanlder too. You
don't need to keep the handler id, you can use
g_signal_handlers_disconnect_matched in the class destructor.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensionsInitialization.cpp:71

> +    WebViewTest::add("WebKitWebView",
"web-extensions-initialization-user-data",
testWebExtensionsInitializationUserData);

Would it be possible to reuse the existing web extensions test file? 

"WebKitWebView" -> "WebKitWebContext"