Bug 66536 - Implement Web IDL Constructor extended attribute in IDLParser.pm and CodeGeneratorV8.pm
Summary: Implement Web IDL Constructor extended attribute in IDLParser.pm and CodeGene...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 66278 (view as bug list)
Depends on:
Blocks: 65839
  Show dependency treegraph
 
Reported: 2011-08-18 21:44 PDT by Kentaro Hara
Modified: 2011-10-12 01:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (38.98 KB, patch)
2011-08-18 21:55 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (39.18 KB, patch)
2011-08-23 21:59 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (39.21 KB, patch)
2011-08-29 23:44 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (37.68 KB, patch)
2011-08-30 15:20 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (37.70 KB, patch)
2011-08-31 00:45 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-08-18 21:44:26 PDT
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
Comment 1 Kentaro Hara 2011-08-18 21:55:05 PDT
Created attachment 104458 [details]
Patch
Comment 2 Kentaro Hara 2011-08-23 21:59:35 PDT
Created attachment 104961 [details]
Patch
Comment 3 Kentaro Hara 2011-08-29 23:44:28 PDT
Created attachment 105588 [details]
Patch
Comment 4 Kentaro Hara 2011-08-29 23:46:19 PDT
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.
Comment 5 Adam Barth 2011-08-30 01:47:56 PDT
@dominicc: Would you like to do an informal review of this patch?
Comment 6 Dominic Cooney 2011-08-30 13:14:47 PDT
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?
Comment 7 Kentaro Hara 2011-08-30 15:20:44 PDT
Created attachment 105706 [details]
Patch
Comment 8 Kentaro Hara 2011-08-30 15:21:08 PDT
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.
Comment 9 Dominic Cooney 2011-08-30 15:31:03 PDT
> >> 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(…).
Comment 10 Adam Barth 2011-08-30 22:24:43 PDT
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.
Comment 11 WebKit Review Bot 2011-08-31 00:34:53 PDT
Comment on attachment 105706 [details]
Patch

Clearing flags on attachment: 105706

Committed r94156: <http://trac.webkit.org/changeset/94156>
Comment 12 WebKit Review Bot 2011-08-31 00:34:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Kentaro Hara 2011-08-31 00:45:17 PDT
Created attachment 105754 [details]
Patch
Comment 14 Kentaro Hara 2011-08-31 00:48:28 PDT
Sorry, I uploaded the last patch by mistake (after this CL is landed). Please ignore the last patch.
Comment 15 Dominic Cooney 2011-08-31 09:14:24 PDT
Comment on attachment 105754 [details]
Patch

Apparently obsolete.
Comment 16 Kentaro Hara 2011-10-12 01:35:06 PDT
*** Bug 66278 has been marked as a duplicate of this bug. ***