Bug 125990 – [GTK] Allow passing extra initialization data to web extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Adrian Perez]

Right now in the GTK port the injected bundles get only the path to the directory
where to search for web extensions, but there is no way of passing data to the web
extensions from the UI process. This leads to a situation where the UI process has
to set environment variables to pass some configurations to the web process. Ideally,
the WebKitGTK port should provide API to set which data is to be passed to the web
extensions, pass it along with the web extensions directory when injected bundle
initialization is done, and provide an API for web extensions to retrieve the data.

[Adrian Perez]

Created attachment 219636 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 219636 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 219636 [details]
Patch

So, what I understand from reading the API and the changelog is we are passing a GVariant with arbitrary initialization parameters for all extensions when we get a signal emission. I have a question and a suggestion:

Why do we need the signal for that, is it because of some architectural constraint/implementation difficulty? Can't we just set the data along with the path before the extensions are even loaded?

I think it would be better to call this "initialization parameters" instead of extra data, so it's clearer what they are for. I thought at first that they would be something that can be sent at any point rather than just for initialization.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 219636 [details])
> So, what I understand from reading the API and the changelog is we are passing a GVariant with arbitrary initialization parameters for all extensions when we get a signal emission. I have a question and a suggestion:
> 
> Why do we need the signal for that, is it because of some architectural constraint/implementation difficulty? Can't we just set the data along with the path before the extensions are even loaded?

Being this a signal fixes two problems:

 - It allows you to set dynamic data, in case of multiple web processes you might want to set different data for each web process, like a unique id, for example. We might use that to make sure that the dbus names used by the web extensions are different.

 - It sets a place to initialize the web extensions directory (and this user_data).

> I think it would be better to call this "initialization parameters" instead of extra data, so it's clearer what they are for. I thought at first that they would be something that can be sent at any point rather than just for initialization.

what about initialization_user_data? which is what wk2 uses internally?

[Adrian Perez]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 219636 [details] [details])
> > So, what I understand from reading the API and the changelog is we are passing a GVariant with arbitrary initialization parameters for all extensions when we get a signal emission. I have a question and a suggestion:
> > 
> > Why do we need the signal for that, is it because of some architectural constraint/implementation difficulty? Can't we just set the data along with the path before the extensions are even loaded?
> 
> Being this a signal fixes two problems:
> 
>  - It allows you to set dynamic data, in case of multiple web processes you might want to set different data for each web process, like a unique id, for example. We might use that to make sure that the dbus names used by the web extensions are different.
> 
>  - It sets a place to initialize the web extensions directory (and this user_data).

Exactly. IMHO not having to warn in the documentation “do this before
$FOO happens” is a good thing: having the warning in the docs makes it
easy for developers to skip it, ot to have to do guesswork when they
have to decide when to call a certain function. If we provide a signal
there is an event that clearly points when.

> > I think it would be better to call this "initialization parameters" instead of extra data, so it's clearer what they are for. I thought at first that they would be something that can be sent at any point rather than just for initialization.
> 
> what about initialization_user_data? which is what wk2 uses internally?

I wasn't sure whether to let the name permeate to the public WebKitGTK API, 
but it is also true that “extra data“ or ”user data“ lacks clarity... Let's
go with “initialization user data” then.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 219636 [details] [details] [details])
> > > So, what I understand from reading the API and the changelog is we are passing a GVariant with arbitrary initialization parameters for all extensions when we get a signal emission. I have a question and a suggestion:
> > > 
> > > Why do we need the signal for that, is it because of some architectural constraint/implementation difficulty? Can't we just set the data along with the path before the extensions are even loaded?
> > 
> > Being this a signal fixes two problems:
> > 
> >  - It allows you to set dynamic data, in case of multiple web processes you might want to set different data for each web process, like a unique id, for example. We might use that to make sure that the dbus names used by the web extensions are different.
> > 
> >  - It sets a place to initialize the web extensions directory (and this user_data).
> 
> Exactly. IMHO not having to warn in the documentation “do this before
> $FOO happens” is a good thing: having the warning in the docs makes it
> easy for developers to skip it, ot to have to do guesswork when they
> have to decide when to call a certain function. If we provide a signal
> there is an event that clearly points when.
> 
> > > I think it would be better to call this "initialization parameters" instead of extra data, so it's clearer what they are for. I thought at first that they would be something that can be sent at any point rather than just for initialization.
> > 
> > what about initialization_user_data? which is what wk2 uses internally?
> 
> I wasn't sure whether to let the name permeate to the public WebKitGTK API, 
> but it is also true that “extra data“ or ”user data“ lacks clarity... Let's
> go with “initialization user data” then.

