Bug 26398 – Make IDLParser.pm conform to the WebIDL spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Darin Adler]

Currently extended attributes are in the wrong place in our syntax. For example, they come after the word "interface" but should come before.

[Darin Adler]

Created attachment 31280 [details]
work in progress

[Dimitri Glazkov (Google)]

I'll make sure this works w/V8 bindings.

[Dimitri Glazkov (Google)]

Yay! Judging from the patch, this should work as-is w/V8 bindings codegen.

[Darin Adler]

IDLParser.pm assumes that the first line of an interface has the word "interface" in it, so this won't work until we deal with that brokenness.

Here's the command that globally changes the .idl files, though:

find . -name '*.idl' | xargs -n1 perl -i -0777 -pe 's/\b(((readonly\s)?attribute|interface|in)\s+)(\[.*?\]\s+)/$4$1/sg'

[Darin Adler]

This is a challenging project. I started on it, but got stuck. Need someone interested in this.

[Takashi Sakamoto]

Created attachment 150773 [details]
Patch

[Takashi Sakamoto]

First, Follow and Director for WebIDL grammar is:
https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dFYzbmE1QTA1NnZydnFXV3J0UjI2V0E

The differences between generated files are:
https://docs.google.com/document/d/1jaE15g8AqeGmC1-FJNpf5dJNlyJmN2jAZ5w_Tb31aZk/edit

Best regards,
Takashi Sakamoto

[Kentaro Hara]

Comment on attachment 150773 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        No new tests. Tested by comparing with sources generated by
> +        the previous IDLParser.pm.

No diff! Great!

> Source/WebCore/ChangeLog:645
> +        * bindings/scripts/IDLParser.pm:

