[webkit-reviews] review denied: [Bug 49704] Make binding code generation scripts support 'short' type : [Attachment 74191] Proposed Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 49704: Make binding code generation scripts support 'short' type
https://bugs.webkit.org/show_bug.cgi?id=49704

Attachment 74191: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=74191&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.