Bug 49704 – Make binding code generation scripts support 'short' type

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

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.

[Jian Li]

Created attachment 74191 [details]
Proposed Patch

[Kenneth Russell]

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]

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]

Created attachment 74296 [details]
Proposed Patch

[Kenneth Russell]

(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]

Created attachment 74315 [details]
Proposed Patch

[Jian Li]

(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 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]

Committed as http://trac.webkit.org/changeset/72358.