Bug 71763 - Add support for float[] and double[] to IDL bindings code generator
Summary: Add support for float[] and double[] to IDL bindings code generator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Graham
URL:
Keywords:
Depends on:
Blocks: 71753
  Show dependency treegraph
 
Reported: 2011-11-07 21:32 PST by Scott Graham
Modified: 2011-11-18 18:24 PST (History)
7 users (show)

See Also:


Attachments
Patch (11.09 KB, patch)
2011-11-07 21:41 PST, Scott Graham
no flags Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2011-11-08 09:39 PST, Scott Graham
no flags Details | Formatted Diff | Diff
cache length (9.58 KB, patch)
2011-11-08 12:49 PST, Scott Graham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Graham 2011-11-07 21:32:50 PST
Rather than writing more custom binding code, add support for array types to code generator.
Comment 1 Scott Graham 2011-11-07 21:41:57 PST
Created attachment 113986 [details]
Patch
Comment 2 Scott Graham 2011-11-07 21:50:52 PST
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).
Comment 3 WebKit Review Bot 2011-11-07 21:55:28 PST
Comment on attachment 113986 [details]
Patch

Attachment 113986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10330922
Comment 4 Adam Barth 2011-11-08 00:21:11 PST
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?
Comment 5 Adam Barth 2011-11-08 00:21:43 PST
This approach looks pretty reasonable to me.  @haraken: Thoughts?
Comment 6 Kentaro Hara 2011-11-08 02:25:44 PST
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>&)'
Comment 7 Kentaro Hara 2011-11-08 02:26:06 PST
(In reply to comment #5)
> This approach looks pretty reasonable to me.  @haraken: Thoughts?

The approach looks good to me.
Comment 8 Scott Graham 2011-11-08 09:39:27 PST
Created attachment 114095 [details]
Patch
Comment 9 Scott Graham 2011-11-08 09:43:28 PST
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).
Comment 10 Adam Barth 2011-11-08 12:40:06 PST
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.
Comment 11 Scott Graham 2011-11-08 12:49:08 PST
Created attachment 114138 [details]
cache length
Comment 12 WebKit Review Bot 2011-11-08 14:44:51 PST
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
Comment 13 Scott Graham 2011-11-08 15:25:30 PST
(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?
Comment 14 WebKit Review Bot 2011-11-08 16:43:22 PST
Comment on attachment 114138 [details]
cache length

Clearing flags on attachment: 114138

Committed r99632: <http://trac.webkit.org/changeset/99632>
Comment 15 WebKit Review Bot 2011-11-08 16:43:27 PST
All reviewed patches have been landed.  Closing bug.