[webkit-reviews] review denied: [Bug 133730] [GTK] Support script message handlers WebKitUserContentManager : [Attachment 234986] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Aug 3 04:01:33 PDT 2014

Carlos Garcia Campos <cgarcia at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 133730: [GTK] Support script message handlers WebKitUserContentManager

Attachment 234986: Patch

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234986&action=review

I thought window.webkit.<name>.postMessage was only allowed from user scripts,
since we use the user content manager to register the handler

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:24
> +#include "WebKitJavascriptResult.h"
> +#include "WebKitJavascriptResultPrivate.h"

WebKitJavascriptResultPrivate.h already includes WebKitJavascriptResult.h

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:175
> + * Registers a new user script message handler name. After it is


> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:178
> + * to the WebKitUserContentManager::script-message-received signal. The


> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:204
> +    const auto handler_name = String::fromUTF8(name);

You don't need this, since it's used only one, you can use
String::fromUTF8(name) directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:205
> +    auto handler =
r, name), handler_name);

I'm not sure about using auto here, because the method returns a
PassRefPtr<WebScriptMessageHandler>, but we want to use a
RefPtr<WebScriptMessageHandler>. This is also one of the cases where making it
explicit is lees confusing.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:214
> + * Unregisters a previously registered message handler name.


> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:217
> + * WebKitUserContentManager::script-message-received signal:


> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:218
> + * the signal handlers will be kept connected, but the signal

s/the signal handlers/they/

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:222
> + * See: webkit_user_content_manager_register_script_message_handler()

s/See:/See also/

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:153

This is exposed to the API, we should always build with

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:157
> +    auto resultRefPtr =
> +    *resultRefPtr = jsResult;

This is a bit confusing, maybe we could use a custom class for this test that
connects to the signal and keeps the result as a member.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:184
> +    // Call "document.webkit.msg.postMessage()" and check that the flag was

document.webkit.msg.postMessage -> window.webkit.msg.postMessage
Also finish the comment with period.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:189
> +    javascriptResult =
message');", &error.outPtr());
> +    g_assert(javascriptResult);
> +    g_assert(!error.get());
> +
> +    g_assert(message.get());

How can you be sure that at this point the signal has already been received? I
think you should run the main loop and wait until the signal is emitted,
instead of until the js is run.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:209

Same comment here, this should never be disabled.

More information about the webkit-reviews mailing list