Bug 78210 – Add support for unsigned long[] to IDL bindings to JSC.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kihong Kwon]

There is needs for support long[] parameter in the idl.
I'll add this to the code generator for JSC.

[Kihong Kwon]

Created attachment 127292 [details]
Patch.

[Kentaro Hara]

Comment on attachment 127292 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127292&action=review

> Source/WebCore/ChangeLog:7
> +

Please describe what this patch is doing.

> Source/WebCore/bindings/js/JSDOMBinding.h:360
> +            result.append(static_cast<T>(indexedValue.toNumber(exec)));

I am afraid that this conversion rule is wrong with the Web IDL spec: http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long

It might be difficult to make such a template<class T> for all types due to complicated conversion rules in the spec. For now, maybe we can write jsUnsignedLongArrayToVector() for unsigned long[] only.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:281
> +    } elsif ($type eq "unsigned long[]") {

We want to avoid such hard-coding, but hard-coding might be a modest solution in this case. OK.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899
> +        return "jsNumberArrayToVector<unsigned long>(exec, $value)";

I cannot find any "jsNumberArrayToVector()" in your run-bindings-tests results...

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:45
> +#include "JSunsigned long[].h"

This is wrong.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:55
> +#include "unsigned long[].h"

This is wrong.

> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:72
> +    putDirect(exec->globalData(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontDelete | DontEnum);

This is not the change caused by your patch. Maybe you need to rebaseline run-bindings-tests results before making your patch, if the results are not correct on trunk. (Sometimes people forget to update run-bindings-tests results.)

[Kihong Kwon]

Comment on attachment 127292 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127292&action=review

Thank you for your comments.

>> Source/WebCore/ChangeLog:7
>> +
> 
> Please describe what this patch is doing.

OK.

>> Source/WebCore/bindings/js/JSDOMBinding.h:360
>> +            result.append(static_cast<T>(indexedValue.toNumber(exec))55);
> 
> I am afraid that this conversion rule is wrong with the Web IDL spec: http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long
> 
> It might be difficult to make such a template<class T> for all types due to complicated conversion rules in the spec. For now, maybe we can write jsUnsignedLongArrayToVector() for unsigned long[] only.

Sure - I will change that way.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:281
>> +    } elsif ($type eq "unsigned long[]") {
> 
> We want to avoid such hard-coding, but hard-coding might be a modest solution in this case. OK.

I think so.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899
>> +        return "jsNumberArrayToVector<unsigned long>(exec, $value)";
> 
> I cannot find any "jsNumberArrayToVector()" in your run-bindings-tests results...

I got it. It will be added.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:45
>> +#include "JSunsigned long[].h"
> 
> This is wrong.

Yes. It will be fixed.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:55
>> +#include "unsigned long[].h"
> 
> This is wrong.

Yes. It will be fixed.

>> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:72
>> +    putDirect(exec->globalData(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontDelete | DontEnum);
> 
> This is not the change caused by your patch. Maybe you need to rebaseline run-bindings-tests results before making your patch, if the results are not correct on trunk. (Sometimes people forget to update run-bindings-tests results.)

You are right. I will remove them.

[Kihong Kwon]

Created attachment 127325 [details]
Fixed patch.

[Kentaro Hara]

Comment on attachment 127325 [details]
Fixed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127325&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:350
> +        if (!object)

"if (exec->hadException())" might be better. If "!object" is true, then "exec->hadException()" is true.

> Source/WebCore/bindings/js/JSDOMBinding.h:354
> +        JSC::JSValue indexedValue;

Nit: You can move this declaration into the loop.

> Source/WebCore/bindings/js/JSDOMBinding.h:357
> +            if (indexedValue == JSC::JSValue(JSC::JSValue::JSUndefined) || !indexedValue.isNumber())

Maybe "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isUInt32())"?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899
> +        return "jsUnsignedLongArrayToVector<unsigned long>(exec, $value)";

This should be "jsUnsignedLongArrayToVector(exec, $value)", since the function is not a template.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2144
> +    Vector<unsigned long> unsignedLongArray(jsUnsignedLongArrayToVector<unsigned long>(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined)));

Ditto.

[Kihong Kwon]

Comment on attachment 127325 [details]
Fixed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127325&action=review

>> Source/WebCore/bindings/js/JSDOMBinding.h:350
>> +        if (!object)
> 
> "if (exec->hadException())" might be better. If "!object" is true, then "exec->hadException()" is true.

OK. You are right.

>> Source/WebCore/bindings/js/JSDOMBinding.h:354
>> +        JSC::JSValue indexedValue;
> 
> Nit: You can move this declaration into the loop.

OK.

>> Source/WebCore/bindings/js/JSDOMBinding.h:357
>> +            if (indexedValue == JSC::JSValue(JSC::JSValue::JSUndefined) || !indexedValue.isNumber())
> 
> Maybe "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isUInt32())"?

I will change this as follows: "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())".
It returns false when indexedValue goes beyond 2147483647.
Please refer to the comments from Gavin Barraclough (https://bugs.webkit.org/show_bug.cgi?id=77317)

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899
>> +        return "jsUnsignedLongArrayToVector<unsigned long>(exec, $value)";
> 
> This should be "jsUnsignedLongArrayToVector(exec, $value)", since the function is not a template.

Yes. It will be fixed.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2144
>> +    Vector<unsigned long> unsignedLongArray(jsUnsignedLongArrayToVector<unsigned long>(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined)));
> 
> Ditto.

Ditto.

[Kihong Kwon]

Created attachment 127352 [details]
Fixed patch.

[Kentaro Hara]

Comment on attachment 127352 [details]
Fixed patch.

OK!

[WebKit Review Bot]

Comment on attachment 127352 [details]
Fixed patch.

Rejecting attachment 127352 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11535167

[Kentaro Hara]

Comment on attachment 127352 [details]
Fixed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127352&action=review

> Source/WebCore/ChangeLog:6
> +        reviewed by nobody (oops!).

Maybe you manually edited this line. It should be exactly "Reviewed by NOBODY (OOPS!)".

Anyway, would you please rebase the patch (just in case), and upload it with correct ChangeLogs with "--no-review --no-obsolete" option?

[Kihong Kwon]

Comment on attachment 127352 [details]
Fixed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=127352&action=review

>> Source/WebCore/ChangeLog:6
>> +        reviewed by nobody (oops!).
> 
> Maybe you manually edited this line. It should be exactly "Reviewed by NOBODY (OOPS!)".
> 
> Anyway, would you please rebase the patch (just in case), and upload it with correct ChangeLogs with "--no-review --no-obsolete" option?

OK. Thank you for your review. Mr. Kentaro Hara.:)

[Kihong Kwon]

Created attachment 127355 [details]
Fixed patch for WebCore/ChangeLog

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 127355 [details]:

perf/object-keys.html bug 63769 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 127355 [details]
Fixed patch for WebCore/ChangeLog

Clearing flags on attachment: 127355

Committed r107926: <http://trac.webkit.org/changeset/107926>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.