Bug 130978 – [GTK] Readonly attributes installed as readwrite in GObject DOM bindings

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

[Martin Robinson]

We get this error when generating the GIR file:

GLib-GObject-CRITICAL **: g_object_class_install_property: assertion 'class->set_property != NULL' failed

This happens because even though an attribute is read-only, it is installed as read-write. This is a problem when there are no other read-write attributes and the code generator does not generate a set_property vmethod.

[Martin Robinson]

Created attachment 228185 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 228185 [details]
Patch

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

Thanks for fixing this, I always wondered where that warning came from. Please, consider my comments before landing.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:424
> +sub IsPropertyWriteable {
> +    my $property = shift;
> +    if ($property->isReadOnly) {
> +        return 0;
> +    }

I know we are not checking if the attribute is skipped, because this is always called for readable properties, but it's confusing, because the name IsPropertyWriteable, like IsPropertyReadable, sound like it could receive any property to check. Since all properties are readable in DOM, we consider non readable properties the ones we are skipping for other reasons, which is also confusing. So, I would remove IsPropertyReadable and use SkipAttribute directly, so that it's more obvious that IsPropertyWriteable is called only for non skipped properties.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:640
> +    my @attributes = $interface->attributes;
> +    my @readableProperties = grep { IsPropertyReadable($_) } @{$interface->attributes};

You are adding @attributes but using @{$interface->attributes} directly here. I would use only attributes as the list of properties not skipped, and then use those only.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:641
> +    my @writeableProperties = grep { IsPropertyWriteable($_) } @readableProperties;;

double trailing ;

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 228185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228185&action=review
> 
> Thanks for fixing this, I always wondered where that warning came from. Please, consider my comments before landing.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:424
> > +sub IsPropertyWriteable {
> > +    my $property = shift;
> > +    if ($property->isReadOnly) {
> > +        return 0;
> > +    }
> 
> I know we are not checking if the attribute is skipped, because this is always called for readable properties, but it's confusing, because the name IsPropertyWriteable, like IsPropertyReadable, sound like it could receive any property to check. Since all properties are readable in DOM, we consider non readable properties the ones we are skipping for other reasons, which is also confusing. So, I would remove IsPropertyReadable and use SkipAttribute directly, so that it's more obvious that IsPropertyWriteable is called only for non skipped properties.

Okay. I will make IsPropertyReadable and IsPropertyWriteable completely independent. IsPropertyWriteable will return false if IsPropertyReadable returns false. I'll keep IsPropertyReadable, because I think it's a bit clearer what's going on and if we need to add more conditions everything should work without changes except to IsPropertyReadable.

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:640
> > +    my @attributes = $interface->attributes;
> > +    my @readableProperties = grep { IsPropertyReadable($_) } @{$interface->attributes};
> 
> You are adding @attributes but using @{$interface->attributes} directly here. I would use only attributes as the list of properties not skipped, and then use those only.

I'll get rid of @attributes. It was left over from a previous version.

[Martin Robinson]

Committed r166568: <http://trac.webkit.org/changeset/166568>