Bug 66536 – Implement Web IDL Constructor extended attribute in IDLParser.pm and CodeGeneratorV8.pm

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

[Kentaro Hara]

Currently, all constructors of V8 are manually written as Custom constructors in WebCore/binding/v8/custom/*.cpp. 

IDLParser.pm and CodeGeneratorV8.pm should parse Web IDL Constructor extended attribute, and generate the constructor codes. The spec is here: http://www.w3.org/TR/WebIDL/#Constructor

[Kentaro Hara]

Created attachment 104458 [details]
Patch

[Kentaro Hara]

Created attachment 104961 [details]
Patch

[Kentaro Hara]

Created attachment 105588 [details]
Patch

[Kentaro Hara]

Nit: Changed a 'V8ConstructorSetsDOMWrapper' extended attribute in the previous patches to a 'V8ConstructorSetsActiveDOMWrapper' extended attribute. This is because I found that V8DOMWrapper::setJSWrapperForDOMObject() should be called by default and V8DOMWrapper::setJSWrapperForActiveDOMObject() should be optional. Please see the comment in IsActiveDomType subroutine in CodeGeneratorV8.pm. I will upload a follow-up patch that removes IsActiveDOMType subroutine by using the 'V8ConstructorSetsActiveDOMWrapper' extended attribute in IDL files.

[Adam Barth]

@dominicc: Would you like to do an informal review of this patch?

[Dominic Cooney]

Comment on attachment 105588 [details]
Patch

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

Is it worth including at least *one* thing that uses Constructor, so that you can have more confidence that this works?

> Source/WebCore/ChangeLog:11
> +        - 'Constructor' generates constructorCallback() with no arguments.

Maybe you don’t need so much detail on point [1] since it follows from the spec.

> Source/WebCore/ChangeLog:22
> +        [3] Added 'V8ConstructorSetsActiveDOMWrapper' extended attribute.

Looks like you need to update this.

> Source/WebCore/ChangeLog:26
> +        [4] Added 'ConstructorRaisesException' extended attribute.

When is this used? Because the Web IDL spec does not have 'raises' for constructors. Maybe we should email the spec author and see if it can be added, although the Web IDL spec is already in last call.

> Source/WebCore/ChangeLog:29
> +        - If 'ConstructorRaisesException' and 'ConstructorWith=ScriptExecutionContext'

You probably don’t need to be so explicit, just mention that ConstructorRaisesException and ConstructorWith can be used together.

> Source/WebCore/ChangeLog:42
> +        (GenerateArgumentsCountCheck): Generates a code for checking the number of arguments. This was a code existing in GenerateFunctionCallback(). This patch just moves the code into this method.

Focus on succinctness. eg. "Split out of GenerateFunctionCallback for reuse" or something.

> Source/WebCore/ChangeLog:44
> +        (GenerateConstructorCallback): Generates a code of constructorCallback().

a code -> code

It might be simpler to say "Generates callback definition."

> Source/WebCore/ChangeLog:45
> +        (GenerateImplementation): Just added a new line for readability.

Probably not worth commenting on this.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:357
> +    if ($dataNode->extendedAttributes->{"CanBeConstructed"} || $dataNode->extendedAttributes->{"CustomConstructor"} || $dataNode->extendedAttributes->{"V8CustomConstructor"} || $dataNode->extendedAttributes->{"Constructor"}) {

Seems like you alphabetized these but then added "Constructor" out of order. Be consistent, or at least minimize the diff.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1256
> +    my $argumentsCountCheckString = GenerateArgumentsCountCheck($function, $dataNode);

This variable does not explain anything that is not in the function name. What about just:

push(@implContentDecls, GenerateArgumentsCountCheck($function, $dataNode));

?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1334
> +    push(@implContentDecls, "$callString");

Why "$callString"? Why not just $callString?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1444
> +            if ($function->signature && $function->signature->extendedAttributes->{"StrictTypeChecking"}) {

Why?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1498
> +    push(@implContent, "\n");

Why not just make this part of the preceding here document?

[Kentaro Hara]

Created attachment 105706 [details]
Patch

[Kentaro Hara]

Comment on attachment 105588 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        - 'Constructor' generates constructorCallback() with no arguments.
> 
> Maybe you don’t need so much detail on point [1] since it follows from the spec.

Done. Removed the redundant lines.

>> Source/WebCore/ChangeLog:22
>> +        [3] Added 'V8ConstructorSetsActiveDOMWrapper' extended attribute.
> 
> Looks like you need to update this.

Update what? I guess that the description is already up-to-date.

>> Source/WebCore/ChangeLog:26
>> +        [4] Added 'ConstructorRaisesException' extended attribute.
> 
> When is this used? Because the Web IDL spec does not have 'raises' for constructors. Maybe we should email the spec author and see if it can be added, although the Web IDL spec is already in last call.

'ConstructorRaisesException' does not mean that the constructor can raise exception (as 'raises' means),  but just mean that XXX::create() requires a placeholder for ExceptionCode, like XXX::create(..., ec). Thus, I guess that this may be renamed to "ConstructorWith=RaisesException" or something.

>> Source/WebCore/ChangeLog:29
>> +        - If 'ConstructorRaisesException' and 'ConstructorWith=ScriptExecutionContext'
> 
> You probably don’t need to be so explicit, just mention that ConstructorRaisesException and ConstructorWith can be used together.

Done.

>> Source/WebCore/ChangeLog:42

> 
> Focus on succinctness. eg. "Split out of GenerateFunctionCallback for reuse" or something.

Done.

>> Source/WebCore/ChangeLog:44
>> +        (GenerateConstructorCallback): Generates a code of constructorCallback().
> 
> a code -> code
> 
> It might be simpler to say "Generates callback definition."

Done.

>> Source/WebCore/ChangeLog:45
>> +        (GenerateImplementation): Just added a new line for readability.
> 
> Probably not worth commenting on this.

Done. Removed this comment.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:357
>> +    if ($dataNode->extendedAttributes->{"CanBeConstructed"} || $dataNode->extendedAttributes->{"CustomConstructor"} || $dataNode->extendedAttributes->{"V8CustomConstructor"} || $dataNode->extendedAttributes->{"Constructor"}) {
> 
> Seems like you alphabetized these but then added "Constructor" out of order. Be consistent, or at least minimize the diff.

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1256
>> +    my $argumentsCountCheckString = GenerateArgumentsCountCheck($function, $dataNode);
> 
> This variable does not explain anything that is not in the function name. What about just:
> 
> push(@implContentDecls, GenerateArgumentsCountCheck($function, $dataNode));
> 
> ?

Done here and there.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1334
>> +    push(@implContentDecls, "$callString");
> 
> Why "$callString"? Why not just $callString?

Done. Removed $callString.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1444
>> +            if ($function->signature && $function->signature->extendedAttributes->{"StrictTypeChecking"}) {
> 
> Why?

Done. Removed this.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1498
>> +    push(@implContent, "\n");
> 
> Why not just make this part of the preceding here document?

Done.

[Dominic Cooney]

> >> Source/WebCore/ChangeLog:22
> >> +        [3] Added 'V8ConstructorSetsActiveDOMWrapper' extended attribute.
> > 
> > Looks like you need to update this.
> 
> Update what? I guess that the description is already up-to-date.

My bad!

> >> Source/WebCore/ChangeLog:26
> >> +        [4] Added 'ConstructorRaisesException' extended attribute.
> > 
> > When is this used? Because the Web IDL spec does not have 'raises' for constructors. Maybe we should email the spec author and see if it can be added, although the Web IDL spec is already in last call.
> 
> 'ConstructorRaisesException' does not mean that the constructor can raise exception (as 'raises' means),  but just mean that XXX::create() requires a placeholder for ExceptionCode, like XXX::create(..., ec). Thus, I guess that this may be renamed to "ConstructorWith=RaisesException" or something.

It seems weird that something in the WebCore layer (as opposed to bindings layer) is generating an exception, but that is not described in WebIDL with raises(…).

[Adam Barth]

Comment on attachment 105706 [details]
Patch

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

This looks great.  We'll need to do something similar to CodeGeneratorJS if we want to use this in more than a handful of IDL files.

Thanks for writing this patch.

> Source/WebCore/bindings/scripts/IDLParser.pm:158
> +        if ($str =~ /^\s*=/) {
> +            # Parse '=' value | '=' value ','
> +            $str =~ s/^\s*=//;
> +            if ($str =~ /^\s*([^,]*),?/) {

Perl makes me sad face.

[WebKit Review Bot]

Comment on attachment 105706 [details]
Patch

Clearing flags on attachment: 105706

Committed r94156: <http://trac.webkit.org/changeset/94156>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Created attachment 105754 [details]
Patch

[Kentaro Hara]

Sorry, I uploaded the last patch by mistake (after this CL is landed). Please ignore the last patch.

[Dominic Cooney]

Comment on attachment 105754 [details]
Patch

Apparently obsolete.

[Kentaro Hara]

*** Bug 66278 has been marked as a duplicate of this bug. ***