Bug 158942 – [GTK] Add support for variadic parameters to GObject DOM bindings

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

[Michael Catanzaro]

webkit_dom_dom_token_list_remove has disappeared from our unstable API. It's not technically a WebKit bug as this was unstable API, but it's required by Epiphany, so GNOME will be stuck on old WebKit until this gets sorted out.

I've attempted to track which commit is to blame with a quick wiki search for "DOMTokenList" but couldn't find any suspicious commit. The change is quite recent (since 2.13.1).

[Carlos Garcia Campos]

Chris, any idea why this is no longer generated? I guess it's using a new idl thing that we don't support in the gobject bindings.

[Chris Dumez]

Looks like a regression from Bug 158529. Changelog says:
"Functions with variadic parameters are skipped for ObjC and GObject code generators."

DOMTokenList.remove() is variadic.

[Carlos Garcia Campos]

(In reply to comment #2)
> Looks like a regression from Bug 158529. Changelog says:
> "Functions with variadic parameters are skipped for ObjC and GObject code
> generators."
> 
> DOMTokenList.remove() is variadic.

Indeed, thank you. I'll try to find some time to fix then.

[Nael Ouedraogo]

I skipped variadic functions in Bug 158529 for GObject bindings because I thought variadic parameters were not correctly handled.

"return 1 if $param->isVariadic;" line from CodeGeneratorGObject.pm can be removed to restore webkit_dom_dom_token_list_remove and webkit_dom_dom_token_list_add functions. I can upload a patch if needed.

However, the generated code seems to process only the first token passed in argument of the function.

Which tests should be run to test such kinds of regression?

[Michael Catanzaro]

(In reply to comment #4)
> I skipped variadic functions in Bug 158529 for GObject bindings because I
> thought variadic parameters were not correctly handled.
> 
> "return 1 if $param->isVariadic;" line from CodeGeneratorGObject.pm can be
> removed to restore webkit_dom_dom_token_list_remove and
> webkit_dom_dom_token_list_add functions. I can upload a patch if needed.
> 
> However, the generated code seems to process only the first token passed in
> argument of the function.

Yeah, the generated functions were not variadic. I guess you would have to call the function once for each argument to get the desired effect. The original declaration, present in 2.12, was this:

/**
 * webkit_dom_dom_token_list_remove:
 * @self: A #WebKitDOMDOMTokenList
 * @tokens: A #gchar
 * @error: #GError
 *
 * Stability: Unstable
**/
WEBKIT_API void
webkit_dom_dom_token_list_remove(WebKitDOMDOMTokenList* self, const gchar* tokens, GError** error);

The GError parameter disappeared in trunk a month or two ago. Not a problem, it's unstable API, after all.

Of course, it would be best to generate real variadic functions, but reverting to this form would suffice for Epiphany.

> Which tests should be run to test such kinds of regression?

We have GObject API break tests that would have caught this if it was stable API. We intentionally don't run the tests for the unstable API, because we don't guarantee this API will continue to exist. The problem is that we have two GNOME applications -- Epiphany and Yelp -- that are unfortunately using the unstable API, and Epiphany used this function in particular. The ideal solution would be for them to use only the stable API, but I'm not sure if it's possible.

[Michael Catanzaro]

(In reply to comment #4)
> Which tests should be run to test such kinds of regression?

It's Tools/gtk/check-for-webkitdom-api-breaks, but again, it would not have caught this, because this was not stable API.

[Nael Ouedraogo]

Created attachment 281736 [details]
Patch

[Nael Ouedraogo]

GObject code generator is modified in uploaded patch to skip functions with variadic parameters except webkit_dom_dom_token_list_remove and webkit_dom_dom_token_list_add.

The signatures of the generated functions match signature of v2.12. A follow-up bug may be added to implement a correct support of variadics parameters in GObject bindings.

[youenn fablet]

Comment on attachment 281736 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:334
> +            return 1 if $param->isVariadic;

In terms of style, I would do:
return 0 if ($functionName...)
Return 1 if (variadic...)

But also, can we define not-variadic add() and remove() methods only for GObject, using something like #if defined(LANGUAGE_GOBJECT)?

[Nael Ouedraogo]

Created attachment 281753 [details]
Patch

[Nael Ouedraogo]

Thanks Youenn for the review.

Add() and remove() functions have been defined (only for GObject) without variadic parameters in the uploaded patch as proposed in your comment. 

I checked that webkit_dom_dom_token_list_remove and webkit_dom_dom_token_list_add are correctly generated.

[Carlos Garcia Campos]

Comment on attachment 281753 [details]
Patch

Thanks for the patch, but the unstable API is expected to be broken so it's not a problem at all. Apps should be updated, the actual problem is that there's no replacement, so I think we should add support for variadic args to GObject bindings and update the apps to use the new API. I have a wip patch to do that, I'll submit it as soon as I finish it

[Carlos Garcia Campos]

Created attachment 281756 [details]
Patch

[Michael Catanzaro]

Comment on attachment 281756 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:905
> + * @...: list of #gchar ended by %NULL.

A small part of me wants to say "no! you must cast the NULL pointer when using it in a variadic function call!" But I know platforms where the width of NULL is not the same as the width of a pointer are never going to be able to run WebKit or maybe even GObject, so OK.

> Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:922
> +webkit_dom_test_obj_variadic_double_method(WebKitDOMTestObj* self, gdouble head, guint n_tail, ...);

You prefer this way to putting n_tail before head? So that you can pass 0 for n_tail and that be the final argument to the function?

[Carlos Garcia Campos]

(In reply to comment #14)
> Comment on attachment 281756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281756&action=review
> 
> > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:905
> > + * @...: list of #gchar ended by %NULL.
> 
> A small part of me wants to say "no! you must cast the NULL pointer when
> using it in a variadic function call!" But I know platforms where the width
> of NULL is not the same as the width of a pointer are never going to be able
> to run WebKit or maybe even GObject, so OK.
> 
> > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:922
> > +webkit_dom_test_obj_variadic_double_method(WebKitDOMTestObj* self, gdouble head, guint n_tail, ...);
> 
> You prefer this way to putting n_tail before head? So that you can pass 0
> for n_tail and that be the final argument to the function?

This is generated code, we don't really know what the other parameters are, we just need to ensure ... is the last one, and in case it's not a pointer type, also include a n_params parameter to know how many params will be provided. In that case, yes, of course, n_params and ... should be together.

[Carlos Garcia Campos]

Committed r202328: <http://trac.webkit.org/changeset/202328>