[webkit-reviews] review granted: [Bug 130978] [GTK] Readonly attributes installed as readwrite in GObject DOM bindings : [Attachment 228185] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 130978: [GTK] Readonly attributes installed as readwrite in GObject DOM
bindings
https://bugs.webkit.org/show_bug.cgi?id=130978

Attachment 228185: Patch
https://bugs.webkit.org/attachment.cgi?id=228185&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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 ;