[Webkit-unassigned] [Bug 202847] [GTK][WPE] Add user messages API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=202847

--- Comment #6 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 380746
  --> https://bugs.webkit.org/attachment.cgi?id=380746
Patch

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

This looks really great, thanks for working on it, Carlos! I have
some use-cases in mind where this will come in very handy and allow
removing custom-written IPC code from applications. Kudos for reusing
GVariant to carry data around, and for future-proofing the feature by
allowing passing file descriptors around.

I just have one small nit/doubt/suggestion below :)

> Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:162
> +     * after the message has been sent.

Maybe we want to explicitly have a note indicating that...

  “If the sender needs to keep the file descriptor, `dup(2)`
   can be used to send a new descriptor referring to the same
   data.”

I suggest this because my first thought was “what if I need to keep
referring to some shared data in both processes” and it took me a
minute to realize how to do it. Maybe for many people it's obvious,
but it won't hurt pointing it out, I think.

> Source/WebKit/Shared/glib/UserMessage.cpp:50
> +        GRefPtr<GDBusMessage> message = adoptGRef(g_dbus_message_new_method_call(nullptr, "/", nullptr, "_"));

Is there any advantage here in abusing GDBusMessage over sending
directly a blob of bytes with the GVariant type and the payload?
Roughly, I am thinking of something like this:

  const GvariantType *payloadType = g_variant_get_type(parameters.get());
  encoder << IPC::DataReference(g_variant_type_peek_string(payloadType),
                                g_variant_type_get_string_length(payloadType));
  encoder << IPC::DataReference(g_variant_get_data(parameters.get(),
                                g_variant_get_size(parameters.get());

This doesn't incur in any allocations in the best case, and only one in the
worst case for the buffer where the GVariant gets serialized on demand if
needed. Using GDBusMessage involves an additional GObject (and its allocations
and deallocations), for no added benefit (I think).

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2105
> +     * and the message has been replied, the operation in the #WebKitWebPage will

>From the context I suppose that this is supposed to be

  “…and the message has not been replied…”
                        ^^^

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191011/b44bc52c/attachment.html>