[Webkit-unassigned] [Bug 26398] Move IDL extended attributes to the location specified in WebIDL
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 25 02:46:43 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=26398
--- Comment #31 from Takashi Sakamoto <tasak at google.com> 2012-07-25 02:46:44 PST ---
(From update of attachment 151870)
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.
--
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