WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(15.23 KB, patch)
2011-04-06 08:10 PDT
,
Kinuko Yasuda
darin
: review-
Details
Formatted Diff
Diff
Patch
(17.39 KB, patch)
2011-04-06 22:09 PDT
,
Kinuko Yasuda
no flags
Details
Formatted Diff
Diff
Patch
(36.22 KB, patch)
2011-04-06 22:35 PDT
,
Kinuko Yasuda
no flags
Details
Formatted Diff
Diff
Patch
(17.20 KB, patch)
2011-04-06 22:42 PDT
,
Kinuko Yasuda
no flags
Details
Formatted Diff
Diff
Removed unnecessary static_casts.
(66.99 KB, patch)
2011-04-28 04:35 PDT
,
Kinuko Yasuda
eric
: review-
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 88419
[details]
Patch
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
Created
attachment 88571
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug