Bug 71763

Created attachment 113986 [details] Patch

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 on attachment 113986 [details] Patch Attachment 113986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10330922

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?

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

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>&)'

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

Created attachment 114095 [details] Patch

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 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.

Created attachment 114138 [details] cache length

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

(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 on attachment 114138 [details] cache length Clearing flags on attachment: 114138 Committed r99632: <http://trac.webkit.org/changeset/99632>

All reviewed patches have been landed. Closing bug.