Bug 173094 - [GTK] Get rid of custom marshallers of signals
Summary: [GTK] Get rid of custom marshallers of signals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2017-06-08 05:15 PDT by Carlos Garcia Campos
Modified: 2017-06-08 22:22 PDT (History)
4 users (show)

See Also:


Attachments
Patch (32.49 KB, patch)
2017-06-08 05:17 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-06-08 05:15:13 PDT
We can simply use g_cclosure_marshal_generic instead.
Comment 1 Carlos Garcia Campos 2017-06-08 05:17:37 PDT
Created attachment 312293 [details]
Patch
Comment 2 Adrian Perez 2017-06-08 06:27:03 PDT
Comment on attachment 312293 [details]
Patch

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

Informal r+ from me.

There is a chance that the generic libffi-based marshaller is slower than the
custom ones, but it should not be much of an issue because other parts of the
WebKit code (e.g. rendering) have more weight on CPU usage. And it is nice to
have code getting removed \o/

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:87
> +        g_cclosure_marshal_generic,

You could as well pass “nullptr” here. According to the documentation for “g_signal_new()”
(https://developer.gnome.org/gobject/stable/gobject-Signals.html#g-signal-new):

> If c_marshaller is NULL, g_cclosure_marshal_generic() will be used as the
> marshaller for this signal.

And the same goes to the other changed locations. I have no strong opinion about whether
passing “g_cclosure_marshal_generic” or “nullptr” should be preferred, so I'd leave it
up to you.
Comment 3 Carlos Garcia Campos 2017-06-08 06:33:01 PDT
(In reply to Adrian Perez from comment #2)
> Comment on attachment 312293 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312293&action=review
> 
> Informal r+ from me.

Thanks.

> There is a chance that the generic libffi-based marshaller is slower than the
> custom ones, but it should not be much of an issue because other parts of the
> WebKit code (e.g. rendering) have more weight on CPU usage. And it is nice to
> have code getting removed \o/

Yes, I don't think it will be noticeable and we are using generic already in other signals.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:87
> > +        g_cclosure_marshal_generic,
> 
> You could as well pass “nullptr” here. According to the documentation for
> “g_signal_new()”
> (https://developer.gnome.org/gobject/stable/gobject-Signals.html#g-signal-
> new):
> 
> > If c_marshaller is NULL, g_cclosure_marshal_generic() will be used as the
> > marshaller for this signal.
> 
> And the same goes to the other changed locations. I have no strong opinion
> about whether
> passing “g_cclosure_marshal_generic” or “nullptr” should be preferred, so
> I'd leave it
> up to you.

g_signal_new is already difficult to read with severl 0, nullptr, nullptr. In some cases we even add a comment explaining what the nullptr/0 is. So, I prefer to pass the marshaller for readability.
Comment 4 Zan Dobersek 2017-06-08 06:38:38 PDT
Comment on attachment 312293 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:87
>>> +        g_cclosure_marshal_generic,
>> 
>> You could as well pass “nullptr” here. According to the documentation for “g_signal_new()”
>> (https://developer.gnome.org/gobject/stable/gobject-Signals.html#g-signal-new):
> 
> g_signal_new is already difficult to read with severl 0, nullptr, nullptr. In some cases we even add a comment explaining what the nullptr/0 is. So, I prefer to pass the marshaller for readability.

SGTM. Let's be explicit where possible.
Comment 5 Michael Catanzaro 2017-06-08 08:21:18 PDT
Good cleanup.

(In reply to Carlos Garcia Campos from comment #3)
> g_signal_new is already difficult to read with severl 0, nullptr, nullptr.
> In some cases we even add a comment explaining what the nullptr/0 is. So, I
> prefer to pass the marshaller for readability.

Well it's up to you, but I prefer the NULL/nullptr style as well. It's simpler than writing out g_cclosure_marshal_generic, and I don't think it hurts readability. Here nullptr means "keep things as the default" and that's all you need to know. I converted Epiphany to use this style a year or two ago.
Comment 6 Carlos Garcia Campos 2017-06-08 22:22:50 PDT
Committed r217960: <http://trac.webkit.org/changeset/217960>