After thining about it a bit more, I think I prefer just user_data, at least in the way it's currently implemented. Ideally, this should actually be initialization data, passed as parameter to webkit_web_extension_initialize, instead of getting it from the WebExtension object. That way we wouldn't need to keep the data in memory all the time. The problem is that we can't change the initialization symbol (unless we deprecate the current one and add a new one receiving the initialization user data, not sure it's worth it). The thing is that the way we are currently implementing, it's data associated to the web extension passed by the user from the ui process. You can get that data from the web extension object at any time. So, in my opinion we could:

 a) Deprecate webkit_web_extension_initialize() and add a new symbol receiving the init user data. We would check the new symbol first always falling back to the deprecated one when not found. 

 b) Use just user_data always available from the web extension and avoid the initialization word to avoid confusion. 

I think a) is the ideal solution from the API point of view, but requires to deprecate current init symbol.

[Gustavo Noronha (kov)]

(In reply to comment #6)
> >  - It allows you to set dynamic data, in case of multiple web processes you might want to set different data for each web process, like a unique id, for example. We might use that to make sure that the dbus names used by the web extensions are different.
> > 
> >  - It sets a place to initialize the web extensions directory (and this user_data).
> 
> Exactly. IMHO not having to warn in the documentation “do this before
> $FOO happens” is a good thing: having the warning in the docs makes it
> easy for developers to skip it, ot to have to do guesswork when they
> have to decide when to call a certain function. If we provide a signal
> there is an event that clearly points when.

Sounds like this bug should depend no bug 123292 then =)
 
(In reply to comment #7)
> After thining about it a bit more, I think I prefer just user_data, at least in the way it's currently implemented. Ideally, this should actually be initialization data, passed as parameter to webkit_web_extension_initialize, instead of getting it from the WebExtension object. That way we wouldn't need to keep the data in memory all the time. The problem is that we can't change the initialization symbol (unless we deprecate the current one and add a new one receiving the initialization user data, not sure it's worth it). The thing is that the way we are currently implementing, it's data associated to the web extension passed by the user from the ui process. You can get that data from the web extension object at any time. So, in my opinion we could:
> 
>  a) Deprecate webkit_web_extension_initialize() and add a new symbol receiving the init user data. We would check the new symbol first always falling back to the deprecated one when not found. 
> 
>  b) Use just user_data always available from the web extension and avoid the initialization word to avoid confusion. 
> 
> I think a) is the ideal solution from the API point of view, but requires to deprecate current init symbol.

I think going with a and calling it 'initialization user data' would be the best from an API perspective. We can ditch the data once the extension is initialized - if the extension really wants to keep it they can store it themselves, how about that? We don't really need to deprecate the initialize function, actually, it's kind of a _new() and _new_with_text() kind of thing from my perspective.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #6)
> > >  - It allows you to set dynamic data, in case of multiple web processes you might want to set different data for each web process, like a unique id, for example. We might use that to make sure that the dbus names used by the web extensions are different.
> > > 
> > >  - It sets a place to initialize the web extensions directory (and this user_data).
> > 
> > Exactly. IMHO not having to warn in the documentation “do this before
> > $FOO happens” is a good thing: having the warning in the docs makes it
> > easy for developers to skip it, ot to have to do guesswork when they
> > have to decide when to call a certain function. If we provide a signal
> > there is an event that clearly points when.
> 
> Sounds like this bug should depend no bug 123292 then =)

It's not the same, this happens before the new web process is launched, and the other one is emitted when the web process has been launched and the connection has been established, which is too late to set user data and web extensions dir.

