Bug 243492 – [GTK][WPE] Support asynchronously returning values from user script messages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Adrian Perez]

The initial implementation of user script messages was added in
bug #133730 (it feels like ages ago!). Currently script messages
handlers never return values, but these days we have support in
WebKit which allows returbning values asynchronously, by means
of a returned Promise object. We would like to expose this
functionality in the public API.

The Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp
file contains the implementation of the ScriptMessageClientGtk
class (around line 227), which needs to be modified to indicate
that it supports asynchronous replies. An implementation of
the didPostMessageWithAsyncReply() method needs to be filled in.

The tricky part here is maintaining API/ABI compatibility with
older release of the GTK/WPE ports, while keeping the changes
as small as possible -- I will add some ideas about this in a
follow comment later.

[Adrian Perez]

In order to maintain API/ABI compatibility we are allowed to:

 - Add new C functions.
 - Add parameters to a C function only if it doesn't cause a leak.
 - Add a return value to a C function returning void only if it doesn't cause a leak.
 - Add/remove modifiers (like “const”), as they are not part of the C ABI.

In all cases we will need a new function that registers a new message
handler *with* async replies enabled. Something like:

  WEBKIT_API void
  webkit_user_content_manager_register_script_message_handler_with_reply (
      WebKitUserContentManager *manager, const char *name, const char *world_name);

The reason to add a “world_name” parameter is that we can allow passing
NULL to use the default, and that way we don't need a second function
that has an “_in_world” suffix in the name.

Calling this new function will also instantiate a ScriptMessageClientGtk,
object (like the existing ones) but will set a flag to indicate that it
supports async replies.


Option 1
--------

We could add a return value and a new parameter to the signature of the
WebKitUserContentManager::script-message-received signal: it would need
to be added at the *end* of the list of parameters; but the conventions
in the API mandate that the userdata pointer is always the last one.

So we cannot really do this.


Option 2
--------

Another option is to add methods to WebKitJavascriptResult that allow
providing the reply, as in:

 webkit_javascript_result_reply (WebKitJavaScriptResult *js_result,
                                 JSCValue               *reply_value,
                                 GError                 *error);

The idea is that one provides a “reply_value” or an “error”, but not both.

Then, we change the return type of the ::script-message-received from
“void” to “gboolean”. The returned value won't be used for the handlers
without reply.

For the handlers that expect a reply, returning TRUE from the signal
handler will mean “the reply will be provided later”, which allows
behaving asynchronously -- we use this same pattern for example in
other signals like WebKitWebView::permission-request.


Option 3
--------

Lastly, we could have a new signal, for example call
it WebKitUserContentManager::script-message (IMO the “-received”
suffix in the old one is redundant) and provide a new helper class,
let's say WebKitUserScriptRequest, with the following methods:

  JSCValue*
  webkit_user_script_request_get_message(WebKitUserScriptRequest *request);

  void
  webkit_user_script_request_reply(WebKitUserScriptRequest *request,
                                   JSCValue *reply_value);

  void
  webkit_user_script_request_error(WebKitUserScriptRequest *request,
                                   GError *error);

Then handlers for the new ::script-message signal would receive an
instance of WebKitUserScriptRequest, which allows accessing the message
passed to the handler from JavaScript, and supports providing a reply.
Signal handler functions would have the following signature:

  gboolean signal_handler(WebKitUserContentManager *content_manager
                          WebKitUserScriptRequest *request,
                          gpointer userdata);

Again, if a handler returns TRUE, that means that the reply will be
done later on, asynchronously.


What to do?
-----------

IMO the best option is number 3. While at it, I would also mark the
existing (old) functions and signal as deprecated: the new functionality
will be a superset of the old one. When a reply is not provided using
webkit_user_script_request_reply() by the signal handler function, we
can return the “undefined” value as the default action.

The other possible one (number 2) would leave us with an uglier API
because it's adding a method to WebKitJavascriptResult which does not
belong in that class, and also changing signatures of existing
functions/signals, while possible in this particular situation, is
quite ugly.

[Adrian Perez]

Actually, I was just reminded by Michael Catanzaro (who knows a good
deal about API/ABI breaks) that we cannot change return types, nor
change parameter names, because those end up in the introspection API
data (.gir/.typelib) and some languages which consume those have
parameter names as part of function signatures (example: Python).

Summarizing: option 3 is the only viable one.

[Michael Catanzaro]

Well Option 2 would work too, but it's not very elegant.

My preference is Option 3.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> Well Option 2 would work too, but it's not very elegant.

Ah, well, I didn't read it closely enough. Indeed, it wouldn't.

[Adrian Perez]

