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

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	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 Dumitru Daniliuc <dumi at chromium.org>
Ported the changes to the V8 code generator too.

Question about V8 callbacks: do we still need to store the current frame and
protect it? Or should we rather protect the callback instance like we do in the
JSC implementation?

> 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?

done. added GenerateHeaderContentHeader and GenerateImplementationContentHeader
functions to both code generators.

> 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?

i changed the generators to automatically insert a ScriptExecutionContext*
parameter.

> 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.

Omitted the ScriptExecutionContext. I don't think we need a [CallWith]
attribute, at least not at this time. It seems to me that all callbacks need to
be called with a ScriptExecutionContext parameter (or at least, the
auto-generated implementation assumes that). If in the future this assumption
needs to change, we can add a [CallWith] attribute then.

> 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).

Done, but not sure if I did it right. Please take a look at the generated .h
files and let me know if I used COMPILE_ASSERT(false) correctly.

By the way, the copyright headers for JSC and V8 generated files have different
addresses for FSF (last 2 lines).