Bug 66536

Created attachment 104458 [details] Patch

Created attachment 104961 [details] Patch

Created attachment 105588 [details] Patch

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.

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

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?

Created attachment 105706 [details] Patch

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.

> >> 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 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 on attachment 105706 [details] Patch Clearing flags on attachment: 105706 Committed r94156: <http://trac.webkit.org/changeset/94156>

All reviewed patches have been landed. Closing bug.

Created attachment 105754 [details] Patch

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

Comment on attachment 105754 [details] Patch Apparently obsolete.

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