[Webkit-unassigned] [Bug 66536] Implement Web IDL Constructor extended attribute in IDLParser.pm and CodeGeneratorV8.pm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66536


Kentaro Hara <haraken at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105588|1                           |0
        is obsolete|                            |




--- Comment #8 from Kentaro Hara <haraken at google.com>  2011-08-30 15:21:08 PST ---
(From update of attachment 105588)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.