[webkit-reviews] review denied: [Bug 52340] Look into possibilities of typedef in webkit idl files : [Attachment 187006] 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 187006: Patch
https://bugs.webkit.org/attachment.cgi?id=187006&action=review

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


As I'm studying this patch and the Web IDL grammar more, it's becoming clearer
to me that this patch is not correct as written. Please look at the comments
above and correct any misunderstandings I have.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1674
>> +sub parseTypeOrExtendedType
> 
> What is this parsing in the IDL grammer
(http://www.w3.org/TR/WebIDL/#idl-grammar)? We'd like to make the parser as
much as conformant with the IDL grammer.

This is an excellent point. There is no concept of an "extended type" in the
IDL grammar. This patch seems to associate the ExtendedAttributeList which a
typedef may contain with the type that follows it. However, throughout the
grammar, the ExtendedAttributeList is associated with a different production:
Argument, Definitions, etc.

The patch should be restructured to follow the grammar. Then it will make more
sense, and confusion I had earlier about ignoring incoming
ExtendedAttributeLists in certain circumstances will be cleared up.
Fundamentally, what this patch should be doing is (a) doing the missing lookup
in the typedef dictionary when parsing a type, and (b) merging the
ExtendedAttributeLists that apply to Argument and those that were specified in
the Typedef.

Here are the key rules in http://dev.w3.org/2006/webapi/WebIDL/ :

"No extended attributes defined in this specification are applicable to
typedefs themselves." This means that in rule [1] Definitions, the
ExtendedAttributeList must be empty if parsing a typedef.

"Any extended attributes that apply to a construct due to their appearing on a
referenced typedef are considered to appear after those that are specified
explicitly on the construct..." This clearly means that it's legal to specify
an ExtendedAttributeList for an Argument even if the Argument's type is a
typedef, and that the extended attribute lists are concatenated. Later text
states "none of the extended attributes defined in this specification that can
validly be used in a typedef are sensitive to order."

"A typedef must not be used as the type of a construct if an extended attribute
that appears after the typedef keyword would be disallowed from appearing
directly on that construct." You've pointed out this rule earlier. This means
that the validation of the extended attribute list has to occur when the entire
production (e.g. Argument) has been parsed, and the whole extended attribute
list and type have been assembled.

In the grammar, typedef lookup is handled in the NonAnyType production.

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

This assertion is incorrect. It's legal for an argument to have an extended
attribute list and also, if the argument refers to a typedef, for the typedef
to have had an ExtendedAttributeList.

>> Source/WebCore/bindings/scripts/test/TestCallback.idl:32
>> +typedef Class2 TYPEDEF_CLASS2;
> 
> Would you add new tests for 'typedef' instead of replacing existing tests?
Replacing existing tests makes the intention of the existing tests unclear.

Agree with this.