Bug 57948 – JSC bindings generator: support non-object numbers as callback arguments

• Component: WebCore JavaScript
• Status: NEW

[Kinuko Yasuda]

Current JSC bindings generator does not assumes callback method's argument type is always String or Object, but non-object numbers should be also supported.

[Kinuko Yasuda]

(In reply to comment #0)
> Current JSC bindings generator does not assumes callback method's argument type is always String or Object, but non-object numbers should be also supported.

(Fixing the description... "does not assumes" -> "assumes")

Current JSC bindings generator assumes callback method's argument type is always String or Object, but non-object numbers should be also supported.

[Kinuko Yasuda]

Created attachment 88419 [details]
Patch

[Kinuko Yasuda]

Created attachment 88424 [details]
Patch

Updated ChangeLog comments.

[Darin Adler]

Comment on attachment 88424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88424&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:141
> +bool JSTestCallback::callbackWithInteger(int* intParam)
> +{
> +    if (!canInvokeCallback())
> +        return true;
> +
> +    RefPtr<JSTestCallback> protect(this);
> +
> +    JSLock lock(SilenceAssertionsOnly);
> +
> +    MarkedArgumentBuffer args;
> +    args.append(jsNumber(static_cast<int>(intParam)));

This is definitely wrong and won’t even compile. You can’t just cast an int* to an int.

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 88424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88424&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:141
> > +bool JSTestCallback::callbackWithInteger(int* intParam)
> > +{
> > +    if (!canInvokeCallback())
> > +        return true;
> > +
> > +    RefPtr<JSTestCallback> protect(this);
> > +
> > +    JSLock lock(SilenceAssertionsOnly);
> > +
> > +    MarkedArgumentBuffer args;
> > +    args.append(jsNumber(static_cast<int>(intParam)));
> 
> This is definitely wrong and won’t even compile. You can’t just cast an int* to an int.

Hmm, it has compiled and worked in my testing environment.  Looking into the generated JS file, the callback method seems to be given a int parameter (or long long parameter if we pass a long long one).  Would you mind giving some hint how I could fix the code?

[Kinuko Yasuda]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 88424 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88424&action=review
> > 
> > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:141
> > > +bool JSTestCallback::callbackWithInteger(int* intParam)
> > > +{
> > > +    if (!canInvokeCallback())
> > > +        return true;
> > > +
> > > +    RefPtr<JSTestCallback> protect(this);
> > > +
> > > +    JSLock lock(SilenceAssertionsOnly);
> > > +
> > > +    MarkedArgumentBuffer args;
> > > +    args.append(jsNumber(static_cast<int>(intParam)));
> > 
> > This is definitely wrong and won’t even compile. You can’t just cast an int* to an int.
> 
> Hmm, it has compiled and worked in my testing environment.  Looking into the generated JS file, the callback method seems to be given a int parameter (or long long parameter if we pass a long long one).  Would you mind giving some hint how I could fix the code?

Ah ok, the generated test code looks wrong.  Let me take a look at it again.

[Kinuko Yasuda]

Created attachment 88571 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 88424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88424&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:141
> > +bool JSTestCallback::callbackWithInteger(int* intParam)
> > +{
> > +    if (!canInvokeCallback())
> > +        return true;
> > +
> > +    RefPtr<JSTestCallback> protect(this);
> > +
> > +    JSLock lock(SilenceAssertionsOnly);
> > +
> > +    MarkedArgumentBuffer args;
> > +    args.append(jsNumber(static_cast<int>(intParam)));
> 
> This is definitely wrong and won’t even compile. You can’t just cast an int* to an int.

Seems like int was not considered as a native type in JSC.  Added some more checks and updated the test code.

[Kinuko Yasuda]

Created attachment 88574 [details]
Patch

rebased.

[Kinuko Yasuda]

Created attachment 88576 [details]
Patch

Rebased.

[Kinuko Yasuda]

Could someone take a look at this patch again?  Thanks,

[Darin Adler]

Comment on attachment 88576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88576&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2290
> +                    push(@implContent, "    args.append(jsNumber(static_cast<$paramType>(${paramName})));\n");

Why the static_cast? The code should work fine, perhaps better, without it.

[Maciej Stachowiak]

Comment on attachment 88576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88576&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2290
>> +                    push(@implContent, "    args.append(jsNumber(static_cast<$paramType>(${paramName})));\n");
> 
> Why the static_cast? The code should work fine, perhaps better, without it.

Indeed, the static_cast seems wrong in all places that its used so r- for that. Please revise and resubmit.

[Kinuko Yasuda]

Created attachment 91472 [details]
Removed unnecessary static_casts.

[Darin Adler]

Comment on attachment 91472 [details]
Removed unnecessary static_casts.

View in context: https://bugs.webkit.org/attachment.cgi?id=91472&action=review

I get the impression that the regression test results are out of date. Your patch includes both your changes and an update to make the tests right. It would be better to first land a fix to the test results so that your patch only covered your changes, not unrelated test result updates.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:400
>  
> +
>  JSValue jsTestObjReflectedStringAttr(ExecState* exec, JSValue slotBase, const Identifier&)

Do you know what’s causing all these additional blank lines?

[Eric Seidel (no email)]

Comment on attachment 91472 [details]
Removed unnecessary static_casts.

Please remove the extra blank lines in the generation to make this diff more readable.