Created attachment 176740 [details] Proposed patch

Comment on attachment 176740 [details] Proposed patch Attachment 176740 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15029840

Comment on attachment 176740 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=176740&action=review This looks good to me. We might want to have haraken take a look as well. It looks like you have some compile errors on gtk and efl. > Source/WebCore/bindings/js/JSDOMBinding.h:433 > + return result; Does this mean we copy the array and churn the refcount, or are we smart enough to move it? We might want to use an out parameter to avoid the extra memcpy.

Comment on attachment 176740 [details] Proposed patch Attachment 176740 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15025842

Comment on attachment 176740 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=176740&action=review The approach looks reasonable. r-ing due to duplicated code in toHostObjectArray(). I hope you can use toNativeArray(). I guess you need to make some change to CodeGenerator{GObject,CPP,ObjC}.pm to fix build errors. > Source/WebCore/bindings/js/JSDOMBinding.h:416 > + template <class T, class JST> > + Vector<RefPtr<T> > toHostObjectArray(JSC::ExecState* exec, JSC::JSValue value, T* (*toT)(JSC::JSValue value)) Can you avoid adding this method by using NativeValueTraits<T> ? See toNativeArray() and NativeValueTraits<T> in JSDOMBinding.h. toNativeArray() is doing the same thing as this method. > Source/WebCore/bindings/js/JSDOMBinding.h:421 > + return result; Nit: 'return Vector<T>()' would be clearer. > Source/WebCore/bindings/js/JSDOMBinding.h:430 > + return result; Maybe this should be 'return Vector<T>()' ? >> Source/WebCore/bindings/js/JSDOMBinding.h:433 >> + return result; > > Does this mean we copy the array and churn the refcount, or are we smart enough to move it? We might want to use an out parameter to avoid the extra memcpy. Good point. We're doing the same thing in other methods of V8Binding.h and JSDOMBinding.h. Let's fix it in a follow-up patch. > Source/WebCore/bindings/v8/V8Binding.h:211 > + template <class T, class V8T> > + Vector<RefPtr<T> > toHostObjectArray(v8::Handle<v8::Value> value) Ditto. Can you avoid adding this method by using NativeValueTraits<T> ?

