Bug 243492

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.

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.

Well Option 2 would work too, but it's not very elegant. My preference is Option 3.

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

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

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.

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

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

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

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.

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

Committed 257016@main (9267215ef0d1): <https://commits.webkit.org/257016@main> Reviewed commits have been landed. Closing PR #3341 and removing active labels.

<rdar://problem/102663208>