Bug 133730 – [GTK] Support script message handlers WebKitUserContentManager

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adrian Perez]

In order to be able to remove WebKitWebViewGroup (as per bug #133729),
we need an alternative API to allow injection of user scripts and style
sheets.

We would add the following new classes to the public API:

 - WebKitUserContentManager
 - WebKitUserContent (abstract, has common parts of the child classes)
    - WebKitUserScript
    - WebKitUserStyleSheet

[Adrian Perez]

JFTR, I have already started working at this, and expect to post a WIP
version of the patches needed soonish :-)

[Adrian Perez]

Created attachment 232952 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

(In reply to comment #2)
> Created an attachment (id=232952) [details]
> Patch

Notes about this first version of the patch:

 - **Not ready** for landing.

 - The main idea is to use it to start reviewing the new API for
   WebKitUserContentManager, WebKitUserContent, WebKitUserScript,
   and WebKitUserStyleSheet.
   
 - The actual implementation is WIP and though the bits to support
   the new classes should be mostly ready, the code to attach an
   WebKitUserContentManager and for handling the user script 
   messages (in ScriptMessageClientGtk) are missing.

What I am most interested for the moment is having a second pair
of eyes review the new bits of the API.

As for attaching a WebKitUserContentManager to a web view, my plan
so far is to:

 - Add webkit_web_view_new_with_user_content_manager(view, manager).
   Views created with the other functions will have a default
   WebKitUserContentManager.

 - Add webkit_web_view_get_user_content_manager(view).

One doubt I have is: when creating a related web view (by means of
webkit_web_view_new_with_related_view()), does the new related web
view use the same WebKitUserContentManager, or would it get a fresh
one? If the correct thing to do is the later, would we also add a
webkit_web_view_new_with_related_view_and_user_content_manager()
function?

[Carlos Garcia Campos]

Comment on attachment 232952 [details]
Patch

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

Looks good so far. Removing the review flags since this is a wip patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:25
> +

Do not leave empty lines between header includes.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:78
> +    _WebKitUserContentPrivate()
> +        : injectedFrames(WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES) { }

You don't really need the constructor, since WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES is 0 and the struct is already filled with 0 when allocated.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:83
> +    CString source;
> +    Vector<String> whitelist;
> +    Vector<String> blacklist;
> +    WebKitUserContentInjectedFrames injectedFrames;

hmm, the problem of having this common class is that we can't wrap the WebCore classes directly, or maybe we could use an union or something like that.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:86
> +WEBKIT_DEFINE_TYPE(WebKitUserContent, webkit_user_content, G_TYPE_OBJECT)

This should be an abstract class.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:91
> +    // Empty

Remove this comment :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:97
> +void webkit_user_content_set_source(WebKitUserContent* user_content, const gchar* source)

I'm not sure about having set/get for all members, some of them (if not all) should probably be construct only, passed to the new method and then readonly. You should not be able to modify them once the object is created, I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:108
> +const gchar* webkit_user_content_get_source(WebKitUserContent* user_content)

user_content -> userContent everywhere :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:135
> +void webkit_user_content_set_whitelist(WebKitUserContent* user_content, GList* whitelist)

I would use const char* const* for the lists instead of GList like our current API does.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:187
> +    _WebKitUserScriptPrivate()
> +        : injectionTime(WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START) { }

You don't need this one either

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:202
> + * @source: (transfer none): Source code of the user script.

I think you don't need to use the transfer none annotation for parameters that are const

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:206
> +WebKitUserContent* webkit_user_script_new(const gchar* source)

I think we should receive all members as contruct parameters, and if possible create the WebCore::UserScript directly here instead of duplicating everything.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:231
> +UserScript webkitWebKitUserScriptToUserScript(WebKitUserScript* script, guint64 scriptId)

Why do we need a script id?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:237
> +    StringBuilder urlStringBuilder;
> +    urlStringBuilder.append("user-script:");
> +    urlStringBuilder.appendNumber(scriptId);

Why? Shouldn't the baseURL be provided by the user as well? The first append should be appendLiteral instead

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:240
> +        String::fromUTF8(script->parent.priv->source.data(), script->parent.priv->source.length()),

Don't use parent.priv, use a cast instead and ->priv directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:241
> +        URL { URL { }, urlStringBuilder.toString() },

I don't understand this, this is the base URL as provided by the user.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:252
> +    _WebKitUserStyleSheetPrivate()
> +        : level(WEBKIT_USER_STYLE_LEVEL_USER) { }

Unneeded too

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:274
> +    userContent->priv->source = g_strdup(source);

This is a leak, CString doesn't adopt the pointer, but duplicates the string already.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:311
> +UserStyleSheet webkitWebKitUserStyleSheetToUserStyleSheet(WebKitUserStyleSheet* styleSheet, guint64 styleSheetId)
> +{
> +    g_assert(WEBKIT_IS_USER_STYLE_SHEET(styleSheet));
> +
> +    StringBuilder urlStringBuilder;
> +    urlStringBuilder.append("user-style-sheet:");
> +    urlStringBuilder.appendNumber(styleSheetId);
> +
> +    return UserStyleSheet {
> +        String::fromUTF8(styleSheet->parent.priv->source.data(), styleSheet->parent.priv->source.length()),
> +        URL { URL { }, urlStringBuilder.toString() },
> +        styleSheet->parent.priv->whitelist,
> +        styleSheet->parent.priv->blacklist,
> +        toUserContentInjectedFrames(styleSheet->parent.priv->injectedFrames),
> +        toUserStyleLevel(styleSheet->priv->level)
> +    };

Same comments here than for user script

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

Use <WebCore/Foo.h> for WebCore includes

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:36
> +class ScriptMessageClientGtk : public WebScriptMessageHandler::Client {

Mark the class as final

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:39
> +    ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager,
> +        const String& handlerName): m_handlerName(handlerName), m_manager(manager) { }

leave the parameters in the same line and move the initializations to new lines

ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const String& handlerName)
    : m_handlerName(handlerName)
    , m_manager(manager)
{
}

Also we should receive the pointer of WebKitUserContentManager

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:60
> +    guint64 numUserStyleSheets;
> +    guint64 numUserScripts;

Use uint64_t for internal types and fooCount instead of numFoo.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:139
> +    manager->priv->userContentController->addUserScript(
> +        webkitWebKitUserScriptToUserScript(script, manager->priv->numUserScripts++));

I think we should either wrap the WebCore::UserScript in WebKitUserScript and return a const reference here or build the WebCore::UserScript here using the WebKitUserScript public interface

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:164
> +    manager->priv->userContentController->addUserStyleSheet(
> +        webkitWebKitUserStyleSheetToUserStyleSheet(stylesheet, manager->priv->numUserStyleSheets++));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:181
> + * webkit_user_content_manager_register_script_message_handler_name:

I would remove the _name webkit_user_content_manager_register_script_message_handler()

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:202
> + * Returns: Whether the message channel was registered successfully.

You should explain why this can fail, and that it's not allowed to register the same name twice.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:207
> +    g_return_val_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager), FALSE);
> +

