Bug 71763 – Add support for float[] and double[] to IDL bindings code generator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Scott Graham]

Rather than writing more custom binding code, add support for array types to code generator.

[Scott Graham]

Created attachment 113986 [details]
Patch

[Scott Graham]

Not ready yet, just looking for some guidance (i.e. am I way off in the weeds here?)

Specific questions:

Currently just using Vector<float> and Vector<double> (and returning them by value from conversions). Is there an appropriate type to use here that's Ref? Or a terser conversion method assuming I wanted to do all of short/ushort/int/uint/long/ulong/float/double?

Is there a test that runs for bindings or do we just rely on the main build as a compile test? (I found Tools/Scripts/run-bindings-test but it's just a diff'r).

[WebKit Review Bot]

Comment on attachment 113986 [details]
Patch

Attachment 113986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10330922

[Adam Barth]

Comment on attachment 113986 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.cpp:328
> +    unsigned size = values.size();

unsigned -> size_t

> Source/WebCore/bindings/v8/V8Binding.cpp:329
> +    v8::Local<v8::Array> ret = v8::Array::New(size);

ret -> result (WebKit prefers complete words for variable names)

> Source/WebCore/bindings/v8/V8Binding.cpp:335
> +v8::Handle<v8::Value> v8NumberArray(const Vector<double>& values)

Can this just be templated?

[Adam Barth]

This approach looks pretty reasonable to me.  @haraken: Thoughts?

[Kentaro Hara]

Comment on attachment 113986 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add support for T[] to IDL bindings code generator

By the way, what are the use cases of T[]? I know there should be use cases, but I am curious about what custom constructors you are assuming as use cases.

> Source/WebCore/bindings/scripts/test/TestObj.idl:178
> +        attribute double[]                  doubleArrayAttr;

'floatArray' and 'doubleArray' might be a better name in order to avoid verbose names like 'floatArrayAttrAttrGetter' and 'doubleArrayAttrAttrGetter'.

> Source/WebCore/bindings/v8/V8Binding.cpp:647
> +        ret->append(indexedValue->ToNumber());

I am not sure, but does this really work? I could not find .append method in Vector. static_cast<float> is needed, isn't it?

> Source/WebCore/bindings/v8/V8Binding.h:353
> +    v8::Handle<v8::Value> v8NumberArray(const Vector<double>& values);

In WebKit a variable name is unnecessary in a method declaration. It should be 'v8::Handle<v8::Value> v8NumberArray(const Vector<float>&)'

[Kentaro Hara]

(In reply to comment #5)
> This approach looks pretty reasonable to me.  @haraken: Thoughts?

The approach looks good to me.

[Scott Graham]

Created attachment 114095 [details]
Patch

[Scott Graham]

Comment on attachment 113986 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Add support for T[] to IDL bindings code generator
> 
> By the way, what are the use cases of T[]? I know there should be use cases, but I am curious about what custom constructors you are assuming as use cases.

Just float[] and double[]. I thought maybe it would be "free" to have generic T[] but it seems not (or at least not obviously to me).

>> Source/WebCore/bindings/scripts/test/TestObj.idl:178
>> +        attribute double[]                  doubleArrayAttr;
> 
> 'floatArray' and 'doubleArray' might be a better name in order to avoid verbose names like 'floatArrayAttrAttrGetter' and 'doubleArrayAttrAttrGetter'.

Done.

>> Source/WebCore/bindings/v8/V8Binding.cpp:328
>> +    unsigned size = values.size();
> 
> unsigned -> size_t

Done.

>> Source/WebCore/bindings/v8/V8Binding.cpp:329
>> +    v8::Local<v8::Array> ret = v8::Array::New(size);
> 
> ret -> result (WebKit prefers complete words for variable names)

Done.

>> Source/WebCore/bindings/v8/V8Binding.cpp:335
>> +v8::Handle<v8::Value> v8NumberArray(const Vector<double>& values)
> 
> Can this just be templated?

Done. (Only OK for floating point-ish numbers because of the v8 constructors, but that's all I need anyway).

>> Source/WebCore/bindings/v8/V8Binding.cpp:647
>> +        ret->append(indexedValue->ToNumber());
> 
> I am not sure, but does this really work? I could not find .append method in Vector. static_cast<float> is needed, isn't it?

Fixed and templated. append is here I think: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Vector.h#L579

>> Source/WebCore/bindings/v8/V8Binding.h:353
>> +    v8::Handle<v8::Value> v8NumberArray(const Vector<double>& values);
> 
> In WebKit a variable name is unnecessary in a method declaration. It should be 'v8::Handle<v8::Value> v8NumberArray(const Vector<float>&)'

Noted, thank you (but as it's now a template I had to keep a name).

[Adam Barth]

Comment on attachment 114095 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:399
> +        for (size_t i = 0; i < v8Array->Length(); ++i) {

Should we store the Length in a temporary?  I'm not sure how expensive this call is.

[Scott Graham]

Created attachment 114138 [details]
cache length

[WebKit Review Bot]

Comment on attachment 114138 [details]
cache length

Rejecting attachment 114138 [details] from commit-queue.

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

Last 500 characters of output:
ab3c-d52691b4dbfc ...
Currently at 99611 = 22fdee69221aa1af63319d551b4cfb947ca7f6e8
r99612 = 5ccb211e65d931949fac632697f43446b7f10096
r99613 = 1bf25204738bc0ef0dc9adfa86fe5384fba24093
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 157.

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

[Scott Graham]

(In reply to comment #12)
> (From update of attachment 114138 [details])
> Rejecting attachment 114138 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> ab3c-d52691b4dbfc ...
> Currently at 99611 = 22fdee69221aa1af63319d551b4cfb947ca7f6e8
> r99612 = 5ccb211e65d931949fac632697f43446b7f10096
> r99613 = 1bf25204738bc0ef0dc9adfa86fe5384fba24093
> Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
> RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295
> 
> Died at Tools/Scripts/update-webkit line 157.
> 
> Full output: http://queues.webkit.org/results/10376186

Wharrgarbl? Try again maybe, since it looks like a timeout?

[WebKit Review Bot]

Comment on attachment 114138 [details]
cache length

Clearing flags on attachment: 114138

Committed r99632: <http://trac.webkit.org/changeset/99632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.