[Webkit-unassigned] [Bug 125990] [GTK] Allow passing extra initialization data to web extensions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=125990


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #221541|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #20 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-01-19 01:08:55 PST ---
(From update of attachment 221541)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.