Would you please upload the new IDLParser.pm as a separate file just for review? The current diff style is not readable...:( I think if IDLParser.pm is OK, then the whole patch is OK.

[Takashi Sakamoto]

Created attachment 150888 [details]
Patch

[Kentaro Hara]

darin: Would you roughly check if rewriting IDLParser in this direction is OK? I am happy to review implementation details.

[Adam Barth]

Generally speaking, we'd like to evolve WebKitIDL towards matching WebIDL to make it easier to compare our implementation with the specs.  In addition to moving the extended attributes, will this new approach to IDLParser.pm let us move our syntax closer to WebIDL?

One thing I noticed about this patch is that it triples the size of IDLParser.pm.  We should be sensitive to that as a cost of this change.

[Kentaro Hara]

(In reply to comment #11)
> will this new approach to IDLParser.pm let us move our syntax closer to WebIDL?

Yes. The patch is implemented in conformance with the grammar defined in the spec:

http://www.w3.org/TR/WebIDL/#idl-grammar

> One thing I noticed about this patch is that it triples the size of IDLParser.pm.  We should be sensitive to that as a cost of this change.

If I had to say one thing, the new IDLParser.pm might be a bit over engineering than is required to parse the current WebKitIDL.

[Adam Barth]

It is possible to follow the grammar of http://www.w3.org/TR/WebIDL/#idl-grammar without tripling the amount of code?

[Erik Arvidsson]

Comment on attachment 150888 [details]
Patch

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

I like this change. I don't think the code size increase is a big deal. The new code is much easier to follow.

> Source/WebCore/bindings/scripts/IDLParser.pm:289
> +# InteraceMembers := ExtendedAttributeList InterfaceMember InterfaceMembers | epsilon

typo

> Source/WebCore/bindings/scripts/IDLParser.pm:866
> +    if ($next->type() == IdentifierToken && $next->value() eq "optional") {
> +        $self->getToken();
> +        return 0;
> +    }
> +    return 0;
> +}

How come we are returning 0 in both these cases?

Same issue with parseEllipsis

[Takashi Sakamoto]

Created attachment 151681 [details]
IDL Parser Template Code Generator

[Takashi Sakamoto]

Created attachment 151682 [details]
IDL Grammar used by WebKit IDL files

[Takashi Sakamoto]

Created attachment 151686 [details]
Modified WebIDL Grammar based on 20110712

[Kentaro Hara]

> It is possible to follow the grammar of http://www.w3.org/TR/WebIDL/#idl-grammar without tripling the amount of code?

tasak: would you comment on abarth's comment above?

[Takashi Sakamoto]

Created attachment 151870 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #11)
> Generally speaking, we'd like to evolve WebKitIDL towards matching WebIDL to make it easier to compare our implementation with the specs.  In addition to moving the extended attributes, will this new approach to IDLParser.pm let us move our syntax closer to WebIDL?

Yes, I'm trying to move closer to WebIDL. 
I attached the IDL grammar which the new IDLParser.pm expects.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

(In reply to comment #13)
> It is possible to follow the grammar of http://www.w3.org/TR/WebIDL/#idl-grammar without tripling the amount of code?

I generated the new IDLParser.pm from the attached WebID grammar (the grammar is based on a little older WebIDL spec) by using the IDL Parser Template Code Generator.
I modified the generated code to support old and new WebIDL grammars and to generate the same outputs as the old IDLParser.pm does.

As current WebKit IDL files doesn't use Dictionary, DictionaryMembers, ImplementStatements and so on, I can remove the code for parsing them. However, I'm not sure whether I can reduce the code.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

(In reply to comment #19)
> Created an attachment (id=151870) [details]
> Patch

I removed diff for IDL files and added backwardCompatibleMode to parse the current WebKit IDL files.
Its default value is 1. This means that IDLParser.pm expects the current WebKit IDL files' grammar.

The following is the way to generate IDLParser.pm:
(1) Run the uploaded IDL ParserTemplate Code Generator script with the WebIDL grammar definition file.

e.g.
  python IDLParserTemplateGenerator.py WebIDLGrammar.txt

The WebIDL grammar definition file is copied from http://www.w3.org/TR/2011/WD-WebIDL-20110712/#idl-grammar 's table and modified to support WebKit IDL files. i.e.

- Support the "module" which is not terminated by ";",
- Support "setter raises" and "getter raises",
- Support multiple parents,
- Support "int".
- Extended attribute can has "condition1|condition2" or "condition1&condition2"... ,
(the spec says, the extended attributes defined in this document only accept a more restricted syntax. 
The above extended attributes don't match the spec.)
- Move extended attributes' location,
- Extended attributes can be terminated by ",]". (According to the spec, the "," is wrong.)

The result is https://bugs.webkit.org/attachment.cgi?id=151682

I created one more WebIDL grammar to move extended attributes to the spec location. The difference between the previous grammar and this is:

- Not support "setter raises" and "getter raises". Support "setraises" and "getraises".
- Move extended attributes to the spec location,

The reason why I didn't use the latest spec is that "module" was removed from the spec.
I'm not sure what is the best way to remove "module"s from WebKit IDL files. So I used the last spec which supports "module".

(2) Merge two template codes and added the code to generate the same outputs as the original IDLParser.pm does.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Comment on attachment 151682 [details]
IDL Grammar used by WebKit IDL files

[57]	IntegerType	→	"short" 
 | "int"
 | "long" OptionalLong

[Adam Barth]

> It is possible to follow the grammar of http://www.w3.org/TR/WebIDL/#idl-grammar without tripling the amount of code?

I've spent some more time looking at this patch.  Like arv, I find the new code easier to read and understand than the old code (which has always been mysterious to me).  Part of the reason the new code is larger than the old code is that it supports both the new and old syntax.  Presumably we'll remove the code for parsing the old syntax once we convert all the IDL files.

I'd like to defer to Kentaro for the actual review, if he's willing.

[Kentaro Hara]

I'll review the details when I have time (maybe in weekend or next week).

Comment on attachment 151870 [details]
Patch

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

> Source/WebCore/bindings/scripts/IDLParser.pm:314
> +    for my $item (@{$interfaceMembers}) {

How about this?

$dataNode->attributes = [ grep { ref($_) eq 'domAttribute' } @$interfaceMembers ];
$dataNode->constants = [ grep { ref($_) eq 'domConstant' } @$interfaceMembers ];
$dataNode->functions = [ grep { ref($_) eq 'domFunction' } @$interfaceMembers ];

I know this is slower than for-loop. But, we may know what attributes, constants and function have at glance.

> Source/WebCore/bindings/scripts/IDLParser.pm:512
> +    for my $item (@{$exceptionMembers}) {

This is fragment as same as in parseInterfaceNew.
Is it better to have subroutine for sharing code?

> Source/WebCore/bindings/scripts/IDLParser.pm:957
> +    if ($backwardCompatibleMode) {

nit: return $backwardCompatibleMode ? $self->parseArgumentOld() : $self->parseAgumentNew();

or

if ($backwardCompatibleMode) {
    return $self->parseArgumentOld();
}
return $self->parseArgumentNew();

> Source/WebCore/bindings/scripts/IDLParser.pm:1008
> +    return;

nit: Do we need to have "return" at end of subroutine?

> Source/WebCore/bindings/scripts/IDLParser.pm:1018
> +    return;

nit: Do we need to have "return" at end of subroutine?

> Source/WebCore/bindings/scripts/IDLParser.pm:1114
> +    my %attr = ();

nit: return {};

> Source/WebCore/bindings/scripts/IDLParser.pm:1134
> +        return \%attr;

How about writing below?

my $attrs = {};
...
$attrs->{$name} = ...;

return $attrs;

This may be easier to put "\" onto each return statement.

> Source/WebCore/bindings/scripts/IDLParser.pm:1216
> +        push(@types,$self->parsePrimitiveOrStringType());

nit: We don't need to have @types to reduce ARRAY copy and to avoid reusing variable.

return join('', $self->parsePimitiveOrStringType(), @{$self->parseTypeSuffix());

This comment applies other usage of @types and join('', @types) in this subroutine.

> Source/WebCore/bindings/scripts/IDLParser.pm:1349
> +        return \@suffix;

nit: return [ "[]", @{$self->parseTypeSuffix() ];

> Source/WebCore/bindings/scripts/IDLParser.pm:1355
> +        return \@suffix;

nit: return [ "?", @{$self->parseTypeSuffixStartingWithArray()} ];

> Source/WebCore/bindings/scripts/IDLParser.pm:1408
> +        return \@names;

nit: return [ $self->parseScopedName(), @{$self->parseScopedNames()} ];

> Source/WebCore/bindings/scripts/IDLParser.pm:1578
> +    for my $item (@{$interfaceMembers}) {

See a comment for parseInterfaceNew and parseExceptionNew.
We may want to share codes.

> Source/WebCore/bindings/scripts/IDLParser.pm:1707
> +    for my $item (@{$exceptionMembers}) {

See a comment for parseInterfaceNew, parseExceptionNew and parseInterfaceOld.
We may want to have "sub" and share code.

[Erik Arvidsson]

I don't think there is any need to support module. It is not used by any of the code generators.

[Kentaro Hara]

Comment on attachment 151870 [details]
Patch

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

Great patch!

General comments:

- Some die "..." messages are not descriptive. Please recheck dying messages so that developers can understand what kind of error is happening.

- Let's remove a bunch of verbose messages like "while parsing the rule: Module -> ...". (Details are explained below.)

- Let's use '...' instead of "..." to avoid escaping "\"".

- Let's avoid perl specific syntax (e.g. values, grep, map etc) for readability.

- Please define a long regular expression in a global variable. (e.g. /^(double|typedef|caller|byte|DOMString|short|int|exception|omittable|float|attribute|static|readonly|deleter|dictionary|\:\:|unsigned|optional|setter|interface|sequence|stringifier|module|void|getter|octet|long|\[|const|boolean|in|Date|creator|any|object)$/). We want to avoid too long regular expressions.

- ':' is not a meta character of regular expressions. You do not need to escape it in regular expressions.

- Please remove unnecessary parentheses from regular expressions. (e.g. You can remove '(' and ')' from '$foo =~ /^(true|false)$/' where you do not use $1 later.)

- m/foo/ => /foo/

- $foo =~ /^(bar)$/ => $foo eq "bar".

> Source/WebCore/bindings/scripts/IDLParser.pm:40
> +use constant True => 1;
> +use constant False => 0;

Nit: I do not think these variables are needed in your patch. You can just use 1 or 0.

> Source/WebCore/bindings/scripts/IDLParser.pm:65
> +    $beQuiet = shift;

Nit: Let's do all shifts at the head of a subroutine.

> Source/WebCore/bindings/scripts/IDLParser.pm:70
> +sub shouldTokenValueEquals

Nit: assertTokenValue might be a better name.

> Source/WebCore/bindings/scripts/IDLParser.pm:74
> +    my $line = shift;

Nit: $message would be a better name.

> Source/WebCore/bindings/scripts/IDLParser.pm:78
> +sub shouldTokenTypeEquals

Nit: assertTokenType

> Source/WebCore/bindings/scripts/IDLParser.pm:117
> +    die $@ . " in $fileName" if $@;

More descriptive message please.

> Source/WebCore/bindings/scripts/IDLParser.pm:119
> +    die "No document found" unless length(@definitions) > 0;

length() returns a string length. This check should be 'unless @definitions'.

> Source/WebCore/bindings/scripts/IDLParser.pm:148
> +    if ($self->{DocumentContent} =~ m/^[\t\n\r ]*[\n\r]/) {

Nit: 'm/foo/' => '/foo/'. The same comment for other regular expressions.

> Source/WebCore/bindings/scripts/IDLParser.pm:149
> +        $self->{DocumentContent} =~ s/^[\t\n\r ]*[\n\r]//g;

'g' is not needed.

> Source/WebCore/bindings/scripts/IDLParser.pm:153
> +            $self->{Line} = $1;

You want to store a line number to $self->{Line}, right? But $1 is ([^\n\r]+) in your patch.

> Source/WebCore/bindings/scripts/IDLParser.pm:155
> +            $self->{Line} = "";

Nit: "" => "Unknown"

> Source/WebCore/bindings/scripts/IDLParser.pm:158
> +    $self->{DocumentContent} =~ s/^([\t\n\r ]+)//g;

s/^([\t\n\r ]*)//. 'g' is not needed.

> Source/WebCore/bindings/scripts/IDLParser.pm:159
> +    if (length($self->{DocumentContent}) <= 0) {

Nit: length($self->{DocumentContent}) == 0

> Source/WebCore/bindings/scripts/IDLParser.pm:167
> +        $self->{DocumentContent} =~ s/^(-?(([0-9]+\.[0-9]*|[0-9]*\.[0-9]+)([Ee][+-]?[0-9]+)?|[0-9]+[Ee][+-]?[0-9]+))//;

Let's avoid writing the regular expression twice. You can store the regular expression in a variable.

> Source/WebCore/bindings/scripts/IDLParser.pm:173
> +        $self->{DocumentContent} =~ s/^(-?[1-9][0-9]*|-?0[Xx][0-9A-Fa-f]+|-?0[0-7]*)//;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:179
> +        $self->{DocumentContent} =~ s/^(\"[^\"]*\")//;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:185
> +        $self->{DocumentContent} =~ s/^([A-Z_a-z][0-9A-Z_a-z]*)//;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:189
> +        $token->type(OtherToken);

What is other token?

> Source/WebCore/bindings/scripts/IDLParser.pm:191
> +        $self->{DocumentContent} =~ s/^(::|\.\.\.|[^\t\n\r 0-9A-Z_a-z])//;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:204
> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(dictionary|typedef|module|\:\:|\[|interface|exception|partial)$/)) {

':' is not a meta character. You do not need to escape it. The same comment for other '\:'s in this patch.

> Source/WebCore/bindings/scripts/IDLParser.pm:210
> +    return\@definitions;

Nit: 'return \@definitions'. A space after 'return' please.

> Source/WebCore/bindings/scripts/IDLParser.pm:217
> +    if ($next->value() =~ /^(partial)$/) {

Nit: $next->value() eq "partial". The same comment for other '$foo =~ /^(bar)/$'s.

> Source/WebCore/bindings/scripts/IDLParser.pm:220
> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(double|typedef|caller|byte|DOMString|short|int|exception|omittable|float|attribute|static|readonly|deleter|dictionary|\:\:|unsigned|optional|setter|interface|sequence|stringifier|module|void|getter|octet|long|\[|const|boolean|in|Date|creator|any|object)$/)) {

This long list is used several times in the parser. Let's define it by a global variable.

In addition, the parenthesis (i.e. '(double|...|object)') is not needed. The same comment for other regular expressions. Please do not add unnecessary parentheses.

> Source/WebCore/bindings/scripts/IDLParser.pm:224
> +    die "Unexpected token " . $next->value() . " while applying the rule: Definition -> ExtendedAttributeList NormalDefinition|PartialInterface at " . $self->{Line};

Let's define assertUnexpectedToken() for this dying message. The same comment for other parts.

> Source/WebCore/bindings/scripts/IDLParser.pm:233
> +    if ($next->value() =~ /^(module)$/) {

Nit: $next->value() eq "module"

> Source/WebCore/bindings/scripts/IDLParser.pm:262
> +    shouldTokenValueEquals($self->getToken(), "module", "while parsing the rule: Module -> \"module\"^^^^ identifier \"\{\" Definitions \"\}\" OptionalSemicolon at line: \"" . $self->{Line} . "\"");

Let's use '...' to avoid escaping "\"". The same comment for all strings that contain "\"".

In addition I want to remove a bunch of verbose messages like "while parsing the rule: Module -> ...". I think that the column number and the line number would be enough for the message. Then the code would look like:

  shouldTokenValueEquals($self->getToken(), "module", $self);

Then shouldTokenValueEquals() can output $self->{Line} and $self->{Column}. It would be nicer if shouldTokenValueEquals() can also output the exact error position of $self->{DocumentContent}.

>> Source/WebCore/bindings/scripts/IDLParser.pm:314
>> +    for my $item (@{$interfaceMembers}) {
> 
> How about this?
> 
> $dataNode->attributes = [ grep { ref($_) eq 'domAttribute' } @$interfaceMembers ];
> $dataNode->constants = [ grep { ref($_) eq 'domConstant' } @$interfaceMembers ];
> $dataNode->functions = [ grep { ref($_) eq 'domFunction' } @$interfaceMembers ];
> 
> I know this is slower than for-loop. But, we may know what attributes, constants and function have at glance.

Both are OK but I would prefer tasak's original code. We might want to avoid using perl-specific syntax such as grep and map, for readability.

> Source/WebCore/bindings/scripts/IDLParser.pm:390
> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(double|stringifier|caller|byte|void|getter|octet|DOMString|long|short|int|omittable|float|attribute|static|readonly|deleter|boolean|\:\:|unsigned|setter|Date|creator|any|object|sequence)$/)) {

Please define the list in a global variable.

> Source/WebCore/bindings/scripts/IDLParser.pm:464
> +    if ($next->value() =~ /^(\=)$/) {

Nit: $next->value() eq "=".

> Source/WebCore/bindings/scripts/IDLParser.pm:468
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:944
> +        if ($next->value() =~ /^(\,)$/) {

Nit: $next->value() eq ","

> Source/WebCore/bindings/scripts/IDLParser.pm:981
> +        $type =~ s/\?//;

The 'g' option is needed. $type =~ s/\?//g;

>> Source/WebCore/bindings/scripts/IDLParser.pm:1008
>> +    return;
> 
> nit: Do we need to have "return" at end of subroutine?

Nit: No.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1018
>> +    return;
> 
> nit: Do we need to have "return" at end of subroutine?

Nit: No.

> Source/WebCore/bindings/scripts/IDLParser.pm:1046
> +        $newDataNode->signature(new domSignature);

Nit: new domSignature()

> Source/WebCore/bindings/scripts/IDLParser.pm:1065
> +        @attrs{keys %{$attr}} = values %{$attr};

I don't have a strong opinion, but you might want to use a simple for-loop for readability. We want to avoid perl-specific syntax.

  for my $key (keys %$attr) {
    $attrs{$key} = $attr->{$key};
  }

> Source/WebCore/bindings/scripts/IDLParser.pm:1067
> +        @attrs{keys %{$attr}} = values %{$attr};

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1082
> +        @attrs{keys %{$attr}} = values %{$attr};

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1085
> +        @attrs{keys %{$attr}} = values %{$attr};

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1100
> +        my %attr = ();
> +        return \%attr;

Nit: return {};

> Source/WebCore/bindings/scripts/IDLParser.pm:1160
> +sub parseExtendedAttributeRest3

I don't understand this code, but is it OK to make the type of return values inconsistent? (i.e. sometimes a string, sometimes a hash)

>> Source/WebCore/bindings/scripts/IDLParser.pm:1216
>> +        push(@types,$self->parsePrimitiveOrStringType());
> 
> nit: We don't need to have @types to reduce ARRAY copy and to avoid reusing variable.
> 
> return join('', $self->parsePimitiveOrStringType(), @{$self->parseTypeSuffix());
> 
> This comment applies other usage of @types and join('', @types) in this subroutine.

Both are OK. (I would prefer @types for readability though.)

> Source/WebCore/bindings/scripts/IDLParser.pm:1322
> +        } else {
> +            return "long";
> +        }

Nit: return "long". 'else' is not needed.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1349
>> +        return \@suffix;
> 
> nit: return [ "[]", @{$self->parseTypeSuffix() ];

Both are OK. (I would prefer @suffix for readability though.)

>> Source/WebCore/bindings/scripts/IDLParser.pm:1355
>> +        return \@suffix;
> 
> nit: return [ "?", @{$self->parseTypeSuffixStartingWithArray()} ];

Ditto.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1408
>> +        return \@names;
> 
> nit: return [ $self->parseScopedName(), @{$self->parseScopedNames()} ];

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1495
> +# For backward compatibility

These subroutines will be removed soon. Is my understanding correct?

> Source/WebCore/bindings/scripts/IDLParser.pm:1675
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:1818
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:1833
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:1878
> +sub applyExtendedAttributeList

Please move this subroutine above. This is not for-backward-compatibility code.

[Kentaro Hara]

(In reply to comment #27)
> I don't think there is any need to support module. It is not used by any of the code generators.

Agree. Modules are going to be deprecated from the Web IDL spec. Let's remove all modules completely from WebKitIDL. Modules in IDL files are not used at all. But let's do it in a follow-up patch.

[Takashi Sakamoto]

Created attachment 154304 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 151870 [details]
Patch

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

>> Source/WebCore/bindings/scripts/IDLParser.pm:155
>> +            $self->{Line} = "";
> 
> Nit: "" => "Unknown"

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:158
>> +    $self->{DocumentContent} =~ s/^([\t\n\r ]+)//g;
> 
> s/^([\t\n\r ]*)//. 'g' is not needed.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:159
>> +    if (length($self->{DocumentContent}) <= 0) {
> 
> Nit: length($self->{DocumentContent}) == 0

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:167
>> +        $self->{DocumentContent} =~ s/^(-?(([0-9]+\.[0-9]*|[0-9]*\.[0-9]+)([Ee][+-]?[0-9]+)?|[0-9]+[Ee][+-]?[0-9]+))//;
> 
> Let's avoid writing the regular expression twice. You can store the regular expression in a variable.

I see. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:173
>> +        $self->{DocumentContent} =~ s/^(-?[1-9][0-9]*|-?0[Xx][0-9A-Fa-f]+|-?0[0-7]*)//;
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:179
>> +        $self->{DocumentContent} =~ s/^(\"[^\"]*\")//;
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:185
>> +        $self->{DocumentContent} =~ s/^([A-Z_a-z][0-9A-Z_a-z]*)//;
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:189
>> +        $token->type(OtherToken);
> 
> What is other token?

"other" comes from WebIDL spec. IDLgrammar defines integer token, float token, identifier token, string token, whitespace token and other token.
So I prepared 5 token types.  The original "other" definition is:

other	=	[^\t\n\r 0-9A-Z_a-z]

However I would like to treat "::" and "..." as one token. So I added "::|\.\.\.".

>> Source/WebCore/bindings/scripts/IDLParser.pm:191
>> +        $self->{DocumentContent} =~ s/^(::|\.\.\.|[^\t\n\r 0-9A-Z_a-z])//;
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:204
>> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(dictionary|typedef|module|\:\:|\[|interface|exception|partial)$/)) {
> 
> ':' is not a meta character. You do not need to escape it. The same comment for other '\:'s in this patch.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:210
>> +    return\@definitions;
> 
> Nit: 'return \@definitions'. A space after 'return' please.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:217
>> +    if ($next->value() =~ /^(partial)$/) {
> 
> Nit: $next->value() eq "partial". The same comment for other '$foo =~ /^(bar)/$'s.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:224
>> +    die "Unexpected token " . $next->value() . " while applying the rule: Definition -> ExtendedAttributeList NormalDefinition|PartialInterface at " . $self->{Line};
> 
> Let's define assertUnexpectedToken() for this dying message. The same comment for other parts.

Sure. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:233
>> +    if ($next->value() =~ /^(module)$/) {
> 
> Nit: $next->value() eq "module"

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:262
>> +    shouldTokenValueEquals($self->getToken(), "module", "while parsing the rule: Module -> \"module\"^^^^ identifier \"\{\" Definitions \"\}\" OptionalSemicolon at line: \"" . $self->{Line} . "\"");
> 
> Let's use '...' to avoid escaping "\"". The same comment for all strings that contain "\"".
> 
> In addition I want to remove a bunch of verbose messages like "while parsing the rule: Module -> ...". I think that the column number and the line number would be enough for the message. Then the code would look like:
> 
>   shouldTokenValueEquals($self->getToken(), "module", $self);
> 
> Then shouldTokenValueEquals() can output $self->{Line} and $self->{Column}. It would be nicer if shouldTokenValueEquals() can also output the exact error position of $self->{DocumentContent}.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:390
>> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(double|stringifier|caller|byte|void|getter|octet|DOMString|long|short|int|omittable|float|attribute|static|readonly|deleter|boolean|\:\:|unsigned|setter|Date|creator|any|object|sequence)$/)) {
> 
> Please define the list in a global variable.

I see. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:464
>> +    if ($next->value() =~ /^(\=)$/) {
> 
> Nit: $next->value() eq "=".

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:468
>> +    return;
> 
> Nit: Remove this.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:512
>> +    for my $item (@{$exceptionMembers}) {
> 
> This is fragment as same as in parseInterfaceNew.
> Is it better to have subroutine for sharing code?

I agree. It is better. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:944
>> +        if ($next->value() =~ /^(\,)$/) {
> 
> Nit: $next->value() eq ","

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:957
>> +    if ($backwardCompatibleMode) {
> 
> nit: return $backwardCompatibleMode ? $self->parseArgumentOld() : $self->parseAgumentNew();
> 
> or
> 
> if ($backwardCompatibleMode) {
>     return $self->parseArgumentOld();
> }
> return $self->parseArgumentNew();

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:981
>> +        $type =~ s/\?//;
> 
> The 'g' option is needed. $type =~ s/\?//g;

Done.

>>> Source/WebCore/bindings/scripts/IDLParser.pm:1008
>>> +    return;
>> 
>> nit: Do we need to have "return" at end of subroutine?
> 
> Nit: No.

I see. I removed "return;".

>>> Source/WebCore/bindings/scripts/IDLParser.pm:1018
>>> +    return;
>> 
>> nit: Do we need to have "return" at end of subroutine?
> 
> Nit: No.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1046
>> +        $newDataNode->signature(new domSignature);
> 
> Nit: new domSignature()

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1065
>> +        @attrs{keys %{$attr}} = values %{$attr};
> 
> I don't have a strong opinion, but you might want to use a simple for-loop for readability. We want to avoid perl-specific syntax.
> 
>   for my $key (keys %$attr) {
>     $attrs{$key} = $attr->{$key};
>   }

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1067
>> +        @attrs{keys %{$attr}} = values %{$attr};
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1082
>> +        @attrs{keys %{$attr}} = values %{$attr};
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1085
>> +        @attrs{keys %{$attr}} = values %{$attr};
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1100
>> +        return \%attr;
> 
> Nit: return {};

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1114
>> +    my %attr = ();
> 
> nit: return {};

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1134
>> +        return \%attr;
> 
> How about writing below?
> 
> my $attrs = {};
> ...
> $attrs->{$name} = ...;
> 
> return $attrs;
> 
> This may be easier to put "\" onto each return statement.

I see. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1160
>> +sub parseExtendedAttributeRest3
> 
> I don't understand this code, but is it OK to make the type of return values inconsistent? (i.e. sometimes a string, sometimes a hash)

The reason is to make the same resut as the original IDLParser's parseExtendedAttribute generates. The following is the rules related to the code, i.e. ExtendedAttribute rules:

[48]    ExtendedAttributeRest  -> "(" ArgumentList ")" | "=" ExtendedAttributeRest2 | empty
[49]    ExtendedAttributeRest2 -> identifier ExtendedAttributeRest3 | integer
[50]    ExtendedAttributeRest3 -> "&" identifier | "|" identifier | "(" ArgumentList ")" | empty

So in the case matching the rule: "&" identifier or "|" identifier, the original parseExtendedAttribute does:

                # Parse '=' value ','?                                            
                if ($str =~ /^\s*([^,]*),?/) {
                    $attrs{$name} = $1;

And in the case matching  "(" ArgumentList ")" ,

                    # Parse '(' arguments ')' ','?                                
                    $str =~ s/^\s*\(//;
                    if ($str =~ /^([^)]*)\),?/) {
                        my $signature = $1;
                        $signature =~ s/^(.*?)\s*$/$1/;
                        $attrs{$name} = {"ConstructorName" => $constructorName, "Signature" => $signature};
...

The original IDLParser calls arguments as "Signature".

>> Source/WebCore/bindings/scripts/IDLParser.pm:1322
>> +        }
> 
> Nit: return "long". 'else' is not needed.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1495
>> +# For backward compatibility
> 
> These subroutines will be removed soon. Is my understanding correct?

I hope so. If we quickly move extended attributes to the right location defined in WebIDL spec, we can remove soon.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1578
>> +    for my $item (@{$interfaceMembers}) {
> 
> See a comment for parseInterfaceNew and parseExceptionNew.
> We may want to share codes.

Sure. Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1675
>> +    return;
> 
> Nit: Remove this.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1707
>> +    for my $item (@{$exceptionMembers}) {
> 
> See a comment for parseInterfaceNew, parseExceptionNew and parseInterfaceOld.
> We may want to have "sub" and share code.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1818
>> +    return;
> 
> Nit: Remove this.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1833
>> +    return;
> 
> Nit: Remove this.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1878
>> +sub applyExtendedAttributeList
> 
> Please move this subroutine above. This is not for-backward-compatibility code.

Sure. Done.

[Takashi Sakamoto]

Thank you for reviewing the big patch.

I uploaded a new patch, but I have one problem. As WebIDL doesn't support "static" attribute, this IDLParser.pm cannot parse current TestObj.idl.

I would like to ask whether we need to support static attribute or not. If needed, I will modify WebIDL grammar and recreate patch.

Best regards,
Takashi Sakamoto

[Adam Barth]

> I would like to ask whether we need to support static attribute or not. If needed, I will modify WebIDL grammar and recreate patch.

Does WebIDL support the static attribute?  I'm pretty sure we use it.  (There have been some recent patches improving the code it generates.)

[Kentaro Hara]

(In reply to comment #33)
> > I would like to ask whether we need to support static attribute or not. If needed, I will modify WebIDL grammar and recreate patch.
> 
> Does WebIDL support the static attribute?  I'm pretty sure we use it.  (There have been some recent patches improving the code it generates.)

Yeah, maybe that was a mistake. As commented in bug 88919, static attributes are not defined in the WebIDL spec. We might need to roll out those changes, depending on the discussion of bug 88919.

[Erik Arvidsson]

(In reply to comment #32)
> I uploaded a new patch, but I have one problem. As WebIDL doesn't support "static" attribute, this IDLParser.pm cannot parse current TestObj.idl.

We use static and WebIDL defines it.

http://www.w3.org/TR/WebIDL/#prod-ArgumentNameKeyword
http://dev.w3.org/2006/webapi/WebIDL/#prod-ArgumentNameKeyword

[Erik Arvidsson]

(In reply to comment #35)

We use static for DOMURL to allow URL.createObjectURL(...) as well as as new URL(...)

The links above are for static attributes but there is a section about static methods too.

http://dev.w3.org/2006/webapi/WebIDL/#dfn-static-operation

[Takashi Sakamoto]

Created attachment 155495 [details]
WIP

[Takashi Sakamoto]

(In reply to comment #36)
> (In reply to comment #35)
> 
> We use static for DOMURL to allow URL.createObjectURL(...) as well as as new URL(...)
> 
> The links above are for static attributes but there is a section about static methods too.
> 
> http://dev.w3.org/2006/webapi/WebIDL/#dfn-static-operation

Thank you. I'm now recreating a patch for supporting static attribute.

Best regards,
Takashi Sakamoto

[Kentaro Hara]

Comment on attachment 155495 [details]
WIP

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

Thank you very much for updating the patch.

> Source/WebCore/ChangeLog:33
> +        * bindings/scripts/IDLParser.pm:
> +        To parse the existing WebKit IDL. Fixed the bug when parsing long
> +        longAttr. Expected result is int longAttr, but the previous one generates
> +        long long Attr.
> +        * bindings/scripts/IDLParserEditorsDraft.pm:
> +        To parse WebIDL based on Editor's Draft. However Editor's Draft doesn't
> +        have setraises, getraises and exception list. Modified the grammar to
> +        support these exceptions.

What's your plan to land this patch? Are you intending to land both IDLParser.pm and IDLParserEditorsDraft.pm?

We want to avoid over-engineering. IDLParser.pm does not have to be strictly conformed to the spec. I think it's OK to accept some temporal inconsistency. Maintainability would be more important.

> Source/WebCore/bindings/scripts/IDLParser.pm:44
> +my $beQuiet = 0;

IMHO, messages output by $beQuiet is not so helpful. Maybe you can remove $beQuiet.

> Source/WebCore/bindings/scripts/IDLParser.pm:113
> +    die $@ . " in $fileName" if $@;

More descriptive message please, so that developers can understand what happened.

> Source/WebCore/bindings/scripts/IDLParser.pm:115
> +    die "No document found" unless @definitions;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:118
> +    die "No document found" unless ref($document) eq "idlDocument";

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:154
> +        $self->{LineNumber}++ while ($skipped =~ m/\n/g);

Nit: 'm' is not needed.

> Source/WebCore/bindings/scripts/IDLParser.pm:157
> +        $self->{DocumentContent} =~ /^([^\n\r]+)/;
> +        # Line is used for error output.
> +        if (defined($1)) {

This can be:

    if ($self->{DocumentContent} =~ /^([^\n\r]+)/)

> Source/WebCore/bindings/scripts/IDLParser.pm:163
> +    $self->{DocumentContent} =~ s/^([\t\n\r ]+)//;

Is this needed?

> Source/WebCore/bindings/scripts/IDLParser.pm:164
> +    if (length($self->{DocumentContent}) == 0) {

if ($self->{DocumentContent} eq "")

> Source/WebCore/bindings/scripts/IDLParser.pm:199
> +    die "Failed in tokenizing at " . $self->{Line};

Can you use $self->assertUnexpectedToken()?

> Source/WebCore/bindings/scripts/IDLParser.pm:210
> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|partial|typedef)$/)) {

Let's avoid hard-coding '::|dictionary|exception|interface|module|partial|typedef'.

Nit: The parentheses around the pattern is not needed.

Nit: Extra parentheses around each condition. The same comment for other parts.

> Source/WebCore/bindings/scripts/IDLParser.pm:226
> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|typedef)$/)) {

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:340
> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|DOMString|Date|\[|any|attribute|boolean|byte|caller|const|creator|deleter|double|float|getter|long|object|octet|omittable|readonly|sequence|setter|short|static|unsigned|void)$/)) {

Let's avoid hard-coding. The same comment for all other hard-codings.

> Source/WebCore/bindings/scripts/IDLParser.pm:444
> +    return;

Nit: Not needed. The same comment for other parts.

[Takashi Sakamoto]

Created attachment 157442 [details]
Patch

[Takashi Sakamoto]

Created attachment 157658 [details]
Patch

[Takashi Sakamoto]

Created attachment 157661 [details]
WebIDL Parser Generator

[Takashi Sakamoto]

Created attachment 157663 [details]
IDL Grammar (merged current WebKit IDL grammar and WebIDL Editor's draft grammar)

For example, this grammar supports:
- attribute [Reflect] DOMString reflectedStringAttr;
- attribute long attrWithGetterException getter raises(DOMException);

and

- [Reflect] attribute DOMString reflectedStringAttr;
- attribute long attrWithGetterException getraises(DOMException);

[Takashi Sakamoto]

Created attachment 157670 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 155495 [details]
WIP

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

>> Source/WebCore/ChangeLog:33
>> +        support these exceptions.
> 
> What's your plan to land this patch? Are you intending to land both IDLParser.pm and IDLParserEditorsDraft.pm?
> 
> We want to avoid over-engineering. IDLParser.pm does not have to be strictly conformed to the spec. I think it's OK to accept some temporal inconsistency. Maintainability would be more important.

The new patch supports both IDL grammars (WebKit one and WebIDL editor's draft one). So after landing the new IDLParser.pm, we can remove "module" or can move extended attributes to the spec position or ...
After finish updating all IDLs, I will remove obsolete grammar from IDLParser.pm.

>> Source/WebCore/bindings/scripts/IDLParser.pm:44
>> +my $beQuiet = 0;
> 
> IMHO, messages output by $beQuiet is not so helpful. Maybe you can remove $beQuiet.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:113
>> +    die $@ . " in $fileName" if $@;
> 
> More descriptive message please, so that developers can understand what happened.

I think, $@ includes information about an error. Do you mean the information is not enough useful?

>> Source/WebCore/bindings/scripts/IDLParser.pm:115
>> +    die "No document found" unless @definitions;
> 
> Ditto.

I changed to "No definitions found in " . $fileName.

>> Source/WebCore/bindings/scripts/IDLParser.pm:118
>> +    die "No document found" unless ref($document) eq "idlDocument";
> 
> Ditto.

I changed to "No definitions found in " . $fileName.

>> Source/WebCore/bindings/scripts/IDLParser.pm:154
>> +        $self->{LineNumber}++ while ($skipped =~ m/\n/g);
> 
> Nit: 'm' is not needed.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:157
>> +        if (defined($1)) {
> 
> This can be:
> 
>     if ($self->{DocumentContent} =~ /^([^\n\r]+)/)

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:163
>> +    $self->{DocumentContent} =~ s/^([\t\n\r ]+)//;
> 
> Is this needed?

This is caused by $whitespaceToken. As I wanted to get {LINE} as true line (including spaces at the top of line),  I used '^[\t\n\r ]*[\n\r]'. However, I removed such spaces and changed $whitespaceToken. So I removed this line.

>> Source/WebCore/bindings/scripts/IDLParser.pm:164
>> +    if (length($self->{DocumentContent}) == 0) {
> 
> if ($self->{DocumentContent} eq "")

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:199
>> +    die "Failed in tokenizing at " . $self->{Line};
> 
> Can you use $self->assertUnexpectedToken()?

I implemented this method to create tokens (i.e.lex part). So I think, this is not an unexpected token. Probably invalid characters or something.

>> Source/WebCore/bindings/scripts/IDLParser.pm:210
>> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|partial|typedef)$/)) {
> 
> Let's avoid hard-coding '::|dictionary|exception|interface|module|partial|typedef'.
> 
> Nit: The parentheses around the pattern is not needed.
> 
> Nit: Extra parentheses around each condition. The same comment for other parts.

I updated the IDL parser generator to avoid hard-coding for such a long regex, '::|dictionary|exception|interface|module|partial|typedef'.
Also removed useless ().

>> Source/WebCore/bindings/scripts/IDLParser.pm:226
>> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|typedef)$/)) {
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:340
>> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|DOMString|Date|\[|any|attribute|boolean|byte|caller|const|creator|deleter|double|float|getter|long|object|octet|omittable|readonly|sequence|setter|short|static|unsigned|void)$/)) {
> 
> Let's avoid hard-coding. The same comment for all other hard-codings.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:444
>> +    return;
> 
> Nit: Not needed. The same comment for other parts.

Done.

[Takashi Sakamoto]

Created attachment 160899 [details]
Patch

[Takashi Sakamoto]

Created attachment 164672 [details]
Patch

[Kentaro Hara]

Sorry for the delay. Started reviewing.

[Kentaro Hara]

Comment on attachment 164672 [details]
Patch

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

Getting close to LGTM.

> Source/WebCore/ChangeLog:11
> +        Firstly merges two grammars (editros draft and WebKit current IDL) and

Typo: editors

> Source/WebCore/bindings/scripts/IDLParser.pm:67
> +    my $self = shift;
> +    my $token = shift;
> +    my $value = shift;
> +    my $line = shift;

It looks like $line is not used by any caller. Maybe you need to add __LINE__ to all callers?

> Source/WebCore/bindings/scripts/IDLParser.pm:70
> +        $msg = $msg . $line;

$msg .= " IDLParser.pm:" . $line;

> Source/WebCore/bindings/scripts/IDLParser.pm:90
> +        $msg = $msg . " IDLParser.pm:" . $line;

Nit: $msg .= " IDLParser.pm:" . $line;

> Source/WebCore/bindings/scripts/IDLParser.pm:116
> +        if ($next->type() != EmptyToken) {
> +            die "Expect EOF, but found " . $next->value() . " at " . $self->{Line};
>          }

You can use assertTokenType().

> Source/WebCore/bindings/scripts/IDLParser.pm:131
> +        
> +    die "No document found" unless ref($document) eq "idlDocument";

Remove this. This should not happen.

> Source/WebCore/bindings/scripts/IDLParser.pm:165
> +        $self->{LineNumber}++ while ($skipped =~ /\n/g);

Can be an infinite loop?

> Source/WebCore/bindings/scripts/IDLParser.pm:167
> +        if ($self->{DocumentContent} =~ /^([^\n\r]+)/) {
> +            $self->{Line} = $self->{LineNumber} . ":" . $1;

If you match '\n\n\n\n\n', then '\n\n\n\n\n' is appended. Is it expected?

> Source/WebCore/bindings/scripts/IDLParser.pm:510
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:628
> +    return;

Nit: Remove this. The same comment for all other last 'return's.

> Source/WebCore/bindings/scripts/IDLParser.pm:1239
> +        if ($type =~ /^(.*)\?$/) {

Nit: $type =~ /\?$/

> Source/WebCore/bindings/scripts/IDLParser.pm:1254
> +        if ($type =~ /^(.*)\?$/) {

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1346
> +        my $attrs = {};
> +        my $attr = $self->parseExtendedAttribute();

These two variable names are confusing.

> Source/WebCore/bindings/scripts/IDLParser.pm:1369
> +            my $attr = $self->parseExtendedAttribute2();

Ditto.

> Source/WebCore/bindings/scripts/test/TestObj.idl:54
> -        JS, V8
> +        // JS, V8

Shall we remove this comment?

> Source/WebCore/bindings/scripts/test/TestObj.idl:232
> +        void convert1(in [TreatReturnedNullStringAs=Null] a value);
> +        void convert2(in [TreatReturnedNullStringAs=Undefined] b value);
> +        void convert4(in [TreatNullAs=NullString] d value);
> +        void convert5(in [TreatNullAs=NullString, TreatUndefinedAs=NullString] e value);

'a', 'b', 'd', 'e' are not valid type names. Shall we change them to DOMString or something?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:168
> -    return v8::Number::New(static_cast<double>(imp->attr()));
> +    return v8Integer(imp->longAttr(), info.GetIsolate());

I know that this code is conformed to the spec. But isn't there any compatibility concern to change a double to an integer?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:176
> -    long long v = toInt64(value);
> -    imp->setAttr(WTF::getPtr(v));
> +    int v = toInt32(value);
> +    imp->setLongAttr(v);

Ditto. Isn't there any compatibility concern to change a long long to an int?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:454
> -    ExceptionCode ec = 0;
> -    imp->setAttrWithGetterException(v, ec);
> -    if (UNLIKELY(ec))
> -        setDOMException(ec, info.GetIsolate());
> +    imp->setAttrWithGetterException(v);

Where does this change come from? Is it correct?

[Takashi Sakamoto]

Created attachment 164814 [details]
Patch

[Kentaro Hara]

(In reply to comment #50)
> Created an attachment (id=164814) [details]
> Patch

Would you comment on my review comments? (I want to know how you fixed them.)

[Takashi Sakamoto]

Created attachment 164825 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 164672 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Firstly merges two grammars (editros draft and WebKit current IDL) and
> 
> Typo: editors

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:67
>> +    my $line = shift;
> 
> It looks like $line is not used by any caller. Maybe you need to add __LINE__ to all callers?

I think, $line provides much more information to debug IDLParser.pm and IDL files.
I added __LINE__.

>> Source/WebCore/bindings/scripts/IDLParser.pm:70
>> +        $msg = $msg . $line;
> 
> $msg .= " IDLParser.pm:" . $line;

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:90
>> +        $msg = $msg . " IDLParser.pm:" . $line;
> 
> Nit: $msg .= " IDLParser.pm:" . $line;

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:116
>>          }
> 
> You can use assertTokenType().

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:131
>> +    die "No document found" unless ref($document) eq "idlDocument";
> 
> Remove this. This should not happen.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:165
>> +        $self->{LineNumber}++ while ($skipped =~ /\n/g);
> 
> Can be an infinite loop?

I think, the "/g" modifier can be used to process all regex matches in a string. The first m/regex/g will find the first match, the second m/regex/g the second match, etc. c.f. http://www.regular-expressions.info/perl.html

>> Source/WebCore/bindings/scripts/IDLParser.pm:167
>> +            $self->{Line} = $self->{LineNumber} . ":" . $1;
> 
> If you match '\n\n\n\n\n', then '\n\n\n\n\n' is appended. Is it expected?

Since ^[^\n\r]+, cannot match '\n\n\n\n\n'. In this case, no character can match.

>> Source/WebCore/bindings/scripts/IDLParser.pm:510
>> +    return;
> 
> Nit: Remove this.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:628
>> +    return;
> 
> Nit: Remove this. The same comment for all other last 'return's.

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1239
>> +        if ($type =~ /^(.*)\?$/) {
> 
> Nit: $type =~ /\?$/

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1254
>> +        if ($type =~ /^(.*)\?$/) {
> 
> Ditto.

Done.

>> Source/WebCore/bindings/scripts/test/TestObj.idl:232
>> +        void convert5(in [TreatNullAs=NullString, TreatUndefinedAs=NullString] e value);
> 
> 'a', 'b', 'd', 'e' are not valid type names. Shall we change them to DOMString or something?

I think, 'a', 'b', 'd', 'e' are for checking the case where some user-defined class is specified. WebIDL allows users to use any identifier as NonAnyType.

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:168
>> +    return v8Integer(imp->longAttr(), info.GetIsolate());
> 
> I know that this code is conformed to the spec. But isn't there any compatibility concern to change a double to an integer?

This double is a bug of old IDLParser.pm. TestObj.idl has "attribute long longAttr;". The old IDLParser.pm cannot parse the line correctly. So the parser generates the code for "long long" + "Attr". But the correct result should be "long" + "longAttr".

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:176
>> +    imp->setLongAttr(v);
> 
> Ditto. Isn't there any compatibility concern to change a long long to an int?

Ditto.

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:454
>> +    imp->setAttrWithGetterException(v);
> 
> Where does this change come from? Is it correct?

The code is generated from "attribute long attrWithGetterException getter raises(DOMException);". So 2 methods are generated, one is attrWithGetterExceptionAttrGetter and the other is attrWithGetterExceptionAttrSetter.
Since only "getter raises" is specified, "setter" should not raise any exception. Does this make sense?

[Takashi Sakamoto]

(In reply to comment #51)
> (In reply to comment #50)
> > Created an attachment (id=164814) [details] [details]
> > Patch
> 
> Would you comment on my review comments? (I want to know how you fixed them.)

Yes, I would like to do. I'm just waiting for cq result.

[Takashi Sakamoto]

Comment on attachment 164825 [details]
Patch

Since qt and qt-wk2 look not good, I would like to request review?.

[Takashi Sakamoto]

(In reply to comment #55)
> (From update of attachment 164825 [details])
> Since qt and qt-wk2 look not good, I would like to request review?.

I mean, the bot seems not to work well.

[Kentaro Hara]

Thanks for all the clarification.

> >> Source/WebCore/bindings/scripts/test/TestObj.idl:232
> >> +        void convert5(in [TreatNullAs=NullString, TreatUndefinedAs=NullString] e value);
> > 
> > 'a', 'b', 'd', 'e' are not valid type names. Shall we change them to DOMString or something?
> 
> I think, 'a', 'b', 'd', 'e' are for checking the case where some user-defined class is specified. WebIDL allows users to use any identifier as NonAnyType.

OK. Then let's change them to 'UserDefinedType' or something like that. You can do it in a follow-up patch.


> >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:168
> >> +    return v8Integer(imp->longAttr(), info.GetIsolate());
> > 
> > I know that this code is conformed to the spec. But isn't there any compatibility concern to change a double to an integer?
> 
> This double is a bug of old IDLParser.pm. TestObj.idl has "attribute long longAttr;". The old IDLParser.pm cannot parse the line correctly. So the parser generates the code for "long long" + "Attr". But the correct result should be "long" + "longAttr".
> 
> >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:454
> >> +    imp->setAttrWithGetterException(v);
> > 
> > Where does this change come from? Is it correct?
> 
> The code is generated from "attribute long attrWithGetterException getter raises(DOMException);". So 2 methods are generated, one is attrWithGetterExceptionAttrGetter and the other is attrWithGetterExceptionAttrSetter.
> Since only "getter raises" is specified, "setter" should not raise any exception. Does this make sense?

Makes sense.

I'd like to confirm one thing: There are changes in the generated code for bindings/scripts/tests/*.idl, but there is no meaningful change in the generated code for real idl files for JSC/V8/ObjC/GObject/CPP, right?

If it is right, I'm happy to r+ the patch.

[Takashi Sakamoto]

Created attachment 165050 [details]
Patch

[Takashi Sakamoto]

I'm sorry. I forgot to remove "// JS,V8" comment line. I uploaded a new patch for fixing the issue.

(In reply to comment #57)
> Thanks for all the clarification.
> 
> > >> Source/WebCore/bindings/scripts/test/TestObj.idl:232
> > >> +        void convert5(in [TreatNullAs=NullString, TreatUndefinedAs=NullString] e value);
> > > 
> > > 'a', 'b', 'd', 'e' are not valid type names. Shall we change them to DOMString or something?
> > 
> > I think, 'a', 'b', 'd', 'e' are for checking the case where some user-defined class is specified. WebIDL allows users to use any identifier as NonAnyType.
> 
> OK. Then let's change them to 'UserDefinedType' or something like that. You can do it in a follow-up patch.
> 
> 
> > >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:168
> > >> +    return v8Integer(imp->longAttr(), info.GetIsolate());
> > > 
> > > I know that this code is conformed to the spec. But isn't there any compatibility concern to change a double to an integer?
> > 
> > This double is a bug of old IDLParser.pm. TestObj.idl has "attribute long longAttr;". The old IDLParser.pm cannot parse the line correctly. So the parser generates the code for "long long" + "Attr". But the correct result should be "long" + "longAttr".
> > 
> > >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:454
> > >> +    imp->setAttrWithGetterException(v);
> > > 
> > > Where does this change come from? Is it correct?
> > 
> > The code is generated from "attribute long attrWithGetterException getter raises(DOMException);". So 2 methods are generated, one is attrWithGetterExceptionAttrGetter and the other is attrWithGetterExceptionAttrSetter.
> > Since only "getter raises" is specified, "setter" should not raise any exception. Does this make sense?
> 
> Makes sense.
> 
> I'd like to confirm one thing: There are changes in the generated code for bindings/scripts/tests/*.idl, but there is no meaningful change in the generated code for real idl files for JSC/V8/ObjC/GObject/CPP, right?

Yes. 
The problem is caused by a pair of "long"(return type or attribute type) and "longXXX"(method or attribute name). 
So I checked real idl files by using the following:
   find . -name \*.idl -exec grep -H -G "long long[A-Za-z_]" {} \;

I saw only TestObj.idl.



> 
> If it is right, I'm happy to r+ the patch.

[Kentaro Hara]

Comment on attachment 165050 [details]
Patch

I've scanned the patch again, and it looks good. Thank you very much for the great patch!

Please keep watching bots when landing. If something is wrong, it will break some build.

[WebKit Review Bot]

Comment on attachment 165050 [details]
Patch

Rejecting attachment 165050 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Resolving [Supplemental=XXX] dependencies in all IDL files out/Release/obj/gen/supplemental_dependency.tmp
Unexpected token [ at 18:         [V8MeasureAs=WebAudioStart] [ImplementedAs=startGrain] void start(in double when, in double grainOffset, in double grainDuration); IDLParser.pm:426 at ../bindings/scripts/IDLParser.pm line 92.
 in ../Modules/webaudio/AudioBufferSourceNode.idl at ../bindings/scripts/IDLParser.pm line 116.
make: *** [out/Release/obj/gen/supplemental_dependency.tmp] Error 255

Full output: http://queues.webkit.org/results/14021036

[Takashi Sakamoto]

Created attachment 165914 [details]
Patch

[Kentaro Hara]

Comment on attachment 165914 [details]
Patch

Here we go!

[Takashi Sakamoto]

(In reply to comment #61)
> (From update of attachment 165050 [details])
> Rejecting attachment 165050 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> Resolving [Supplemental=XXX] dependencies in all IDL files out/Release/obj/gen/supplemental_dependency.tmp
> Unexpected token [ at 18:         [V8MeasureAs=WebAudioStart] [ImplementedAs=startGrain] void start(in double when, in double grainOffset, in double grainDuration); IDLParser.pm:426 at ../bindings/scripts/IDLParser.pm line 92.
>  in ../Modules/webaudio/AudioBufferSourceNode.idl at ../bindings/scripts/IDLParser.pm line 116.
> make: *** [out/Release/obj/gen/supplemental_dependency.tmp] Error 255
> 
> Full output: http://queues.webkit.org/results/14021036

As the new IDLParser only allow restricted syntax, "[V8MeasureAs=WebAudioStart] [ImplementedAs=startGrain]" cannot be parsed.
I modified the IDL to use ",", i.e. "[V8MeasureAs=WebAudioStart, ImplementedAs=startGrain]"

Best regards,
Takashi Sakamoto

[Kentaro Hara]

Committed r129723: <http://trac.webkit.org/changeset/129723>

[Kentaro Hara]

Landed manually to avoid style check errors.

[Erik Arvidsson]

(In reply to comment #65)
> Committed r129723: <http://trac.webkit.org/changeset/129723>

Awesome. Much appreciated

[Mark Rowe (bdash)]

This broke our internal Safari build. We have code that makes use of IDLParser, and this change appears to have broken the parsing of some extended attributes that were formerly valid ([SomeName=Some::Value], where Some::Value is a C++ identifier). Can you suggest a quick fix for this, or should we consider rolling this out until we come up with some way to rework our use of IDLParser?

[Kentaro Hara]

(In reply to comment #68)
> This broke our internal Safari build. We have code that makes use of IDLParser, and this change appears to have broken the parsing of some extended attributes that were formerly valid ([SomeName=Some::Value], where Some::Value is a C++ identifier). Can you suggest a quick fix for this, or should we consider rolling this out until we come up with some way to rework our use of IDLParser?

It will be an easy fix. tasak@ is uploading a follow-up patch. One sec please.

[Mark Rowe (bdash)]

Which bug will it be attached to?

[Kentaro Hara]

(In reply to comment #70)
> Which bug will it be attached to?

Will be to this bug (in 20 mins).

[Mark Rowe (bdash)]

Thanks for looking in to it so promptly!

[Takashi Sakamoto]

(In reply to comment #70)
> Which bug will it be attached to?

I'm sorry. I will update soon.

I'm now checking my new patch by using build-webkit or building DumpRenderTree.
The original run-bindings-tests passed and modified run-bindings-tests (i.e. modified TestObj.idl, i.e. =Test::TestObject) passed (passed means the same result as the original IDLParser.pm's result).

Best regards,
Takashi Sakamoto

[Kenichi Ishibashi]

Requested by Takashi

[Takashi Sakamoto]

Created attachment 165936 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #74)
> Requested by Takashi

Thank you, Ishibashi-san.

I have just uploaded my new patch.
The patch passes all rub-bindings-tests and buld-webkit and building DumpRenderTree.

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 165936 [details]
Patch

Rejecting attachment 165936 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
1746.
Hunk #13 FAILED at 1757.
Hunk #14 FAILED at 1768.
Hunk #15 FAILED at 1779.
Hunk #16 FAILED at 1825.
Hunk #17 FAILED at 1836.
Hunk #18 FAILED at 1847.
Hunk #19 FAILED at 1858.
Hunk #20 FAILED at 1924.
Hunk #21 FAILED at 2040.
Hunk #22 FAILED at 2180.
22 out of 22 hunks FAILED -- saving rejects to file Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14036700

[Mark Rowe (bdash)]

That new patch seems to include the entirety of your previous patch which was already landed. I think we'd only need the diff against TOT in order to get it landed?

[Takashi Sakamoto]

Created attachment 165937 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #79)
> Created an attachment (id=165937) [details]
> Patch

Sorry. I have just uploaded the difference between the newest one and the submitted one.

Best regards,
Takashi Sakamoto

[Kentaro Hara]

Comment on attachment 165937 [details]
Patch

Looks OK as a temporal fix. (Later, we might want to kill the [XXX=YYY::ZZZ] syntax and then remove this code.)

[WebKit Review Bot]

Comment on attachment 165937 [details]
Patch

Rejecting attachment 165937 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14032727

[Mark Rowe (bdash)]

It needs a ChangeLog entry before the commit queue can process it…

[Mark Rowe (bdash)]

I made up a ChangeLog entry and landed this myself in r129737 since I can't leave our build broken all night. Thanks for taking the time to fix this.