[webkit-reviews] review denied: [Bug 38414] Auto-generate the callbacks code : [Attachment 54899] patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 38414: Auto-generate the callbacks code
https://bugs.webkit.org/show_bug.cgi?id=38414

Attachment 54899: patch
https://bugs.webkit.org/attachment.cgi?id=54899&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This is looking great.	A few small comments below.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1909
 +	push(@headerContentHeader, "\n#define $className" . "_h\n\n");
This must be copy/paste from elsewhere in this file.  Can we factor the common
code into a subroutine?

WebCore/bindings/scripts/CodeGeneratorJS.pm:2022
 +		    !($params[0]->type eq "ScriptExecutionContext")) {
Maybe add a COMPILE_ASSERT(false) to explain that we don't support these cases
yet?

WebCore/bindings/scripts/test/TestCallback.idl:36
 +	  boolean callbackWithClass1Param(in ScriptExecutionContext context, in
Class1 class1Param);
I wonder if we should omit the ScriptExecutionContext parameter from this
declaration.  We can use a [CallWith=ScriptExecutionContext] attribute instead.
 You can see examples of how we handled [CallWith=ScriptState] in other IDL
files.

WebCore/bindings/scripts/test/JS/JSTestCallback.h:44
 +	// Non-bool return type for non-custom function:    int
callbackWithNonBoolReturnType(ScriptExecutionContext* context, Class3*
class3Param);
I don't think we should have commented out code.  If these are an error to use
in the IDL file, we should generated COMPILE_ASSERT(false).