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

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

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


Thanks for the patch, it looks great and I'm looking forward to removing the
env variables hack we currently have in ephy. One of the problems we have with
our API is that it's not that easy to know when it's a good moment to call
methods like webkit_web_context_set_web_extensions_directory(), and it has
caused problems already. We encourage to call it asap, but if we had a signal
emitted everytime a new web process is going to be launched, we would have a
perfect place for the user to place those calls. So instead of a specific
signal to initialize the web extensions user data, we could add a method
webkit_web_context_set_web_extensions_user_data() and a generic signal to
notify when a new web process is about to be launched, that could also be used
to call other methods that affect the new web process. The only think I don't
like about this approach is that we would be using a injected bundle client
callback as a generic signal for new web processes launched, only because
there's no other client callback in WebContext::createNewWebProcess(). The
other method that needs to be called before the web process is started is the
onse setting the disk cache directory, so this won't be required once we have
the network process. So, I think we could limit the signal to the web
extensions in the end, but instead of returning a GVariant, both
set_extensions_dir and set_extensions_user_data is expected to be called there.
What do you think?

> Source/WebKit2/ChangeLog:4
> +

Move the bug URL here , please. That's how all other commits are formatted.

> Source/WebKit2/ChangeLog:10
> +	   Allow passing additional information to the injected bundle. On top
of the
> +	   string containing the path to the web extensions directory, a
GVariant can
> +	   be returned by a callback connected to the web-extensions-loaded
signal.
> +	   The GVariant is serialized, passed to the injected bundle, and the
> +	   injected bundle deserializes back the data, which can then be
retrieved
> +	   using the webkit_web_extension_get_extra_data() method.

And the description after the Reviewed by line.

> Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp:125
> +    GVariant* extraData =
webkitWebContextWebExtensionsLoaded(WEBKIT_WEB_CONTEXT(clientInfo));
> +    if (extraData)
> +	   g_variant_ref_sink(extraData);

You can use GRefPtr here.

> Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp:128
> +    GVariant* data = g_variant_new("(msmv)",
> +	  
webkitWebContextGetWebExtensionsDirectory(WEBKIT_WEB_CONTEXT(clientInfo)),
extraData);

And here with adoptGRef

> Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp:133
> +    gchar* dataString = g_variant_print(data, TRUE);

GOwnPtr here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:159
> +    gchar* webExtensionsDirectory;

Use CString for this.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:187
> +	* WebKitWebContext::web-extensions-loaded:

Since this signal is not a notification, but an action required to complete the
initialization of the web extensions, I think we could use a name like
"initialize-web-extensions"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:191
> +	* This signal is emitted when the web extensions have been loaded and
> +	* before they are initialized. It is possible to return a #GVariant

This is not very accurate, and that's also why I don't like the name of the
signal. This signal is emitted when a new web process is about to be launched,
because the user data is passed as part of the web process initialization data.
When the new web process is launched, it's initialized and the injected bundle
is loaded with the user data. So this is emitted even before the web process
has been created, and then web extensions aren't yet loaded.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:196
> +	* Returns: (allow-none): a #GVariant with arbitrary data.

, or %NULL. We should probably use the transfer none annotation since we are
going to consume the gvariant weak ref.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:197
> +	*/

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:202
> +	       0, 0, 0,

You need to use an accumulator here, to make sure the signal is not propagated
once a handler has returned an non NULL pointer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:810
> +    g_free(context->priv->webExtensionsDirectory);
> +    context->priv->webExtensionsDirectory = g_strdup(directory);

Using a CString you don't need the g_free()

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:221
> +    g_signal_connect(webkit_web_context_get_default(),
"web-extensions-loaded",
> +	   G_CALLBACK(webExtensionsLoadedCallback), nullptr);

We can probably move the whole test into its own class and connect the signal
in the constructor and disconnect it in the destructor to make sure it won't
affect other tests.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:233
> +    WebViewTest::add("WebKitWebView", "web-extensions-loaded-signal",
testWebExtensionsLoadedSignal);
> +    WebViewTest::add("WebKitWebView", "web-extensions-extra-data",
testWebExtensionsExtraData);

We can probably merge both into a single test case, because they depend on each
other. It's impossible to get the extra data if the signal hasn't been emitted.


> Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp:174
> +	   GVariant* extraData =
webkit_web_extension_get_extra_data(WEBKIT_WEB_EXTENSION(userData));
> +	   if (extraData) {

if (GVariant* extraData =
webkit_web_extension_get_extra_data(WEBKIT_WEB_EXTENSION(userData)))

> Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp:175
> +	       gchar* extraDataString = g_variant_print(extraData, TRUE);

Use GOwnPtr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:46
> +    GVariant* extraData;

Use GrefPtr here

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:162
> + * webkit_web_extension_get_extra_data:

Instead of extra data (the use of the injected bundle user data for the web
extensions directory is actually something internal) I would use user_data
which is quite common in GNOME APIs.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:165
> + * Obtains the extra data returned from a
#WebKitWebContext::web-extensions-loaded

Obtains the initialization user data returned form the
#WebKitWebContext::web-extensions-loaded signal.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:171
> + */

Since: 2.4

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.h:71
> +WEBKIT_API GVariant *
> +webkit_web_extension_get_extra_data (WebKitWebExtension *extension);

Please line up arguments, like we do in other headers.

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:66
> +	   GVariant* variant =
> +	       g_variant_parse(nullptr, dataString.data(), dataString.data() +
dataString.length(), nullptr, nullptr);
> +
> +	   if (variant) {

Can this really fail? We created that variant in the ui process with
g_variant_print().

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:69
> +	       gchar* directory = nullptr;

This should be const char*

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:75
> +	       if (extraData)
> +		   g_variant_ref_sink(extraData);

I think we should pass the weak ref of the variant to the web extension to be
consumed there.

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:76
> +	       g_variant_unref(variant);

Use GRefPtr