Bug 38414 – Auto-generate the callbacks code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

CodeGenerator{JS|V8}.pm should be able to auto-generate code for most callbacks.

[Dumitru Daniliuc]

Created attachment 54842 [details]
patch

Attaching only the CodeGeneratorJS.pm changes for now (will port to CodeGeneratorV8.pm once we have a final CodeGeneratorJS.pm version).

The patch adds support for callbacks that do not return a value.

The developers will be expected to provide the MyCallback.h and MyCallback.idl files as follows:

MyCallback.h:
namespace WebCore {

class MyClass1;
class MyClass2;
class ScriptExecutionContext;

class MyCallback {
public:
    virtual ~MyCallback();
    virtual bool doStuffWithClass1(ScriptExecutionContext* context, MyClass1* obj1) = 0;
    virtual bool doStuffWithClass2(ScriptExecutionContext* context, MyClass2* obj2, ... other input parameters ... ) = 0;
};

} // namespace WebCore


MyCallbacks.idl
module SomeModule {
    interface [
        Callback
    ] MyCallbacks {
        boolean doStuffWithClass1(in ScriptExecutionContext context, in MyClass1 obj1);
        boolean doStuffWithClass2(in ScriptExecutionContext context, in MyClass2 obj2, ... other input parameters ... );
    }
}

Then the code generators will auto-generate a MyCallback implementation that will invoke the callback with the given parameters.

Restrictions:
-- First parameter must be a ScriptExecutionContext.
-- The return type must be bool (but can be ignored). It tells the developer if an exception was thrown while invoking the callback.

[Adam Barth]

Excited about this patch.  Will review in the next few days.

[Eric Seidel (no email)]

Comment on attachment 54842 [details]
patch

Please add a test of this to TestObject.idl (used by run-bindings-tests).  That would make this easier to review.

[Dumitru Daniliuc]

Created attachment 54895 [details]
patch

Added a test case.

[WebKit Review Bot]

Attachment 54895 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:63:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:84:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dumitru Daniliuc]

Created attachment 54899 [details]
patch

Fixing the style problem in the auto-generated code.

[Adam Barth]

Comment on attachment 54899 [details]
patch

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

[Dumitru Daniliuc]

Created attachment 54908 [details]
patch

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

[Adam Barth]

Comment on attachment 54908 [details]
patch

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.

[Dumitru Daniliuc]

Created attachment 54963 [details]
patch

adam, please take another look, just to be sure everything's fine.

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

done:

@argvs = ();
foreach my $param (@params) {
    my $paramName = $param->name;
    push(@argvs, "        toV8(${paramName})");
}
push(@implContent, join(",\n", @argvs));


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

the GetNativeType() method in the JSC code generator works just fine. the one in the V8 code generator though wants to either convert DOMString to V8Parameter or wrap all pointers in RefPtrs, and i'm not sure how to get around that.

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

what would be a better way to do this?

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

sounds good.

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

oops, this was a bug. instead of 1 it should be the length of 'argv'.

[Dumitru Daniliuc]

(In reply to comment #10)
> @argvs = ();
> foreach my $param (@params) {
>     my $paramName = $param->name;
>     push(@argvs, "        toV8(${paramName})");
> }
> push(@implContent, join(",\n", @argvs));

hmm, should be "my @argvs". will fix on landing.

[Adam Barth]

Comment on attachment 54963 [details]
patch

Okiedokes!

WebCore/bindings/scripts/CodeGeneratorJS.pm:525
 +  sub GenerateHeaderContentHeader
The pattern we prefer here is to return a local @code and have the caller push it onto @headerContentHeader.  That way we have fewer mucking with global variables.

WebCore/bindings/scripts/CodeGeneratorV8.pm:2218
 +      push(@implContent, <<END);
It's unclear when you decide to use this patter and when you use strings directly.  I'm not sure it really matters, but we should aim for consistency in the long term.

WebCore/bindings/scripts/CodeGeneratorV8.pm:2261
 +              @argvs = ();
my @argvs

WebCore/bindings/scripts/CodeGeneratorV8.pm:2761
 +  sub GetNativeTypeForCallbacks
This function continues to make me sad.  We should rationalize how this works in the two generators someday.

[Dumitru Daniliuc]

> WebCore/bindings/scripts/CodeGeneratorJS.pm:525
>  +  sub GenerateHeaderContentHeader
> The pattern we prefer here is to return a local @code and have the caller push
> it onto @headerContentHeader.  That way we have fewer mucking with global
> variables.

done.

> WebCore/bindings/scripts/CodeGeneratorV8.pm:2218
>  +      push(@implContent, <<END);
> It's unclear when you decide to use this patter and when you use strings
> directly.  I'm not sure it really matters, but we should aim for consistency in
> the long term.

in the JSC generator push(@implContent, <<END) doesn't seem to be used at all, so i didn't use it there. in the V8 generator it seems to be used whenever a large block of code that doesn't depend on any perl variable needs to be written to a file, so that's when i tried to use it too.

> WebCore/bindings/scripts/CodeGeneratorV8.pm:2261
>  +              @argvs = ();
> my @argvs

fixed.

[Dumitru Daniliuc]

Landed as r58710.