> (In reply to comment #7)
> > After thining about it a bit more, I think I prefer just user_data, at least in the way it's currently implemented. Ideally, this should actually be initialization data, passed as parameter to webkit_web_extension_initialize, instead of getting it from the WebExtension object. That way we wouldn't need to keep the data in memory all the time. The problem is that we can't change the initialization symbol (unless we deprecate the current one and add a new one receiving the initialization user data, not sure it's worth it). The thing is that the way we are currently implementing, it's data associated to the web extension passed by the user from the ui process. You can get that data from the web extension object at any time. So, in my opinion we could:
> > 
> >  a) Deprecate webkit_web_extension_initialize() and add a new symbol receiving the init user data. We would check the new symbol first always falling back to the deprecated one when not found. 
> > 
> >  b) Use just user_data always available from the web extension and avoid the initialization word to avoid confusion. 
> > 
> > I think a) is the ideal solution from the API point of view, but requires to deprecate current init symbol.
> 
> I think going with a and calling it 'initialization user data' would be the best from an API perspective. We can ditch the data once the extension is initialized - if the extension really wants to keep it they can store it themselves, how about that? We don't really need to deprecate the initialize function, actually, it's kind of a _new() and _new_with_text() kind of thing from my perspective.

It's a bit weird having two entry points, but I don't see any problem. We can add webkit_web_extension_inisitalize_with_user_data() then and document that if this is present the other one is ignored.

[Gustavo Noronha (kov)]

(In reply to comment #9)
> > Sounds like this bug should depend no bug 123292 then =)
> 
> It's not the same, this happens before the new web process is launched, and the other one is emitted when the web process has been launched and the connection has been established, which is too late to set user data and web extensions dir.

Oh, I see, though I guess the use cases are pretty much the same, the only reason why the other patch relies on the connection is because it's a convenient place to store the process identifier, AFAIK. If we could think of a better way of having that identifier managed we could fuse these two signals.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > > Sounds like this bug should depend no bug 123292 then =)
> > 
> > It's not the same, this happens before the new web process is launched, and the other one is emitted when the web process has been launched and the connection has been established, which is too late to set user data and web extensions dir.
> 
> Oh, I see, though I guess the use cases are pretty much the same, the only reason why the other patch relies on the connection is because it's a convenient place to store the process identifier, AFAIK. If we could think of a better way of having that identifier managed we could fuse these two signals.

I'm not sure, I think it might be useful to know when the web process has been launched, so I think we could have initialize-web-extensions emitted for getInjectedBundleInitializationUserData as the perfect place to call set_web_extensions_dir + set_web_extensions_initialization_user_data, and web-process-started emitted when the web process has been launched as two different signals.

[Gustavo Noronha (kov)]

OK, makes sense to me, I'll unlink the bugs then

[Adrian Perez]

Created attachment 220378 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 220378 [details]
Patch

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"

[Adrian Perez]

Created attachment 221381 [details]
Patch

[Adrian Perez]

(In reply to comment #15)
> Created an attachment (id=221381) [details]
> Patch

Regarding this version of the patch, note that I still have to update the
tests. And talking about tests: the intialization user data being passed
correctly is a must for web extensions to work at all, because the web
extensions directory is now passed as part of the serialized GVariant.
As the mechanism must always work, I am thinking of keeping the test case
in “TestWebExtensions.cpp”, WDYT?

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=221381) [details] [details]
> > Patch
> 
> Regarding this version of the patch, note that I still have to update the
> tests. And talking about tests: the intialization user data being passed
> correctly is a must for web extensions to work at all, because the web
> extensions directory is now passed as part of the serialized GVariant.
> As the mechanism must always work, I am thinking of keeping the test case
> in “TestWebExtensions.cpp”, WDYT?

Sounds good to me.

[Adrian Perez]

Created attachment 221540 [details]
Patch

[Adrian Perez]

