[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


Kentaro Hara <haraken at chromium.org> changed:

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




--- Comment #49 from Kentaro Hara <haraken at chromium.org>  2012-09-19 00:52:57 PST ---
(From update of attachment 164672)
View in context: https://bugs.webkit.org/attachment.cgi?id=164672&action=review

Getting close to LGTM.

> Source/WebCore/ChangeLog:11
> +        Firstly merges two grammars (editros draft and WebKit current IDL) and

Typo: editors

> Source/WebCore/bindings/scripts/IDLParser.pm:67
> +    my $self = shift;
> +    my $token = shift;
> +    my $value = shift;
> +    my $line = shift;

It looks like $line is not used by any caller. Maybe you need to add __LINE__ to all callers?

> Source/WebCore/bindings/scripts/IDLParser.pm:70
> +        $msg = $msg . $line;

$msg .= " IDLParser.pm:" . $line;

> Source/WebCore/bindings/scripts/IDLParser.pm:90
> +        $msg = $msg . " IDLParser.pm:" . $line;

Nit: $msg .= " IDLParser.pm:" . $line;

> Source/WebCore/bindings/scripts/IDLParser.pm:116
> +        if ($next->type() != EmptyToken) {
> +            die "Expect EOF, but found " . $next->value() . " at " . $self->{Line};
>          }

You can use assertTokenType().

> Source/WebCore/bindings/scripts/IDLParser.pm:131
> +        
> +    die "No document found" unless ref($document) eq "idlDocument";

Remove this. This should not happen.

> Source/WebCore/bindings/scripts/IDLParser.pm:165
> +        $self->{LineNumber}++ while ($skipped =~ /\n/g);

Can be an infinite loop?

> Source/WebCore/bindings/scripts/IDLParser.pm:167
> +        if ($self->{DocumentContent} =~ /^([^\n\r]+)/) {
> +            $self->{Line} = $self->{LineNumber} . ":" . $1;

If you match '\n\n\n\n\n', then '\n\n\n\n\n' is appended. Is it expected?

> Source/WebCore/bindings/scripts/IDLParser.pm:510
> +    return;

Nit: Remove this.

> Source/WebCore/bindings/scripts/IDLParser.pm:628
> +    return;

Nit: Remove this. The same comment for all other last 'return's.

> Source/WebCore/bindings/scripts/IDLParser.pm:1239
> +        if ($type =~ /^(.*)\?$/) {

Nit: $type =~ /\?$/

> Source/WebCore/bindings/scripts/IDLParser.pm:1254
> +        if ($type =~ /^(.*)\?$/) {

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:1346
> +        my $attrs = {};
> +        my $attr = $self->parseExtendedAttribute();

These two variable names are confusing.

> Source/WebCore/bindings/scripts/IDLParser.pm:1369
> +            my $attr = $self->parseExtendedAttribute2();

Ditto.

> Source/WebCore/bindings/scripts/test/TestObj.idl:54
> -        JS, V8
> +        // JS, V8

Shall we remove this comment?

> Source/WebCore/bindings/scripts/test/TestObj.idl:232
> +        void convert1(in [TreatReturnedNullStringAs=Null] a value);
> +        void convert2(in [TreatReturnedNullStringAs=Undefined] b value);
> +        void convert4(in [TreatNullAs=NullString] d value);
> +        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?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:168
> -    return v8::Number::New(static_cast<double>(imp->attr()));
> +    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?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:176
> -    long long v = toInt64(value);
> -    imp->setAttr(WTF::getPtr(v));
> +    int v = toInt32(value);
> +    imp->setLongAttr(v);

Ditto. Isn't there any compatibility concern to change a long long to an int?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:454
> -    ExceptionCode ec = 0;
> -    imp->setAttrWithGetterException(v, ec);
> -    if (UNLIKELY(ec))
> -        setDOMException(ec, info.GetIsolate());
> +    imp->setAttrWithGetterException(v);

Where does this change come from? Is it correct?

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