Bug 103642

Summary: Add support for generic types in arrays and sequences to the code generators
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, code.vineet, dglazkov, eric.carlson, feature-media-reviews, haraken, hta, japhet, jsbell, ojan, philn, tommyw, webkit.review.bot, xan.lopez, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98416, 103899    
Attachments:
Description Flags
Proposed patch
haraken: review-, eflews.bot: commit-queue-
Updated patch
eflews.bot: commit-queue-
Updated patch
haraken: review+
patch for landing none

Description Adam Bergkvist 2012-11-29 08:07:58 PST
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.
Comment 1 Adam Bergkvist 2012-11-29 08:28:26 PST
Created attachment 176740 [details]
Proposed patch
Comment 2 EFL EWS Bot 2012-11-29 08:57:59 PST
Comment on attachment 176740 [details]
Proposed patch

Attachment 176740 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15029840
Comment 3 Adam Barth 2012-11-29 10:05:27 PST
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 4 WebKit Review Bot 2012-11-29 12:20:45 PST
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 5 Kentaro Hara 2012-11-29 16:11:56 PST
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> ?
Comment 6 Adam Bergkvist 2012-11-30 09:04:44 PST
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 7 EFL EWS Bot 2012-11-30 09:35:24 PST
Comment on attachment 176978 [details]
Updated patch

Attachment 176978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15057408
Comment 8 Kentaro Hara 2012-12-02 16:07:03 PST
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.
Comment 9 Adam Bergkvist 2012-12-03 09:01:31 PST
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 10 Kentaro Hara 2012-12-03 15:57:05 PST
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() ?
Comment 11 Adam Bergkvist 2012-12-04 02:49:15 PST
(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.
Comment 12 Kentaro Hara 2012-12-04 02:51:06 PST
(In reply to comment #11)
> I renamed it to GetNativeVectorInnerType() since I thought that it might be more descriptive.

Sounds nicer!
Comment 13 Adam Bergkvist 2012-12-04 02:53:12 PST
Created attachment 177461 [details]
patch for landing

Patch for landing
Comment 14 WebKit Review Bot 2012-12-04 03:18:03 PST
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 15 WebKit Review Bot 2012-12-04 06:57:15 PST
Comment on attachment 177461 [details]
patch for landing

Clearing flags on attachment: 177461

Committed r136507: <http://trac.webkit.org/changeset/136507>
Comment 16 WebKit Review Bot 2012-12-04 06:57:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Joshua Bell 2012-12-04 12:27:25 PST
*** Bug 100537 has been marked as a duplicate of this bug. ***