[webkit-reviews] review denied: [Bug 26398] Move IDL extended attributes to the location specified in WebIDL : [Attachment 151870] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 26398: Move IDL extended attributes to the location specified in WebIDL
https://bugs.webkit.org/show_bug.cgi?id=26398

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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|att
ribute|static|readonly|deleter|dictionary|\:\:|unsigned|optional|setter|interfa
ce|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|att
ribute|static|readonly|deleter|dictionary|\:\:|unsigned|optional|setter|interfa
ce|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|om
ittable|float|attribute|static|readonly|deleter|boolean|\:\:|unsigned|setter|Da
te|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.