[webkit-reviews] review denied: [Bug 80573] Support [Custom] for static functions : [Attachment 130781] Patch made with -w

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Jon Lee <jonlee at apple.com>'s
request for review:
Bug 80573: Support [Custom] for static functions
https://bugs.webkit.org/show_bug.cgi?id=80573

Attachment 130781: Patch made with -w
https://bugs.webkit.org/attachment.cgi?id=130781&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review


>>> Source/WebCore/ChangeLog:32
>>> +	     (V8TestObj):
>> 
>> It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not
included in this patch. Would you please update the patch correctly? BTW, don't
we need to modify CodeGenerator{GObject,CPP}.pm?
> 
> Those are just what the scripts produced as a result of the new test case. I
did not change the generation scripts. But you're right for V8; I see some
missing generated code, so I'll work on that. I might need some assistance
because I'm unfamiliar with that area.
> 
> Looking at the existing test output code for ObjC, GObject, and CPP, it looks
like static isn't used for those languages, because the generated code is
already incorrect. I will file bugs to track that work, but don't think it
belongs in this bug since it just extends the existing static generation code.

Sorry for the confusion. You are right. In my understanding,

- We need to modify CodeGeneratorJS.pm.
- We do not need to modify CodeGeneratorV8.pm, since it has already supported
[Custom] static, fortunately.
- We do not need to modify CodeGenerator{ObjC, GObject, CPP}.pm, since they
have not yet supported custom bindings.

Is my understanding correct?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094
> +	       if ($function->isStatic) {
> +		   if ($isCustom) {
> +		       GenerateArgumentsCountCheck(\@implContent, $function,
$dataNode);
> +		       push(@implContent, "    return
JSValue::encode(${className}::" . $functionImplementationName . "(exec));\n");
> +		   } else {
> +		       GenerateArgumentsCountCheck(\@implContent, $function,
$dataNode);
> +
> +		       if (@{$function->raisesExceptions}) {
> +			   push(@implContent, "    ExceptionCode ec = 0;\n");
> +		       }
> +
> +		       my $numParameters = @{$function->parameters};
> +		       my ($functionString, $dummy) =
GenerateParametersCheck(\@implContent, $function, $dataNode, $numParameters,
$implClassName, $functionImplementationName, $svgPropertyType,
$svgPropertyOrListPropertyType, $svgListPropertyType);
> +		       GenerateImplementationFunctionCall($function,
$functionString, "    ", $svgPropertyType, $implClassName);
> +		   }

Can't we remove this block? It seems this block is just a duplication of the
line 2119 -- 2153.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2098
>	       if ($interfaceName eq "DOMWindow") {
>		   push(@implContent, "    $className* castedThis =
toJSDOMWindow(exec->hostThisValue().toThisObject(exec));\n");
>		   push(@implContent, "    if (!castedThis)\n");

Nit: Indentation seems incorrect around here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2154
> +	       }

Nit: Indentation seems incorrect around here.