Bug 49704

Summary: Make binding code generation scripts support 'short' type
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore JavaScriptAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, kbr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Proposed Patch
kbr: review-, jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch kbr: review+, jianli: commit-queue-

Jian Li
Reported 2010-11-17 18:04:11 PST
The current code generation scripts have some handling for 'short' type, but incomplete. We need to enhance the scripts to support using 'short' type in IDL. This is needed in DataView idl.
Attachments
Proposed Patch (63.74 KB, patch)
2010-11-17 18:49 PST, Jian Li
kbr: review-
jianli: commit-queue-
Proposed Patch (68.86 KB, patch)
2010-11-18 15:06 PST, Jian Li
jianli: commit-queue-
Proposed Patch (69.42 KB, patch)
2010-11-18 16:21 PST, Jian Li
kbr: review+
jianli: commit-queue-
Jian Li
Comment 1 2010-11-17 18:49:14 PST
Created attachment 74191 [details] Proposed Patch
Kenneth Russell
Comment 2 2010-11-18 13:53:25 PST
Comment on attachment 74191 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74191&action=review This mostly looks good, but r- for the style issues, and I have a couple of additional questions. > WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:62 > - WebCore::JSMainThreadNullState state; > g_return_val_if_fail(self, 0); > + WebCore::JSMainThreadNullState state; These movements of lines of code are a significant fraction of this patch and don't seem directly related to this fix. If this is the case then please split them off into a separate patch. It would seem fine to me to check in support for "short" first and then move this code. > WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:920 > -#if ENABLE(Condition1) > glong > webkit_dom_test_obj_get_conditional_attr1(WebKitDOMTestObj* self) > { > - WebCore::JSMainThreadNullState state; > +#if ENABLE(Condition1) > g_return_val_if_fail(self, 0); > + WebCore::JSMainThreadNullState state; > WebCore::TestObj * item = WebKit::core(self); > glong res = item->conditionalAttr1(); > return res; > -} > +#else > + return static_cast<glong>(0); > #endif /* ENABLE(Condition1) */ > +} Could you explain why the structures of these #ifs are being changed? > WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1461 > + "test_obj_short-attr", /* short description */ > + "read-write gshort TestObj.short-attr", /* longer - could do with some extra doc stuff here */ > + G_MININT, /* min */ > +G_MAXINT, /* max */ > +0, /* default */ > + WEBKIT_PARAM_READWRITE)); Indentation is messed up here. > WebCore/bindings/scripts/test/JS/JSTestObj.cpp:286 > - JSValue result = jsNumber(exec, imp->readOnlyIntAttr()); > + JSValue result = jsNumber(imp->readOnlyIntAttr()); How were these changes tested? Clearly the earlier code didn't compile, and I don't see any build file changes in this patch. > WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1513 > + UNUSED_PARAM(exec); return jsNumber(static_cast<int>(0)); Minimally you need to split these into two lines. However, a solution that conforms more to WebKit style would be just to declare "ExecState*, (other args)" and remove the UNUSED_PARAM. > WebCore/bindings/scripts/test/TestObj.idl:38 > + attribute short shortAttr; I think you should test both short and unsigned short, in particular including boundary conditions.
Jian Li
Comment 3 2010-11-18 15:05:28 PST
All the changes in *.cpp and *.h are auto-generated by run-binding-tests. The last person who made changes to code generation scripts did not run the script to regenerate the binding test results. So this is the reason that you see quite a few changes in *.cpp and *.h that are unrelated to adding "short" support. Those style errors were introduced by some other people's changes to code generation scripts, for which I rather not to fix. (In reply to comment #2) > (From update of attachment 74191 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74191&action=review > > > WebCore/bindings/scripts/test/TestObj.idl:38 > > + attribute short shortAttr; > > I think you should test both short and unsigned short, in particular including boundary conditions. I am fixing this and will upload a new patch soo.
Jian Li
Comment 4 2010-11-18 15:06:38 PST
Created attachment 74296 [details] Proposed Patch
Kenneth Russell
Comment 5 2010-11-18 15:58:16 PST
(In reply to comment #3) > All the changes in *.cpp and *.h are auto-generated by run-binding-tests. The last person who made changes to code generation scripts did not run the script to regenerate the binding test results. So this is the reason that you see quite a few changes in *.cpp and *.h that are unrelated to adding "short" support. Those style errors were introduced by some other people's changes to code generation scripts, for which I rather not to fix. I see. Looking at CodeGeneratorJS.pm, it would be trivial to at least insert the missing line break after UNUSED_PARAM(exec) (line 2080). If that clears up the style errors I think it is absolutely worth doing. Would you please consider doing this in this patch? > (In reply to comment #2) > > (From update of attachment 74191 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=74191&action=review > > > > > WebCore/bindings/scripts/test/TestObj.idl:38 > > > + attribute short shortAttr; > > > > I think you should test both short and unsigned short, in particular including boundary conditions. > > I am fixing this and will upload a new patch soo. OK. I don't see exactly where (for example) it's tested that setting the unsigned short attribute to 65535 and fetching it results in 65535. Are tests like that handled by the harness?
Jian Li
Comment 6 2010-11-18 16:21:01 PST
Created attachment 74315 [details] Proposed Patch
Jian Li
Comment 7 2010-11-18 16:22:31 PST
(In reply to comment #5) > (In reply to comment #3) > > All the changes in *.cpp and *.h are auto-generated by run-binding-tests. The last person who made changes to code generation scripts did not run the script to regenerate the binding test results. So this is the reason that you see quite a few changes in *.cpp and *.h that are unrelated to adding "short" support. Those style errors were introduced by some other people's changes to code generation scripts, for which I rather not to fix. > > I see. Looking at CodeGeneratorJS.pm, it would be trivial to at least insert the missing line break after UNUSED_PARAM(exec) (line 2080). If that clears up the style errors I think it is absolutely worth doing. Would you please consider doing this in this patch? Done. > > > (In reply to comment #2) > > > (From update of attachment 74191 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=74191&action=review > > > > > > > WebCore/bindings/scripts/test/TestObj.idl:38 > > > > + attribute short shortAttr; > > > > > > I think you should test both short and unsigned short, in particular including boundary conditions. > > > > I am fixing this and will upload a new patch soo. > > OK. I don't see exactly where (for example) it's tested that setting the unsigned short attribute to 65535 and fetching it results in 65535. Are tests like that handled by the harness? Unfortunately, I think the current binding test is only for visual verification purpose. The generated files are not compiled and ran.
Kenneth Russell
Comment 8 2010-11-18 16:46:47 PST
Comment on attachment 74315 [details] Proposed Patch (In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > All the changes in *.cpp and *.h are auto-generated by run-binding-tests. The last person who made changes to code generation scripts did not run the script to regenerate the binding test results. So this is the reason that you see quite a few changes in *.cpp and *.h that are unrelated to adding "short" support. Those style errors were introduced by some other people's changes to code generation scripts, for which I rather not to fix. > > > > I see. Looking at CodeGeneratorJS.pm, it would be trivial to at least insert the missing line break after UNUSED_PARAM(exec) (line 2080). If that clears up the style errors I think it is absolutely worth doing. Would you please consider doing this in this patch? > > Done. Thanks. Unfortunately it doesn't look like that cleared up everything but it's still an improvement. > > > > > (In reply to comment #2) > > > > (From update of attachment 74191 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=74191&action=review > > > > > > > > > WebCore/bindings/scripts/test/TestObj.idl:38 > > > > > + attribute short shortAttr; > > > > > > > > I think you should test both short and unsigned short, in particular including boundary conditions. > > > > > > I am fixing this and will upload a new patch soo. > > > > OK. I don't see exactly where (for example) it's tested that setting the unsigned short attribute to 65535 and fetching it results in 65535. Are tests like that handled by the harness? > > Unfortunately, I think the current binding test is only for visual verification purpose. The generated files are not compiled and ran. Ah. Well, I suppose it will be tested with the DataView implementation shortly.
Jian Li
Comment 9 2010-11-18 18:16:22 PST
Note You need to log in before you can comment on or make changes to this bug.