[Webkit-unassigned] [Bug 125990] [GTK] Allow passing extra data to web extensions
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 19 04:38:12 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=125990
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #219636|review? |review-
Flag| |
--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-12-19 04:36:16 PST ---
(From update of attachment 219636)
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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list