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

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	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 176978: Updated patch
https://bugs.webkit.org/attachment.cgi?id=176978&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
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)