Bug 78210

Summary: Add support for unsigned long[] to IDL bindings to JSC.
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, haraken, morrita, s.choi, vimff0, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 72010    
Attachments:
Description Flags
Patch.
haraken: review-, haraken: commit-queue-
Fixed patch.
haraken: review-, haraken: commit-queue-
Fixed patch.
haraken: review+, webkit.review.bot: commit-queue-
Fixed patch for WebCore/ChangeLog none

Description Kihong Kwon 2012-02-09 00:18:07 PST 
There is needs for support long[] parameter in the idl.
I'll add this to the code generator for JSC.
Comment 1 Kihong Kwon 2012-02-15 18:43:14 PST 
Created attachment 127292 [details]
Patch.
Comment 2 Kentaro Hara 2012-02-15 19:16:58 PST 
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.)
Comment 3 Kihong Kwon 2012-02-16 00:16:03 PST 
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.
Comment 4 Kihong Kwon 2012-02-16 00:41:08 PST 
Created attachment 127325 [details]
Fixed patch.
Comment 5 Kentaro Hara 2012-02-16 00:56:36 PST 
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.
Comment 6 Kihong Kwon 2012-02-16 03:12:30 PST 
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.
Comment 7 Kihong Kwon 2012-02-16 03:50:38 PST 
Created attachment 127352 [details]
Fixed patch.
Comment 8 Kentaro Hara 2012-02-16 03:55:48 PST 
Comment on attachment 127352 [details]
Fixed patch.

OK!
Comment 9 WebKit Review Bot 2012-02-16 04:17:58 PST 
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
Comment 10 Kentaro Hara 2012-02-16 04:21:40 PST 
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?
Comment 11 Kihong Kwon 2012-02-16 04:28:00 PST 
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.:)
Comment 12 Kihong Kwon 2012-02-16 04:30:02 PST 
Created attachment 127355 [details]
Fixed patch for WebCore/ChangeLog
Comment 13 WebKit Review Bot 2012-02-16 05:44:16 PST 
The commit-queue encountered the following flaky tests while processing attachment 127355 [details]:

perf/object-keys.html bug 63769 (author: ojan@chromium.org)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Review Bot 2012-02-16 05:46:52 PST 
Comment on attachment 127355 [details]
Fixed patch for WebCore/ChangeLog

Clearing flags on attachment: 127355

Committed r107926: <http://trac.webkit.org/changeset/107926>
Comment 15 WebKit Review Bot 2012-02-16 05:47:06 PST 
All reviewed patches have been landed.  Closing bug.