Bug 71756 – CodeGeneratorV8.pm can generate a NamedConstructor

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

IDLParser.pm should parse [NamedConstructor] IDL and CodeGeneratorV8.pm should generate a NamedConstructor. 
The spec: http://www.w3.org/TR/WebIDL/#NamedConstructor

[Kentaro Hara]

Created attachment 114098 [details]
Patch

[Kentaro Hara]

I guess that we can ignore style errors in this patch, since their cause is in CodeGeneratorGObject.pm and CodeGeneratorObjC.pm.

[WebKit Review Bot]

Attachment 114098 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestNamedConstructorInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:22:  Found WebCore config.h after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:24:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:120:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:145:  Extra space between return and WEBKIT_DOM_TEST_NAMED_CONSTRUCTOR  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:21:  #ifndef header guard has wrong style, please use: WebKitDOMTestNamedConstructorPrivate_h  [build/header_guard] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 13 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 114098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114098&action=review

I feel slightly like we're copying and pasting code in the code generator, but I think it's ok.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1659
> +    if (!document)
> +        return throwError("${implClassName} constructor associated document is unavailable", V8Proxy::ReferenceError);

This can't occur.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1711
> +    obj->ref();

Yuck.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1719
> +    if ($raisesExceptions) {
> +        push(@implContent, "  fail:\n");
> +        push(@implContent, "    return throwError(ec);\n");
> +    }

Why do we need goto?  There only seems to be one goto.  It seems like we should just put this code there.

> Source/WebCore/bindings/scripts/IDLParser.pm:155
> +            if ($name eq "NamedConstructor") {

These regexps are kind of mysterious to me.

[Kentaro Hara]

Comment on attachment 114098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114098&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1659
>> +        return throwError("${implClassName} constructor associated document is unavailable", V8Proxy::ReferenceError);
> 
> This can't occur.

OK, I will remove this. (But I don't know if it cannot occur. Current HTMLXXXXElementConstructor.cpp has this check.)

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1711
>> +    obj->ref();
> 
> Yuck.

What do you mean?

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1719
>> +    }
> 
> Why do we need goto?  There only seems to be one goto.  It seems like we should just put this code there.

GenerateNamedConstructorCallback calls GenerateParamtersCheck, and GenerateParamtersCheck can generate goto.

[Adam Barth]

(In reply to comment #5)
> (From update of attachment 114098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114098&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1659
> >> +        return throwError("${implClassName} constructor associated document is unavailable", V8Proxy::ReferenceError);
> > 
> > This can't occur.
> 
> OK, I will remove this. (But I don't know if it cannot occur. Current HTMLXXXXElementConstructor.cpp has this check.)

It used to be possible in the distant past, but not anymore.

> >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1711
> >> +    obj->ref();
> > 
> > Yuck.
> 
> What do you mean?

We try to avoid manual reference counting whenever possible.  I understand that this is how the hand-written version of this code used to work.

> >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1719
> >> +    }
> > 
> > Why do we need goto?  There only seems to be one goto.  It seems like we should just put this code there.
> 
> GenerateNamedConstructorCallback calls GenerateParamtersCheck, and GenerateParamtersCheck can generate goto.

I see.  Thanks.

[Kentaro Hara]

Committed r99641: <http://trac.webkit.org/changeset/99641>

[Kentaro Hara]

Comment on attachment 114098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114098&action=review

>>>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1659
>>>> +        return throwError("${implClassName} constructor associated document is unavailable", V8Proxy::ReferenceError);
>>> 
>>> This can't occur.
>> 
>> OK, I will remove this. (But I don't know if it cannot occur. Current HTMLXXXXElementConstructor.cpp has this check.)
> 
> It used to be possible in the distant past, but not anymore.

I got it, thanks. Removed it.

I committed this patch manually to avoid the style error check.