[webkit-reviews] review denied: [Bug 103642] Add support for generic types in arrays and sequences to the code generators : [Attachment 176740] Proposed patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 103642: Add support for generic types in arrays and sequences to the code
generators
https://bugs.webkit.org/show_bug.cgi?id=103642

Attachment 176740: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=176740&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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> ?