We should also check the name is != NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:214
> + * webkit_user_content_manager_unregister_script_message_handler_name:

Same here webkit_user_content_manager_unregister_script_message_handler()

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.h:62
> +    void (*_webkit_reserved0) (void);
> +    void (*_webkit_reserved1) (void);
> +    void (*_webkit_reserved2) (void);
> +    void (*_webkit_reserved3) (void);
> +    void (*_webkit_reserved4) (void);
> +    void (*_webkit_reserved5) (void);
> +    void (*_webkit_reserved6) (void);
> +    void (*_webkit_reserved7) (void);

I don't think we need all this padding, probably 4 is enough, considering that we don't have vmethods.

[Adrian Perez]

Created attachment 233651 [details]
Patch

[Adrian Perez]

(In reply to comment #5)
> (From update of attachment 232952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232952&action=review
> 
> Looks good so far. Removing the review flags since this is a wip patch.

\o/

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:83
> > +    CString source;
> > +    Vector<String> whitelist;
> > +    Vector<String> blacklist;
> > +    WebKitUserContentInjectedFrames injectedFrames;
> 
> hmm, the problem of having this common class is that we can't wrap the WebCore classes directly, or maybe we could use an union or something like that.

I have changed this to use an union in _WebKitUserContentPrivate, and
then wrapping the WebCore classes directly.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:97
> > +void webkit_user_content_set_source(WebKitUserContent* user_content, const gchar* source)
> 
> I'm not sure about having set/get for all members, some of them (if not all) should probably be construct only, passed to the new method and then readonly. You should not be able to modify them once the object is created, I think.

You are right, it seems sensible that WebKitUser{Script,StyleSheet} are
are created and then they can be inspected (e.g. they have getters) but
not modified (e.g. they do not have setters). The WebCore classes are done
that way and I have modified our public API to have only getters as well.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:202
> > + * @source: (transfer none): Source code of the user script.
> 
> I think you don't need to use the transfer none annotation for parameters that are const

True, you are right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:231
> > +UserScript webkitWebKitUserScriptToUserScript(WebKitUserScript* script, guint64 scriptId)
> 
> Why do we need a script id?

The script identifier is used internally in WebCore to build a fake URI
that can then be used to reference the user script. In practice that is
used only to remove an specific script (or style sheet), but there are
no IPC messages for that operation, so there is not much point in trying
to provide different URIs for each script — if I am understanding the
WebCore code correctly.

As it seems we will not be needing the URIs, I have gotten rid of the
script identifiers (same for style sheets) and all the URIs are set to
empty values.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:237
> > +    StringBuilder urlStringBuilder;
> > +    urlStringBuilder.append("user-script:");
> > +    urlStringBuilder.appendNumber(scriptId);
> 
> Why? Shouldn't the baseURL be provided by the user as well? The first append should be appendLiteral instead

No, it is used internally. Read the comment above.

> ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const String& handlerName)
>     : m_handlerName(handlerName)
>     , m_manager(manager)
> {
> }

The new version of the patch has this now implemented, but I am not very
happy of using webkit_web_context_get_default() in the code... Probably
I will try to come up with something that won't choke if we ever have
support for multiple contexts.

[Carlos Garcia Campos]

Comment on attachment 233651 [details]
Patch

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

Looks good in general, I think we can simplify it by not exposing getters for now. We still need unit tests! :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:32
> +
> +

Two empty lines here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:46
> +
> +

We usually leave only one empty line between functions.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:140
> +        UserScript* script;
> +        UserStyleSheet* styleSheet;

Isn't it possible to use std::unique_ptr here?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:158
> + * Obtains a copy of the source for an user content object. The returned
> + * string must be freed using g_free().
> + *
> + * Returns: (transfer full): An UTF-8 string.

Why returning a copy? It would be more convenient to return a const char*

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:183
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:218
> +    for (auto str = *strv; str != nullptr; ++str)

Don't compare to nullptr

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:236
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:248
> +    switch (userContent->priv->type) {
> +    case UserContentScript:
> +        return toStrV(userContent->priv->script->whitelist());
> +    case UserContentStyleSheet:
> +        return toStrV(userContent->priv->styleSheet->whitelist());
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return nullptr;

I'm actually wondering how useful it would be to add all those getters, I think user content are objects that the user creates to inject them, but that are not needed to be queried. In the current API they are parameters of a function. I think we could simplify this by avoinfing all the getters for now, and if they are needed eventually, we can add them later.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:283
> +struct _WebKitUserScriptPrivate {
> +};

Do not include an empty Private struct, use G_DEFINE_TYPE instead of WEBKIT_DEFINE_TYPE if we don't really need a Private struct

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:309
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:311
> +WebKitUserScript* webkit_user_script_new(const gchar* source, WebKitUserContentInjectedFrames injectedFrames,
> +    WebKitUserScriptInjectionTime injectionTime, const char* const* whitelist, const char* const* blacklist)

This should be one line.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:313
> +    WebKitUserContent* userContent = WEBKIT_USER_CONTENT(g_object_new(WEBKIT_TYPE_USER_SCRIPT, nullptr));

You should check at least source is not NULL with a g_return macro. Are you sure we don't need the baseURI too?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:319
> +    return WEBKIT_USER_SCRIPT(userContent);

You don't need to cast again.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:340
> +struct _WebKitUserStyleSheetPrivate {
> +};

Do not include an empty Private struct, use G_DEFINE_TYPE instead of WEBKIT_DEFINE_TYPE if we don't really need a Private struct. hmm, since the common stuff is already in the parent, I wonder whether it would be ebtter to expose these as boxed types instead

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:366
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:368
> +WebKitUserStyleSheet* webkit_user_style_sheet_new(const gchar* source, WebKitUserContentInjectedFrames injectedFrames,
> +    WebKitUserStyleLevel level, const char* const* whitelist, const char* const* blacklist)

Single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:376
> +    return WEBKIT_USER_STYLE_SHEET(userContent);

Double cast, actually same comments than before :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:396
> +const UserScript& webkitWebKitUserScriptToUserScript(WebKitUserScript* userScript)

webkitUserScriptGetUserScript()

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:401
> +const UserStyleSheet& webkitWebKitUserStyleSheetToUserStyleSheet(WebKitUserStyleSheet* userStyleSheet)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.h:42
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.h:148
> + */

Since: 2.6

> 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:25
> +#include "WebKitPrivate.h"

This is also included by all Private.h headers

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:33
> +#include <WebCore/bindings/js/SerializedScriptValue.h>

Use #include <WebCore/SerializedScriptValue.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:34
> +#include <WebCore/page/UserScript.h>

#include <WebCore/UserScript.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:35
> +#include <WebCore/page/UserStyleSheet.h>

#include <WebCore/UserStyleSheet.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:42
> +
> +

Double line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:77
> +
> +

double line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:89
> +    ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const gchar* handlerName)

Use the pointer instead of const GRefPtr<WebKitUserContentManager>&

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:97
> +        auto webView = webkitWebContextGetWebViewForPage(webkit_web_context_get_default(), &page);

Yes, it's not correct to use the default web context here, use page.viewWidget() instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:99
> +        auto jsValue = WebSerializedScriptValue::create(&jsSerializedValue);
> +        auto jsResult = adoptGRef(webkitJavascriptResultCreate(webView, jsValue.get()));

Add a new constructor for WebKitJavaScriptResult that receives a WebCore::SerializedScriptValue& directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:100
> +        g_signal_emit(m_manager.get(), signals[SCRIPT_MESSAGE_RECEIVED], m_handlerName, jsResult.get());

Maybe we should also include the web view as parameter of the signal, since the manager can be shared with multiple view, we don't know which view is associated to this particular message, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:125
> +     */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:142
> + * Returns: (transfer none): a #WebKitUserContentManager

Transfer full

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:143
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:154
> + * Adds a script to an user content manager. The same #WebKitUserScript can

Adds a #WebKitUserScript to the given #WebKitUserContentManager.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:156
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:158
> +void
> +webkit_user_content_manager_add_script(WebKitUserContentManager* manager, WebKitUserScript* script)

Single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:169
> + * Removes all user scripts from the user content manager.

Removes all user scripts from @manager or from the #WebKitUserContentManager

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:170
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:172
> +void
> +webkit_user_content_manager_remove_all_scripts(WebKitUserContentManager* manager)

Single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:184
> + * Adds a script to an user content manager. The same #WebKitUserScript can
> + * be reused with multiple managers.

copy paste here, use style sheet instead of script

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:185
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:187
> +void
> +webkit_user_content_manager_add_style_sheet(WebKitUserContentManager* manager, WebKitUserStyleSheet* stylesheet)

Single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:201
> + * Removes all user style sheets from the user content manager.
> + */
> +void
> +webkit_user_content_manager_remove_all_style_sheets(WebKitUserContentManager* manager)

Same comments here

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

s/name//

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

#WebKitUserContentManager::script-message-received

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:234
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:238
> +    g_return_val_if_fail(name != nullptr, FALSE);

Do not compare to nullptr

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

handler_name -> handlerName

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:241
> +    auto handler = WebScriptMessageHandler::create(std::make_unique<ScriptMessageClientGtk>(manager, name), handler_name);

You can use String::fromUTF8(name) directly here without a local variable

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

#WebKitUserContentManager::script-message-received signal

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

s/See:/See also/

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:259
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:269
> +    g_return_val_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager), nullptr);

