[webkit-reviews] review denied: [Bug 26398] Move IDL extended attributes to the location specified in WebIDL : [Attachment 164672] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 26398: Move IDL extended attributes to the location specified in WebIDL
https://bugs.webkit.org/show_bug.cgi?id=26398

Attachment 164672: Patch
https://bugs.webkit.org/attachment.cgi?id=164672&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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?