NEW 57948
JSC bindings generator: support non-object numbers as callback arguments
https://bugs.webkit.org/show_bug.cgi?id=57948
Summary JSC bindings generator: support non-object numbers as callback arguments
Kinuko Yasuda
Reported 2011-04-06 07:51:22 PDT
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.
Attachments
Patch (14.93 KB, patch)
2011-04-06 08:03 PDT, Kinuko Yasuda
no flags
Patch (15.23 KB, patch)
2011-04-06 08:10 PDT, Kinuko Yasuda
darin: review-
Patch (17.39 KB, patch)
2011-04-06 22:09 PDT, Kinuko Yasuda
no flags
Patch (36.22 KB, patch)
2011-04-06 22:35 PDT, Kinuko Yasuda
no flags
Patch (17.20 KB, patch)
2011-04-06 22:42 PDT, Kinuko Yasuda
no flags
Removed unnecessary static_casts. (66.99 KB, patch)
2011-04-28 04:35 PDT, Kinuko Yasuda
eric: review-
Kinuko Yasuda
Comment 1 2011-04-06 07:52:38 PDT
(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
Comment 2 2011-04-06 08:03:01 PDT
Kinuko Yasuda
Comment 3 2011-04-06 08:10:38 PDT
Created attachment 88424 [details] Patch Updated ChangeLog comments.
Darin Adler
Comment 4 2011-04-06 09:31:16 PDT
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
Comment 5 2011-04-06 18:57:06 PDT
(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
Comment 6 2011-04-06 18:58:01 PDT
(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
Comment 7 2011-04-06 22:09:06 PDT
Kinuko Yasuda
Comment 8 2011-04-06 22:10:45 PDT
(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
Comment 9 2011-04-06 22:35:25 PDT
Created attachment 88574 [details] Patch rebased.
Kinuko Yasuda
Comment 10 2011-04-06 22:42:45 PDT
Created attachment 88576 [details] Patch Rebased.
Kinuko Yasuda
Comment 11 2011-04-11 23:24:59 PDT
Could someone take a look at this patch again? Thanks,
Darin Adler
Comment 12 2011-04-26 16:06:29 PDT
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 13 2011-04-26 17:04:39 PDT
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
Comment 14 2011-04-28 04:35:51 PDT
Created attachment 91472 [details] Removed unnecessary static_casts.
Darin Adler
Comment 15 2011-04-29 10:58:42 PDT
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 16 2011-05-01 10:45:41 PDT
Comment on attachment 91472 [details] Removed unnecessary static_casts. Please remove the extra blank lines in the generation to make this diff more readable.
Note You need to log in before you can comment on or make changes to this bug.