Don't use g_return macros in private methods, use assert instead if you want to be extra sure

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:687
> +     */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1971
> + *

You should add a brief explanation here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1973
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2018
> + * Gets the user content manager associated to @web_view.

Or %NULL if the view doesn't have a user content manager.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2021
> + * Returns: (transfer none): the #WebKitUserContentManager associated with
> + *    the view

Use a single line here.

[Adrian Perez]

I am moving user script support to a separate patch, to be tracked in
bug #134738, and reusing this one bug for the script message handler
support, which would be the last feature WebKitUserContentManager we
would be implementing in WebKitGTK for 2.6

[Adrian Perez]

Created attachment 234981 [details]
Patch

[Adrian Perez]

The new version of the patch that I have just uploaded has only the
script message handler support (user script injection was moved to
bug #134738 as a separate patch). It now includes unit tests for the
feature.

[Build Bot]

Comment on attachment 234981 [details]
Patch

Attachment 234981 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5711439666872320

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 234982 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Adrian Perez]

Created attachment 234986 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 234986 [details]
Patch

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>(manager, 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.

[Adrian Perez]

Created attachment 240219 [details]
Patch

[Adrian Perez]

(In reply to comment #15)
> Comment on attachment 234986 [details]
> Patch
> 
> 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

I believe it can be used by any loaded content, not just uses scripts,
but I am going to double check that.
 
> > 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.

True that. Now that I have rewritten this using a subclass, it looks
much, much better.
 
The rest of the review comments should be fixed in the latest version
of the patch. The only remaning issue will be that now that WebKitGTK+
2.6.2 has been released I have to change the “Since:” tags in the
documentation comments to be “Since: 2.6.3”. Also, I will be checking
if all content can access use the “...postMessage()” functions.

[Carlos Garcia Campos]

(In reply to comment #17)
> (In reply to comment #15)
> > Comment on attachment 234986 [details]
> > Patch
> > 
> > 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
> 
> I believe it can be used by any loaded content, not just uses scripts,
> but I am going to double check that.
>  
> > > 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.
> 
> True that. Now that I have rewritten this using a subclass, it looks
> much, much better.
>  
> The rest of the review comments should be fixed in the latest version
> of the patch. The only remaning issue will be that now that WebKitGTK+
> 2.6.2 has been released I have to change the “Since:” tags in the
> documentation comments to be “Since: 2.6.3”. Also, I will be checking
> if all content can access use the “...postMessage()” functions.

Since: 2.8. We don't add new API/features in stable branches, so we only use major stable versions in Since tags.

[Adrian Perez]

(In reply to comment #18)
> (In reply to comment #17)
> >
> > The rest of the review comments should be fixed in the latest version
> > of the patch. The only remaning issue will be that now that WebKitGTK+
> > 2.6.2 has been released I have to change the “Since:” tags in the
> > documentation comments to be “Since: 2.6.3”. Also, I will be checking
> > if all content can access use the “...postMessage()” functions.
> 
> Since: 2.8. We don't add new API/features in stable branches, so we only use
> major stable versions in Since tags.

Changed. Also I have made sure that the patch builds. Somehow the build
is not working with Clang 3.5.0 so I had to go back to GCC to test it.

[Carlos Garcia Campos]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > >
> > > The rest of the review comments should be fixed in the latest version
> > > of the patch. The only remaning issue will be that now that WebKitGTK+
> > > 2.6.2 has been released I have to change the “Since:” tags in the
> > > documentation comments to be “Since: 2.6.3”. Also, I will be checking
> > > if all content can access use the “...postMessage()” functions.
> > 
> > Since: 2.8. We don't add new API/features in stable branches, so we only use
> > major stable versions in Since tags.
> 
> Changed. Also I have made sure that the patch builds. Somehow the build
> is not working with Clang 3.5.0 so I had to go back to GCC to test it.

Changed where? could you upload the new version of the patch?

[Adrian Perez]

Created attachment 240479 [details]
Patch

[Adrian Perez]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > >
> > > > The rest of the review comments should be fixed in the latest version
> > > > of the patch. The only remaning issue will be that now that WebKitGTK+
> > > > 2.6.2 has been released I have to change the “Since:” tags in the
> > > > documentation comments to be “Since: 2.6.3”. Also, I will be checking
> > > > if all content can access use the “...postMessage()” functions.
> > > 
> > > Since: 2.8. We don't add new API/features in stable branches, so we only use
> > > major stable versions in Since tags.
> > 
> > Changed. Also I have made sure that the patch builds. Somehow the build
> > is not working with Clang 3.5.0 so I had to go back to GCC to test it.
> 
> Changed where? could you upload the new version of the patch?

Now it was properly updated. It seems webkit-patch failed silently the
previous time I tried to upload it, and I did not check with the browser
that Bugzilla hadn't been updated...

[Carlos Garcia Campos]

Comment on attachment 240479 [details]
Patch

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

Looks better but there are still several important issues

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:42
> +    _WebKitJavascriptResult(WebKitWebView* view, WebCore::SerializedScriptValue& serializedScriptValue)
> +        : webView(view)
> +        , referenceCount(1)
> +    {
> +        value = serializedScriptValue.deserialize(webkit_web_view_get_javascript_global_context(view), nullptr);
> +    }

Instead of duplicating this, and having to Create methods, we could leave this one, and remove the old one. The caller of the old one can use WebSerializedScriptValue::internalRepresentation() to get the WebCore::SerializedScriptValue

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:82
> +     * @js_result: the #WebKitJavascriptResult holding the value received
> +     *     from the JavaScript world.

Leave this in one line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:89
> +     * Since: 2.6

2.8

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:190
> +        auto jsResult = adoptGRef(webkitJavascriptResultCreate(WEBKIT_WEB_VIEW(page.viewWidget()), serializedScriptValue));

You can't use adoptGref with a WebKitJavaScriptResult, because it's not a GObject, but a refcounted boxed type. Also, use WebKitJavaScriptResult* instead, I think the auto here confuses more than helps.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:191
> +        g_signal_emit(m_manager.get(), signals[SCRIPT_MESSAGE_RECEIVED], m_handlerName, jsResult.get());

jsResult.get() -> jsResult And then you need to explicitly call webkit_java_script_result_unref(jsResult);

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:198
> +    GRefPtr<WebKitUserContentManager> m_manager;

This is creating a circular dependency. The WebKitUserContentManager creates the WebUserContentControllerProxy and keeps a reference. This client is created when a message is registered, and passed to the WebUserContentControllerProxy as part of a WebScriptMessageHandler object. The WebScriptMessageHandler is added to the WebUserContentControllerProxy map, that also keeps a reference. So, when the WebUserContentControllerProxy is destroyed the map releases all references and the ScriptMessageClientGtk is released. But that doesn't happen because the ScriptMessageClientGtk keeps a reference to WebKitUserContentManager that prevents the WebUserContentControllerProxy from being destroyed. This only works if the user explicitly unregisters the message handler, because the WebScriptMessageHandler is removed from the WebUserContentControllerProxy map, and the ScriptMessageClientGtk is destroyed releasing the WebKitUserContentManager reference.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:207
> + * registered, scripts can use `window.webkit.<name>.postMessage(value)`

window.webkit.<name>.postMessage(value) -> window.webkit.messageHandlers.<name>.postMessage(value)

Shouldn't you use &lt &gt here too?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:226
> + * Returns: Whether the message channel was registered successfully.

%TRUE if the message handler was registered successfully, or %FALSE otherwise

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:230
> +gboolean webkit_user_content_manager_register_script_message_handler(WebKitUserContentManager* manager, const gchar* name)

Use char here instead of gchar, we use the glib types only in the public headers.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:256
> +void webkit_user_content_manager_unregister_script_message_handler(WebKitUserContentManager* manager, const gchar* name)

gchar -> char

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:258
> +    g_return_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager));

You should also check name is not NULL

> Source/cmake/OptionsGTK.cmake:148
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_USER_MESSAGE_HANDLERS ON)

This is not enough, you need to pass this value somehow to the build. You can either add a explicit -DENABLE_USER_MESSAGE_HANDLERS, or better add a #define to Source/cmakeconfig.h.cmake

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:2
> - * Copyright (C) 2013 Igalia S.L.
> + * Copyright (C) 2014 Igalia S.L.

This should probably be 2013,2014 or 2013-2014

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:59
> +        test->m_userScriptMessage = jsResult;

You need to unref any previous value and ref the given one manually.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:62
> +    bool registerHandler(const gchar* handlerName)

gchar -> char

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:66
> +        if (!g_signal_connect(m_userContentManager.get(), signalName.get(), G_CALLBACK(scriptMessageReceived), this))
> +            return false;

How can this fail? the class adds the signal, it will always be found, unless the caller uses a wrong signal name, but we are the caller. Also, nobody is checking the return value of registerHandler, so better make it void..

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:68
> +        webkit_user_content_manager_register_script_message_handler(m_userContentManager.get(), handlerName);
> +        return true;

webkit_user_content_manager_register_script_message_handler can indeed fail, if the handler is already register, so maybe it makes sense to keep this method bool, but using the return value of webkit_user_content_manager_register_script_message_handler.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:81
> +    void waitUntilMessageReceived(const gchar* handlerName)
> +    {
> +        GUniquePtr<gchar> signalName(g_strdup_printf("script-message-received::%s", handlerName));
> +        g_signal_connect(m_userContentManager.get(), signalName.get(), G_CALLBACK(scriptMessageReceivedStopMainLoop), this);
> +        g_main_loop_run(m_mainLoop);

Instead of connecting to the signal twice, I think we can just connect here only, and in the same callback we set the value and stop the main loop. Also this is racy, because runJavaScriptAndWaitUntilFinished, runs its own main loop to wait for the js result, so at this point the message could have been sent already, and we are connecting late. We could rename this as postMessageAndWaitUntilReceived(). Then we first connect to the signal, and then we run the javascript, but using webkit_web_view_run_javascript directly, without using any callback nor waiting for the result. Then we run our main loop. If the js fails, the message will not be received and the test will timeout anyway.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:84
> +    GRefPtr<WebKitJavascriptResult> m_userScriptMessage;

You can't use GRefPtr with WebKitJavascriptResult, since it's not a GObject, but a refocunted boxed type.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:258
> +    GRefPtr<WebKitJavascriptResult> message;

This is unused and wrong, since WebKitJavascriptResult is not a GObject.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:260
> +    g_assert(test->registerHandler("msg"));

We should also check that registerHandler fails when trying to register the same name twice.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:273
> +    javascriptResult = test->runJavaScriptAndWaitUntilFinished("window.webkit.msg ? 'y' : 'n';", &error.outPtr());

window.webkit.msg -> window.webkit.messageHandlers.msg

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:284
> +    // Call "window.webkit.msg.postMessage()" and check that the flag was flipped.
> +    javascriptResult = test->runJavaScriptAndWaitUntilFinished("window.webkit.msg.postMessage('user message');", &error.outPtr());
> +    g_assert(javascriptResult);
> +    g_assert(!error.get());
> +
> +    test->waitUntilMessageReceived("msg");

This could be merged as I suggested.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:305
> +    UserScriptMessageTest::add("WebKitUserContentManager", "script-message-received", testUserContentManagerScriptMessageReceived);

webkit_user_content_manager_unregister_script_message_handler() is not covered by the tests, we should test that too.

[Adrian Perez]

Created attachment 240561 [details]
Patch

[Adrian Perez]

I have updated with all the fixes, and hopefully now this should
be ready for landing. There is one test case in which I have left
the code commented out with a TODO to uncomment it when bug #138142
gets fixed and unregistering and re-registering a handler with
the same name works as expected in WebCore.

[Carlos Garcia Campos]

Comment on attachment 240561 [details]
Patch

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

Looks great,  this is new API, so wait until anoother GTK+ reviewer checks the new API before landing the patch.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:266
> +        g_assert(m_userScriptMessage);
> +        WebKitJavascriptResult* result = m_userScriptMessage;
> +        m_userScriptMessage = nullptr;
> +        return result;

I think it would be better if the class keeps the reference of the result, so that we don't need to call unref in the caller all the time. The value will be reset every time a new message is posted. You would need to also unref the result in the destructor.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:277
> +    // Trying to register the same handler a second time must fail

Nit: finish comments with period.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:331
> +    // TODO Enable after https://bugs.webkit.org/show_bug.cgi?id=138142 gets fixed.

Use FIXME: instead of TODO

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:344
> +#if 0
> +    test->unregisterHandler("msg");
> +    javascriptResult = test->runJavaScriptAndWaitUntilFinished("window.webkit.messageHandlers.msg.postMessage('42');", &error.outPtr());
> +    g_assert(!javascriptResult);
> +    g_assert(error.get());
> +
> +    // Re-registering a handler that has been unregistered must work
> +    g_assert(test->registerHandler("msg"));
> +    message = test->postMessageAndWaitUntilReceived("msg", "'handler: msg'");
> +    valueString.reset(WebViewTest::javascriptResultToCString(message));
> +    webkit_javascript_result_unref(message);
> +    g_assert_cmpstr(valueString.get(), ==, "handler: msg");
> +#endif

You should unregister the handlers anyway, even if you don't test that messages shouldn't work yet.

[Adrian Perez]

Created attachment 240666 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 240666 [details]
Patch

cq- only because this requires approval from another GTK+ reviewer.

[Gustavo Noronha (kov)]

Comment on attachment 240666 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 240666 [details]
Patch

Clearing flags on attachment: 240666

Committed r175414: <http://trac.webkit.org/changeset/175414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.