Bug 103642 – Add support for generic types in arrays and sequences to the code generators

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Bergkvist]

We need to generate bindings for MediaStreamTrack[] in webkit.org/b/98416. Currently, there's only limited support for sequences and arrays of primitive types and strings.

[Adam Bergkvist]

Created attachment 176740 [details]
Proposed patch

[EFL EWS Bot]

Comment on attachment 176740 [details]
Proposed patch

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

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 176740 [details]
Proposed patch

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

[Kentaro Hara]

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

[Adam Bergkvist]

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)

[EFL EWS Bot]

Comment on attachment 176978 [details]
Updated patch

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

[Kentaro Hara]

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.

[Adam Bergkvist]

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.

[Kentaro Hara]

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() ?

[Adam Bergkvist]

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

[Kentaro Hara]

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

Sounds nicer!

[Adam Bergkvist]

Created attachment 177461 [details]
patch for landing

Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 177461 [details]
patch for landing

Clearing flags on attachment: 177461

Committed r136507: <http://trac.webkit.org/changeset/136507>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

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