Created attachment 221541 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 221541 [details]
Patch

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::create(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>("webkit_web_extension_initialize_with_user_data");
>          WebKitWebExtensionInitializeFunction initializeFunction =
>              module->functionPointer<WebKitWebExtensionInitializeFunction>("webkit_web_extension_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.

[Carlos Garcia Campos]

I've noticed that GVariant floating references are not correctly handled by GRefPtr, and my review comments assumed they were correctly handled. See https://bugs.webkit.org/show_bug.cgi?id=127246

[Adrian Perez]

Created attachment 221586 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 221586 [details]
Patch

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

Looks great! I have just a few more nits. This would be r+ with nits if you were a committer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:805
> + * It is recommended to use this method in a handler for the
> + * WebKitWebContext::initialize-web-extensions signal.

In GTK+ we usually use callback instead of handler, so maybe "in the callback of the". 
WebKitWebContext::initialize-web-extensions -> #WebKitWebContext::initialize-web-extensions

I think we should merge this with the previous paragraph, because we are giving two different messages, cal this before loading anything and call this in initialize-web-extensions signal. So what about something like:

This method must be called before loading anything in this context, otherwise it will not have any effect. You can connect to #WebKitWebContext::initialize-web-extensions signal to call this method before anything is loaded.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:820
> + * Set arbitrary user data to be passed to Web Extensions on initialization.

s/arbitrary//

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:822
> + * %webkit_web_context_set_web_extensions_initialization_user_data() of type

%? The function name is webkit_web_extension_initialize_with_user_data(). I would just mention that this data is passed to the #WebKitWebExtensionInitializeWithUserDataFunction defined in the web extension.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:826
> + * Passing %NULL will cause no data being passed to Web Extensions, which
> + * will receive a %NULL as second parameter to the initialization function.

What's the difference between calling this with NULL and not calling it at all? I think we should say that passing NULL resets any data previously set with this function. this makes clear that there's no point on calling this with NULL the first time. I'm still not sure we should allow passing null, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:829
> + * It is recommended to use this method in a handler for the
> + * WebKitWebContext::initialize-web-extensions signal.

We should say something similar to the other method, it's important to say here too that this doesn't have any effect if it's called late. We can use the same sentence.

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:56
> +static void parseUserData(WKTypeRef userData, String& webExtensionsDirectory,
> +    GRefPtr<GVariant>& initializationUserData)

Use a single line

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:73
> +    if (directory)
> +        webExtensionsDirectory = WebCore::filenameToString(directory);

filenameToString returns String() in case the passed filename is NULL, so you don't need the if you do:

webExtensionsDirectory = WebCore::filenameToString(directory);

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:75
> +    if (data)
> +        initializationUserData = adoptGRef(data);

Same here, adoptGRef accepts NULL

> Source/WebKit2/WebProcess/gtk/WebGtkExtensionManager.cpp:111
> +        WebKitWebExtensionInitializeWithUserDataFunction initializeWithUserDataFunction =
> +            module->functionPointer<WebKitWebExtensionInitializeWithUserDataFunction>("webkit_web_extension_initialize_with_user_data");
> +
> +        if (initializeWithUserDataFunction) {
> +            initializeWithUserDataFunction(m_extension.get(), userData.get());
> +            m_extensionModules.append(module.leakPtr());
> +        } else {
> +            WebKitWebExtensionInitializeFunction initializeFunction =
> +                module->functionPointer<WebKitWebExtensionInitializeFunction>("webkit_web_extension_initialize");
> +            if (initializeFunction) {
> +                initializeFunction(m_extension.get());
> +                m_extensionModules.append(module.leakPtr());
> +            }
> +        }

What do you think about moving this to a helper function too? This way we avoid duplicating the m_extensionModules.append(module.leakPtr()); Something like:

if (initializeWebExtension(module.get(), userData.get()))
    m_extensionModules.append(module.leakPtr());

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:187
> +    GRefPtr<GVariant> result = g_dbus_proxy_call_sync(

I think g_dbus_proxy_call_sync returns a full reference, so it should be adopted.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:195
> +    const gchar* str = nullptr;

Use something like userData instead of str.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:196
> +    g_variant_get(result.get(), "(s)", &str);

You need to use "(&s)" to get a const char *

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:197
> +    g_assert(g_str_equal(str, webExtensionsUserData));

Use g_assert_cmpstr instead

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:225
> +        g_dbus_method_invocation_return_value(invocation, g_variant_new("(s)",
> +            initializationUserData && g_variant_is_of_type(initializationUserData.get(), G_VARIANT_TYPE_STRING)
> +                ? g_variant_get_string(initializationUserData.get(), nullptr)
> +                : ""));

We are always passing the data, since this is not a generic extension, I think we can simply assert if the user data is NULL at this point. That would make this easier to read.

g_assert(initializationUserData);
g_assert(g_variant_is_of_type(initializationUserData.get(), G_VARIANT_TYPE_STRING));
g_dbus_method_invocation_return_value(invocation, g_variant_new("(s)", g_variant_get_string(initializationUserData.get(), nullptr));

[Adrian Perez]

Created attachment 221731 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 221731 [details]
Patch

Excellent! Thank you very much. This is new API, and we already discussed this with Gustavo, but not setting cq+ yet until Gustavo confirms he is happy with the final patch.

[WebKit Commit Bot]

Comment on attachment 221731 [details]
Patch

Clearing flags on attachment: 221731

Committed r162441: <http://trac.webkit.org/changeset/162441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.