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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=133730

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

------- 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

s/name//

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

#WebKitUserContentManager::script-message-received

> 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 =
WebScriptMessageHandler::create(std::make_unique<ScriptMessageClientGtk>(manage
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.

s/name//

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

#WebKitUserContentManager::script-message-received
s/signal:/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
> +#if ENABLE_USER_MESSAGE_HANDLERS

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:157
> +    auto resultRefPtr =
reinterpret_cast<GRefPtr<WebKitJavascriptResult>*>(refPtr);
> +    *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
flipped

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:189
> +    javascriptResult =
test->runJavaScriptAndWaitUntilFinished("window.webkit.msg.postMessage('user
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
> +#if ENABLE_USER_MESSAGE_HANDLERS

Same comment here, this should never be disabled.