Bug 173094 – [GTK] Get rid of custom marshallers of signals

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We can simply use g_cclosure_marshal_generic instead.

[Carlos Garcia Campos]

Created attachment 312293 [details]
Patch

[Adrian Perez]

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.

[Carlos Garcia Campos]

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

[Zan Dobersek]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Committed r217960: <http://trac.webkit.org/changeset/217960>