Bug 138871

Summary: [GTK] Provide convenience API in DOM bindings to post messages to user message handlers
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WebKitGTKAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, darin, gustavo, pnormand, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Adrian Perez 2014-11-19 07:47:19 PST 
As per this comment: https://bugs.webkit.org/show_bug.cgi?id=138411#c1

  ”The "raw" API is not very convenient to use, we should
  add custom API to make it easier to use.”
Comment 1 Adrian Perez 2014-11-19 08:11:07 PST 
Created attachment 241860 [details]
Patch
Comment 2 Carlos Garcia Campos 2014-11-19 08:43:18 PST 
Comment on attachment 241860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241860&action=review

Thanks! I have just some minor comments.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:68
> +gboolean webkit_dom_dom_window_post_user_message(WebKitDOMDOMWindow* window, const gchar* handlerName, const gchar* message)

This is equivalent to window.webkit.messageHandlers.<handler>.postMessage, so I think we could convert that into webkit_dom_dom_window_webkit_message_handlers_post_message(WebKitDOMDOMWindow* window, const gchar* handlerName, const gchar* message);

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:70
> +#if ENABLE(USER_MESSAGE_HANDLERS)

We are building with USER_MESSAGE_HANDLERS unconditionally, so we can just remove the #if.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:72
> +    g_return_val_if_fail(WEBKIT_DOM_IS_DOM_WINDOW(window), FALSE);
> +    g_return_val_if_fail(message, FALSE);

You should check handlerName is a valid pointer too

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:78
> +    auto handler = domWindow->webkitNamespace()->messageHandlers()->handler(String::fromUTF8(handlerName), WebCore::mainThreadNormalWorld());

DOMWindow::webkitNamespace() can return nullptr, you should check it.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:79
> +    if (handler == nullptr)

Don't compare to nullptr, use !handler

> Source/WebCore/bindings/gobject/WebKitDOMCustom.h:51
> + */

Since: 2.8

> Source/WebCore/bindings/gobject/WebKitDOMCustom.symbols:3
> +gboolean webkit_dom_dom_window_post_user_message(WebKitDOMDOMWindow*, const gchar*, const gchar*)

You should also add the symbols to the webkitdom.symbols file, right after webkit_dom_dom_window_get_type

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:371
> +    g_assert(test->registerHandler("dom-convenience"));
> +
> +    test->loadHtml("<html></html>", nullptr);
> +    WebKitJavascriptResult* javascriptResult = test->waitUntilMessageReceived("dom-convenience");
> +    g_assert(javascriptResult);
> +    GUniquePtr<char> valueString(WebViewTest::javascriptResultToCString(javascriptResult));
> +    g_assert_cmpstr(valueString.get(), ==, "DocumentLoaded");
> +
> +    test->unregisterHandler("dom-convenience");

Maybe we can merge both tests, connect to both handlers and check that both messages are received and are the same.
Comment 3 Adrian Perez 2014-11-19 11:40:27 PST 
Created attachment 241871 [details]
Patch
Comment 4 Adrian Perez 2014-11-19 11:41:06 PST 
Nits fixed, the patch should be good to go now :-)
Comment 5 Carlos Garcia Campos 2014-11-20 00:26:03 PST 
Comment on attachment 241871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241871&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:355
> +    g_assert(javascriptResult);

Why duplicating the assert here?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:366
> +    g_assert(javascriptResult);
> +    g_assert(javascriptResult);

Ditto.
Comment 6 Adrian Perez 2014-11-20 04:00:29 PST 
Created attachment 241939 [details]
Patch
Comment 7 WebKit Commit Bot 2014-11-20 04:46:05 PST 
Comment on attachment 241939 [details]
Patch

Clearing flags on attachment: 241939

Committed r176393: <http://trac.webkit.org/changeset/176393>
Comment 8 WebKit Commit Bot 2014-11-20 04:46:09 PST 
All reviewed patches have been landed.  Closing bug.