Bug 130174 – [GTK] Fix unused parameter warnings in the GObject WebKitDOM bindings

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

[Martin Robinson]

These warnings are prevalent when building with CMake.

[Martin Robinson]

Created attachment 226572 [details]
Patch

[Daniel Bates]

Comment on attachment 226572 [details]
Patch

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

OK.

> Source/WebCore/bindings/gobject/WebKitDOMObject.cpp:28
> +    switch (propertyId) {
>      default:
> -        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propertyId, pspec);
>          break;
>      }

The switch statement is unnecessary as we always execute the code for the default case.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:661
> +        push(@txtGetProps, "#else\n") if $conditionalString;
> +        push(@txtGetProps, "    UNUSED_PARAM(value);\n") if $conditionalString;
> +        push(@txtGetProps, "${conditionGuardEnd}\n") if $conditionalString;

Unless you feel the duplication of the if conditional ("if $conditionalString") improves the readability of this code, I would write this as:

if ($conditionalString) {
    push(@txtGetProps, "#else\n");
    push(@txtGetProps, "    UNUSED_PARAM(value);\n");
    push(@txtGetProps, "${conditionGuardEnd}\n");
}

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:674
> +            push(@txtSetProps, "#else\n") if $conditionalString;
> +            push(@txtSetProps, "    UNUSED_PARAM(value);\n") if $conditionalString;
> +            push(@txtSetProps, "${conditionGuardEnd}\n") if $conditionalString;

Similarly, I would write this as:

if ($conditionalString) {
    push(@txtSetProps, "#else\n");
    push(@txtSetProps, "    UNUSED_PARAM(value);\n");
    push(@txtSetProps, "${conditionGuardEnd}\n");
}

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:797
> +        push(@cBodyProperties, "UNUSED_PARAM(request);\n");

Nit: We should indent "UNUSED_PARAM(request);".

> Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNode.cpp:101
> +UNUSED_PARAM(request);

Nit: This line should be indented 4 spaces.

[Carlos Garcia Campos]

Comment on attachment 226572 [details]
Patch

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

Thank you!

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:59
> -static void weakRefNotify(gpointer data, GObject* zombie)
> +static void weakRefNotify(gpointer data, GObject* /* zombie */)

I think e can simply avoid the parameter name in this case.

>> Source/WebCore/bindings/gobject/WebKitDOMObject.cpp:28
>>      }
> 
> The switch statement is unnecessary as we always execute the code for the default case.

The whole function is unnecessary :-) We can simply remove it and not give any imp for GObjectClass::get_property, since the only property this object has is not readable.

[Martin Robinson]

Comment on attachment 226572 [details]
Patch

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

>>> Source/WebCore/bindings/gobject/WebKitDOMObject.cpp:28
>>>      }
>> 
>> The switch statement is unnecessary as we always execute the code for the default case.
> 
> The whole function is unnecessary :-) We can simply remove it and not give any imp for GObjectClass::get_property, since the only property this object has is not readable.

I hope you don't mind if I don't take the suggestion to remove the method entirely. I don't have time to rebuild and test right now, but I want to land this get it off my plate. The DOM bindings and code generator need *a lot* of cleanup, so we should definitely think about running through them again pre-3.0.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 226572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226572&action=review
> 
> >>> Source/WebCore/bindings/gobject/WebKitDOMObject.cpp:28
> >>>      }
> >> 
> >> The switch statement is unnecessary as we always execute the code for the default case.
> > 
> > The whole function is unnecessary :-) We can simply remove it and not give any imp for GObjectClass::get_property, since the only property this object has is not readable.
> 
> I hope you don't mind if I don't take the suggestion to remove the method entirely. I don't have time to rebuild and test right now, but I want to land this get it off my plate. The DOM bindings and code generator need *a lot* of cleanup, so we should definitely think about running through them again pre-3.0.

Sure, go ahead and fix the warnings :-)

[Martin Robinson]

Committed r165618: <http://trac.webkit.org/changeset/165618>