Summary: | [GTK] Readonly attributes installed as readwrite in GObject DOM bindings | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | alecflett, cdumez, cgarcia, commit-queue, jsbell | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Martin Robinson
2014-03-31 12:23:33 PDT
Created attachment 228185 [details]
Patch
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 ; (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. Committed r166568: <http://trac.webkit.org/changeset/166568> |