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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 54908: patch
https://bugs.webkit.org/attachment.cgi?id=54908&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
Thanks so much for writing this patch.	I'm sure we'll have to iterate on this
code as we port more of the callbacks to autogen, but this looks like a great
start.


WebCore/bindings/scripts/CodeGeneratorV8.pm:2266
 +		pop(@implContent);  # remove the last comma and newline
Isn't there some sort of join function that can do this more cleanly?

WebCore/bindings/scripts/CodeGeneratorV8.pm:2761
 +  sub GetNativeTypeForCallbacks
It's strange that we have this for V8 but not for jsc.

WebCore/bindings/scripts/test/JS/JSTestCallback.h:44
 +	COMPILE_ASSERT(false)	 virtual int
callbackWithNonBoolReturnType(ScriptExecutionContext*, Class3* class3Param);
I don't think this is quite right, but it will have the desired effect of not
compiling.

WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:37
 +	, m_frame(frame)
I don't think holding the frame is right, as we discussed earlier.  However I
think that's ok for now given that this is what the custom code does.

WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:66
 +	return !invokeCallback(m_callback, 1, argv, callbackReturnValue);
It's strange to see this constant here, but I guess it makes sense.