[webkit-reviews] review denied: [Bug 52340] Look into possibilities of typedef in webkit idl files : [Attachment 186866] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Andrey Adaikin
<aandrey at chromium.org>'s request for review:
Bug 52340: Look into possibilities of typedef in webkit idl files
https://bugs.webkit.org/show_bug.cgi?id=52340

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=186866&action=review


This patch is very exciting; it would be a huge cleanup to be able to use
typedefs in WebGL's IDL for example. However, I perceive a few problems with
this implementation. Perhaps others who work on the bindings will have more and
better suggestions, but there are at least a couple of things that I definitely
think need to be cleaned up.

> Source/WebCore/bindings/scripts/IDLParser.pm:102
> +struct( ExtendedType => {

The name of this class is not clear. It actually represents either a type or a
typedef. Also, for consistency, it should probably not start with a capital
letter.

> Source/WebCore/bindings/scripts/IDLParser.pm:104
> +    type => '$', # Type of typedef

This comment is not clear. Please change it to something like: "If this
represents a typedef, the actual type it refers to."

> Source/WebCore/bindings/scripts/IDLParser.pm:166
> +    my $msg = "Unexpected ExtendedAttributeList in typedef \"$name\" at " .
$self->{Line};

For consistency I think this should read "extendedAttributeList" (no initial
capital letter).

> Source/WebCore/bindings/scripts/IDLParser.pm:749
> +    my $extendedAttributeList = shift; # ignored: Extended attributes are
not applicable to typedefs themselves

Then please change the call sites to no longer pass this argument, leaving this
comment here though.

> Source/WebCore/bindings/scripts/IDLParser.pm:754
> +	   my $typedef = $self->parseExtendedType();

I strongly think you should call assertNoExtendedAttributesInTypedef here so
that errors are caught when the typedef is parsed, not when it's used.

> Source/WebCore/bindings/scripts/IDLParser.pm:759
> +	   die "typedef redifinition for " . $name . " at " . $self->{Line} if
exists $typedefs{$name};

typo: redifinition -> redefinition

> Source/WebCore/bindings/scripts/IDLParser.pm:1066
> +	   $extendedAttributeList = $typedef->extendedAttributes();

Is this correct? Won't this override any extended attributes for the attribute
with those for its type?

> Source/WebCore/bindings/scripts/IDLParser.pm:1349
> +	   $paramDataNode->extendedAttributes($typedef->extendedAttributes());

Same question about whether overriding the domSignature's extendedAttributes
with those coming from the parsed type is correct.

> Source/WebCore/bindings/scripts/IDLParser.pm:1673
> +sub parseExtendedType

I think a name like "parseTypeOrTypedef" would be clearer.

> Source/WebCore/bindings/scripts/IDLParser.pm:1679
> +	   $extendedAttributeList =
$self->parseExtendedAttributeListAllowEmpty();

Is this really correct? We only parse extended attributes on a type if there
aren't any specified e.g. on the attribute or operation? This seems wrong.

> Source/WebCore/bindings/scripts/IDLParser.pm:1690
> +	       $self->assertNoExtendedAttributesInTypedef($next->value(),
__LINE__);

As mentioned above, I think this check should go inside parseTypedef.

> Source/WebCore/bindings/scripts/IDLParser.pm:1787
> +	   $self->assertNoExtendedAttributesInTypedef($next->value(),
__LINE__);

Again, this check should be done earlier.