(In reply to Adrian Perez from comment #1)
 
> Option 3
> --------
> 
> Lastly, we could have a new signal, for example call
> it WebKitUserContentManager::script-message (IMO the “-received”
> suffix in the old one is redundant) and provide a new helper class,
> let's say WebKitUserScriptRequest, with the following methods:
>
> [...]

We may want to call the signal ::script-request-received, for consistency
with the fact that handlers will receive a WebKitUserScriptRequest object.

[Michael Catanzaro]

I'm a little uncertain whether webkit_user_script_request_get_message() is useful or not. If you put the message directly in the signal parameters instead, then applications can ignore the WebKitUserScriptRequest altogether, which is clearly better in the common case of a message that does not require a reply. Making it accessible via WebKitUserScriptRequest the application has to do one extra step to get at the JSCValue. But it also makes it easier to pass the value around with the WebKitUserScriptRequest to a different point in the application, which is possibly useful.

Anyway, this is just something to think about.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #5)
> We may want to call the signal ::script-request-received, for consistency
> with the fact that handlers will receive a WebKitUserScriptRequest object.

Or call it WebKitUserScriptMessage instead? The "message" terminology is not bad.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Adrian Perez from comment #5)
> > We may want to call the signal ::script-request-received, for consistency
> > with the fact that handlers will receive a WebKitUserScriptRequest object.
> 
> Or call it WebKitUserScriptMessage instead? The "message" terminology is not
> bad.

Yes, option 3 + WebKitUserScriptMessage name sounds good to me too.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #6)
> I'm a little uncertain whether webkit_user_script_request_get_message() is
> useful or not. If you put the message directly in the signal parameters
> instead, then applications can ignore the WebKitUserScriptRequest
> altogether, which is clearly better in the common case of a message that
> does not require a reply. Making it accessible via WebKitUserScriptRequest
> the application has to do one extra step to get at the JSCValue. But it also
> makes it easier to pass the value around with the WebKitUserScriptRequest to
> a different point in the application, which is possibly useful.
> 
> Anyway, this is just something to think about.

Currently one has to go through the WebKitJavascriptResult that gets passed
to signal handlers to obtain the JSCValue. Having a WebKitUserScriptMessage
instead which carries the JSCValue does not add additional indirections (than
the ones we already have) so I think that's fine. Also, this way the same
object instance carries both the value passed to the handler from JS code
and the methods used to reply to that one message. Passing the JSCValue
as a separate paremeter makes it a bit more gnarly to keep around both the
JSCValue and the WebKitUserScriptMessage to do async processing and send a
reply later on.

[Adrian Perez]

Here's a summary of what we will be doing, to have it in a single comment:

1. Add a new WebKitUserScriptMessage type, I think this can be a boxed
   struct type. I do not expect we will be needing to add signals or
   properties to it, so it does not need to be a full-fledged GObject.

   This type will have the following methods:

   JSCValue*
   webkit_user_script_message_get_message(WebKitUserScriptMessage *message);

   void
   webkit_user_script_message_reply(WebKitUserScriptMessage *message,
                                   JSCValue *reply_value);

   void
   webkit_user_script_message_error(WebKitUserScriptMessage *message,
                                    GError *error);

   IMO, the “error” parameter in the last function can be (transfer full)
   because I expect in most cases the error passed to this method will
   not be used anymore--it seems more convenient that the function takes
   ownwership of the error and frees it.

2. Modify ScriptMessageClientGtk so it takes a boolean in the constructor,
   indicating whether the client is working in async mode. The existing
   webkit_user_content_manager_register_script_message_handler[_in_world]()
   functions will leave this flag disabled so they continue behaving in the
   same way.

3. Add a new function to register user message handlers which supports
   sending async replies. Calling this will create a ScriptMessageClientGtk
   with the boolean flag as “true” to enable async support. The signature of
   the function will be:

   void
   webkit_user_content_manager_register_script_message_handler_with_reply (
      WebKitUserContentManager *manager, const char *name, const char *world_name);

   When “NULL” is passed as “world_name”, the default script world is used.
   That way we do not need to provide a separate _in_world() variant.

4. Add a new WebKitUserContentManager::script-request-received, with the
   following signature:

   gboolean handler(WebKitUserContentManager *content_manager
                    WebKitUserScriptMessage *message, gpointer userdata);

   Returning TRUE from a signal handler means that the reply will be done
   later. If the last reference to a WebKitUserScriptMessage is removed
   and a reply/error has not been sent, the default action would be to
   return “undefined” (this can be done in the signal handler's default
   implementation, WebKitWebView::permission-request and a few others
   works in that way already, IIRC).

5. Fill in the ScriptMessageClientGtk::didPostMessageWithAsyncReply()
   implementation, which is currently empty. This has to instantiate the
   WebKitUserScriptMessage for the request with the completion handler
   that WebKit passes to it and any other data it may need to produce
   an reply/error and emit the signal.

[Adrian Perez]

By the way, once this is done, we can deprecate the old API, I have
filed bug #243666 to do that later on =)

Pull request: https://github.com/WebKit/WebKit/pull/3341

[EWS]

Committed 257016@main (9267215ef0d1): <https://commits.webkit.org/257016@main>

Reviewed commits have been landed. Closing PR #3341 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/102663208>