Bug 101079 – [GTK] Clean up g_return macros usage in GObject DOM bindings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

g_return macros should be used to check parameters of public API methods. For DOM bindings GObjects types, it would be better to use the GObject macros to check the type in the glib type system, and not only if it's NULL or not. Return value of g_return_val_if_fail() for boolean methods should be FALSE instead of 0. We should also protect GError parameters with g_return macros.

[Carlos Garcia Campos]

Created attachment 172098 [details]
Patch

[Carlos Garcia Campos]

Created attachment 172108 [details]
Updated patch to include test results

[Xan Lopez]

Comment on attachment 172108 [details]
Updated patch to include test results

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

Looks mostly good to me, I just have a couple of questions.

> Source/WebCore/ChangeLog:11
> +          - Use them only to check parameters of public API.

Not trying to get into a philosophical debate here, and it's really not so relevant for this bug because I basically agree with the changes, but I don't see this as a good practice per se. The good practice about g_return macros is to use them in public API instead of ASSERTs or no checks at all. The reason for that is that we cannot control what the user gives us, so it makes sense to check things but it does not make sense to crash on error. That being said, using g_return macros internally makes sense for situations where you want to fail, but do not want to crash (ie, for situations that are neither a programmer's error nor so critical as to justify just giving up and crashing the program).

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1233
> +    return request ? request->priv->coreObject.get() : 0;

This seems consistent with what we do in most places (but not all!) in WebKitGTK+, so I think that's OK.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1245
> +    ASSERT(coreObject);

Any reason why this should be different than ::core or ::kit?

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1346
> +        return 0;

Same comment than in ::core..

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 172108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172108&action=review
> 
> Looks mostly good to me, I just have a couple of questions.
> 
> > Source/WebCore/ChangeLog:11
> > +          - Use them only to check parameters of public API.
> 
> Not trying to get into a philosophical debate here, and it's really not so relevant for this bug because I basically agree with the changes, but I don't see this as a good practice per se. The good practice about g_return macros is to use them in public API instead of ASSERTs or no checks at all. The reason for that is that we cannot control what the user gives us, so it makes sense to check things but it does not make sense to crash on error. That being said, using g_return macros internally makes sense for situations where you want to fail, but do not want to crash (ie, for situations that are neither a programmer's error nor so critical as to justify just giving up and crashing the program).

The main reason to not use the g_return macros internally is because they can be disabled (it's not very common, but it possible). If something is never expected to fail, then I think it's better to crash because the program is likely to crash sooner or later anyway, and it helps to debug the issue. If something is expected to fail we can simply return early and avoid the overhead of the g_return macros or a crash if they are disabled. See the glib documentation:

"The g_return family of macros (g_return_if_fail(), g_return_val_if_fail(), g_return_if_reached(), g_return_val_if_reached()) should only be used for programming errors, a typical use case is checking for invalid parameters at the beginning of a public function. They should not be used if you just mean "if (error) return", they should only be used if you mean "if (bug in program) return". The program behavior is generally considered undefined after one of these checks fails. They are not intended for normal control flow, only to give a perhaps-helpful warning before giving up."

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1233
> > +    return request ? request->priv->coreObject.get() : 0;
> 
> This seems consistent with what we do in most places (but not all!) in WebKitGTK+, so I think that's OK.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1245
> > +    ASSERT(coreObject);
> 
> Any reason why this should be different than ::core or ::kit?

Yes, in core or kit NULL is a valid expected value. For example, a method that returns a core element can return NULL in case of exception, the conversion to a WebKit object in this case is NULL. Here, we never expect NULL for coreObject, it's a construct only property of WebKitDOMObject and the program is going to crash sooner or later, because we are assuming coreObject is a valid pointer everywhere. 

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1346
> > +        return 0;
> 
> Same comment than in ::core..

[Xan Lopez]

Comment on attachment 172108 [details]
Updated patch to include test results

OK, let's get this in.

[Carlos Garcia Campos]

Committed r137074: <http://trac.webkit.org/changeset/137074>