[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 #221586|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #23 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-01-21 01:23:20 PST ---
(From update of attachment 221586)
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));

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