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

[bugzilla-daemon at webkit.org]

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





--- Comment #45 from Takashi Sakamoto <tasak at google.com>  2012-08-10 01:40:35 PST ---
(From update of attachment 155495)
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.

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