[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





--- Comment #6 from Dominic Cooney <dominicc at chromium.org>  2011-08-30 13:14:47 PST ---
(From update of attachment 105588)
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?

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