[webkit-reviews] review denied: [Bug 125990] [GTK] Allow passing extra initialization data to web extensions : [Attachment 221586] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 21 01:25:48 PST 2014


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 initialization data to web extensions
https://bugs.webkit.org/show_bug.cgi?id=125990

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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>("webk
it_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_exten
sion_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));


More information about the webkit-reviews mailing list