Bug 98416

Created attachment 167115 [details] Proposed patch The custom bindings could share some helper functions. If desirable, what's the best way to do that?

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

Comment on attachment 167115 [details] Proposed patch Why do we need to use a custom constructor? Can we improve the code generator so we don't need to add more custom code?

Yeah, I am sorry that custom code are coming back, especially since I fixed a few issues with the code generators and threw out all the old custom code earlier this year, but I can's see how the code generator could easily be modified for this use case. The constructor now takes an array that can contain one to many objects of three different kinds. I tried to "talk sense" to the standardization committee but they want it like this. One other possible approach that would avoid custom code is to heavily modify the Dictionary class to be able to return the real objects, but I don't know if that would be much better.

I think we already support overloaded constructors. I don't think it would be that hard to support overloaded constructors with []. The IDL declaration would look something like: interface [ Conditional=MEDIA_STREAM, EventTarget, Constructor(in MediaStream[] streams), Constructor(in MediaStreamTrackList[] trackLists), Constructor(in MediaStreamTrack[] tracks), CallWith=ScriptExecutionContext, ConstructorRaisesException ] MediaStream { ...

(In reply to comment #4) > Yeah, I am sorry that custom code are coming back, especially since I fixed a few issues with the code generators and threw out all the old custom code earlier this year, but I can's see how the code generator could easily be modified for this use case. Yes, it's a tricky translation with a union type. I know that at least the WebSocket constructor has a union type argument to the constructor and it uses a custom binding too. > The constructor now takes an array that can contain one to many objects of three different kinds. I tried to "talk sense" to the standardization committee but they want it like this. The discussion [1] kind of died out. I don't know if that means that people are happy with the current behavior or if they don't care. I'm not sure that the current behavior will be the final, but it seems that people like some kind of flexible argument list (some combination of MediaStream, MediaStreamTrackList and MediaStreamTrack objets). [1] http://lists.w3.org/Archives/Public/public-media-capture/2012Sep/0027.html Just thinking out loud around this below. The current definition in WebIDL is: typedef (MediaStream? or MediaStreamTrackList or MediaStreamTrack) TrackContainer; [Constructor (TrackContainer[]? trackContainers)] Could we introduce a type in WebKit that represents the typedef'ed TrackContainer? The corresponding static create function would look something like: static PassRefPtr<MediaStream> create(ScriptExecutionContext*, TrackContainerVector); The functionality to extract the tracks from the track containers would then be in MediaStream.cpp instead of the custom binding. That could make it easier to move over to an automatically generated binding since it's pure argument translation. An alternative to the create function above would be with one list for each type of track container: static PassRefPtr<MediaStream> create(ScriptExecutionContext*, MediaStreamVector, MediaStreamTrackListVector, MediaStreamTrackVector);

(In reply to comment #5) > I think we already support overloaded constructors. I don't think it would be that hard to support overloaded constructors with []. The IDL declaration would look something like: > > interface [ > Conditional=MEDIA_STREAM, > EventTarget, > Constructor(in MediaStream[] streams), > Constructor(in MediaStreamTrackList[] trackLists), > Constructor(in MediaStreamTrack[] tracks), > CallWith=ScriptExecutionContext, > ConstructorRaisesException > ] MediaStream { > ... The behavior would be different since you couldn't use two different types in one constructor call. For example: var audioStream = new MediaStream([localStream.audioTracks, remoteStream.audioTracks[0]]);

I see. IMHO, we should push back on the standards body to make the API more like other web platform APIs.

We can take it another round the the Media Capture TF. But if the outcome does use a WebIDL union, wouldn't it be OK to let it start off as a custom binding and then modify the bindings generator when some other object needs similar functionality and we know how this type of binding should look.

Created attachment 174660 [details] Updated patch The spec has been updated with a simplified version of the MediaStream constructor. [Constructor (), Constructor (MediaStream? stream), Constructor (MediaStreamTrack[] tracks)] It's still not possible to do this without a custom binding, but the bindings are simpler.

Comment on attachment 174660 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=174660&action=review > Source/WebCore/Modules/mediastream/MediaStream.idl:29 > + ConstructorParameters=0, Have you tried to use Constructor(in sequence<MediaStreamTrack> tracks) Constructor(in [Optional] MediaStream stream) instead of custom bindings?

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

Comment on attachment 174660 [details] Updated patch We should be able to do this without custom bindings.

(In reply to comment #11) > (From update of attachment 174660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174660&action=review > > > Source/WebCore/Modules/mediastream/MediaStream.idl:29 > > + ConstructorParameters=0, > > Have you tried to use > Constructor(in sequence<MediaStreamTrack> tracks) > Constructor(in [Optional] MediaStream stream) > instead of custom bindings? Thanks for the pointer, but it won't work afaik without modifying the bindings generator. I've only seen [] and sequence<> used with basic types (mainly long) and DOMString for arguments. Constructor(in sequence<MediaStreamTrack> tracks) will look for: ::create(Vector<MediaStreamTrack>&) while we want something like: ::create(Vector<RefPtr<MediaStreamTrack> >&)

> Thanks for the pointer, but it won't work afaik without modifying the bindings generator. Then we'll need to improve the bindings generator. We don't want to add custom code to implement this behavior. The bindings generator should handle it for us.

(In reply to comment #15) > Then we'll need to improve the bindings generator. We don't want to add custom code to implement this behavior. The bindings generator should handle it for us. OK. Should that be done in a separate bug/patch?

> OK. Should that be done in a separate bug/patch? Yes, that's probably easiest. You can add a test using run-bindings-tests if you like.

Created attachment 177482 [details] Updated patch Updated patch with generated bindings for V8 (custom for JSC). Tommy is working on support for overloaded constructors in the JSC bindings generator (http://webkit.org/b/103226) and once he's done, we can remove the JSC custom binding.

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

Created attachment 177672 [details] Updated patch Fixed GTK build error.

Comment on attachment 177672 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=177672&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:41 > + for (size_t i = 0; i < sourceVector.size(); ++i) Please use { } when the body has more than one (textual) line. > Source/WebCore/bindings/js/JSMediaStreamCustom.cpp:46 > +EncodedJSValue JSC_HOST_CALL JSMediaStreamConstructor::constructJSMediaStream(ExecState* exec) I thought we were going to avoid custom code here by improving CodeGeneratorJS...

(In reply to comment #21) > (From update of attachment 177672 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177672&action=review > > > Source/WebCore/bindings/js/JSMediaStreamCustom.cpp:46 > > +EncodedJSValue JSC_HOST_CALL JSMediaStreamConstructor::constructJSMediaStream(ExecState* exec) > > I thought we were going to avoid custom code here by improving CodeGeneratorJS... Yes that's the goal. We had two issues with the code generators. The first, general support for arrays and sequences has been fixed in b103642. The second, support for overloaded constructors is only fixed for V8 at the moment, but Tommy is working on support in CodeGeneratorJS (b103226). What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed?

> What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed? I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time). My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator.

(In reply to comment #23) > > What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed? > > I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time). My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator. I feel guilty for not having finished the code generator feature yet but I got drafted to do some WebAudio and SpeechRecognition coding while at the same time trying to get some WebRTC code finished for M25 :/ Not an excuse but maybe an explanation. However whatever happens it's my P0 task for next week.

(In reply to comment #23) > I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time). My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator. Ok I understand. Could you disregard from the custom binding and continue to review this patch anyhow? It would save us one iteration since the non custom patch is almost identical to this patch without the custom binding.

Comment on attachment 177672 [details] Updated patch The rest of this patch looks fine. :)

(In reply to comment #26) > (From update of attachment 177672 [details]) > The rest of this patch looks fine. :) Great. :) Then I'll update the patch when bug 103226 is resolved. If the change only consists of removing the custom binding and "JSCustomConstructor" from the idl I'll land it. Otherwise, if there are any major changes I'll mark it for review again. Sounds ok?

I'll fix the first comment from #21 as well.

Sgtm

Created attachment 180351 [details] patch for landing Patch for landing without custom binding for JSC (thanks to bug 103226)

Comment on attachment 180351 [details] patch for landing Clearing flags on attachment: 180351 Committed r138354: <http://trac.webkit.org/changeset/138354>

All reviewed patches have been landed. Closing bug.