[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 #53 from Takashi Sakamoto <tasak at google.com>  2012-09-19 23:10:50 PST ---
(From update of attachment 164672)
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?

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