[Webkit-unassigned] [Bug 26398] Move IDL extended attributes to the location specified in WebIDL

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jul 22 22:13:25 PDT 2012 

https://bugs.webkit.org/show_bug.cgi?id=26398


Kentaro Hara <haraken at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151870|review?                     |review-
               Flag|                            |




--- Comment #28 from Kentaro Hara <haraken at chromium.org>  2012-07-22 22:13:27 PST ---
(From update of attachment 151870)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list