Created attachment 176978 [details] Updated patch Thank you for reviewing. (In reply to comment #5) > (From update of attachment 176740 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176740&action=review > > The approach looks reasonable. r-ing due to duplicated code in toHostObjectArray(). I hope you can use toNativeArray(). > > I guess you need to make some change to CodeGenerator{GObject,CPP,ObjC}.pm to fix build errors. The build errors are due to a "sequence<String>" in an IDL file (the change to "sequence<DOMString>" got lost in a last minute rebase). This is fixed now. > > > Source/WebCore/bindings/js/JSDOMBinding.h:416 > > + template <class T, class JST> > > + Vector<RefPtr<T> > toHostObjectArray(JSC::ExecState* exec, JSC::JSValue value, T* (*toT)(JSC::JSValue value)) > > Can you avoid adding this method by using NativeValueTraits<T> ? See toNativeArray() and NativeValueTraits<T> in JSDOMBinding.h. toNativeArray() is doing the same thing as this method. > We could solve this by adding a "struct NativeValueTraits<RefPtr<[Type]> >" for every host object type that we need to put in an array or sequence, but then we would have to update the code generators every time a new type is needed. The alternative would be to make a NativeValueTraits solution for the general RefPtr<T> type, but I think it will be hard to add that kind of support to the existing toNativeArray() which only deals with primitive types and strings. For example, besides the type T, the binding type (JST and V8T in this patch) is needed to deal with host objects. I've kept toHostObjectArray() for now. > > Source/WebCore/bindings/js/JSDOMBinding.h:421 > > + return result; > > Nit: 'return Vector<T>()' would be clearer. Fixed (for V8Binding.h as well) > > > Source/WebCore/bindings/js/JSDOMBinding.h:430 > > + return result; > > Maybe this should be 'return Vector<T>()' ? You're right. Fixed (for V8Binding.h as well)

Comment on attachment 176978 [details] Updated patch Attachment 176978 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15057408

Comment on attachment 176978 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=176978&action=review Thanks for updating the patch. Almost looks OK. > Source/WebCore/bindings/js/JSDOMBinding.h:416 > + Vector<RefPtr<T> > toHostObjectArray(JSC::ExecState* exec, JSC::JSValue value, T* (*toT)(JSC::JSValue value)) toRefPtrNativeArray() might be a better name, as this is just another version of toNativeArray(). > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3070 > + return "Vector<" . GetNativeInnerVectorType($arrayOrSequenceType) . ">" if $arrayOrSequenceType; Can't you use GetNativeType() ? > Source/WebCore/bindings/v8/V8Binding.h:211 > + Vector<RefPtr<T> > toHostObjectArray(v8::Handle<v8::Value> value) toRefPtrNativeArray() might be a better name.

Created attachment 177263 [details] Updated patch (In reply to comment #8) > (From update of attachment 176978 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176978&action=review > > Thanks for updating the patch. Almost looks OK. > > > Source/WebCore/bindings/js/JSDOMBinding.h:416 > > + Vector<RefPtr<T> > toHostObjectArray(JSC::ExecState* exec, JSC::JSValue value, T* (*toT)(JSC::JSValue value)) > > toRefPtrNativeArray() might be a better name, as this is just another version of toNativeArray(). Fixed. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3070 > > + return "Vector<" . GetNativeInnerVectorType($arrayOrSequenceType) . ">" if $arrayOrSequenceType; > > Can't you use GetNativeType() ? That was my initial approach as well (since I use it for V8), but the JSC-version of GetNativeType doesn't behave the same as the V8 counterpart for some types. For example, DOMString is translated to "const String&" instead of "String". > > Source/WebCore/bindings/v8/V8Binding.h:211 > > + Vector<RefPtr<T> > toHostObjectArray(v8::Handle<v8::Value> value) > > toRefPtrNativeArray() might be a better name. Fixed. The build errors on EFL were due to a mismatch between the IDL type and the implementation type in the Vibration API. I've added a workaround and filed a separate bug to resolve that.

Comment on attachment 177263 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=177263&action=review Looks good to me. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3076 > +sub GetNativeInnerVectorType Shall we rename it to GetRefPtrNativeType() ?

(In reply to comment #10) > (From update of attachment 177263 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177263&action=review > > Looks good to me. > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3076 > > +sub GetNativeInnerVectorType > > Shall we rename it to GetRefPtrNativeType() ? This sub routine is used to get the Vector inner type for primitive types and String as well (when used together with the old toNativeArray) so it shouldn't have "RefPtr" in the name. I renamed it to GetNativeVectorInnerType() since I thought that it might be more descriptive. Thank you for reviewing.

(In reply to comment #11) > I renamed it to GetNativeVectorInnerType() since I thought that it might be more descriptive. Sounds nicer!

Created attachment 177461 [details] patch for landing Patch for landing

Comment on attachment 177461 [details] patch for landing Rejecting attachment 177461 [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: ripts/update-webkit line 152. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource From http://git.chromium.org/external/Webkit 9119335..f15c604 HEAD -> origin/HEAD error: Ref refs/remotes/origin/master is at f15c604c4ceb9e673f01c999e7b638e0dead7e5c but expected 91193351fe3f098d5e3121072b1a9b057086e670 ! 9119335..f15c604 master -> origin/master (unable to update local ref) Died at Tools/Scripts/update-webkit line 152. Full output: http://queues.webkit.org/results/15126586

Comment on attachment 177461 [details] patch for landing Clearing flags on attachment: 177461 Committed r136507: <http://trac.webkit.org/changeset/136507>

All reviewed patches have been landed. Closing bug.

*** Bug 100537 has been marked as a duplicate of this bug. ***