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.
(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.
Created attachment 88419 [details] Patch
Created attachment 88424 [details] Patch Updated ChangeLog comments.
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.
(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?
(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.
Created attachment 88571 [details] Patch
(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.
Created attachment 88574 [details] Patch rebased.
Created attachment 88576 [details] Patch Rebased.
Could someone take a look at this patch again? Thanks,
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.
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.
Created attachment 91472 [details] Removed unnecessary static_casts.
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?
Comment on attachment 91472 [details] Removed unnecessary static_casts. Please remove the extra blank lines in the generation to make this diff more readable.