[webkit-reviews] review requested: [Bug 38414] Auto-generate the callbacks code : [Attachment 54963] 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 54963: patch
https://bugs.webkit.org/attachment.cgi?id=54963&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
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'.