Bug 65101 – MediaStream API: Implement PeerConnection and SignalingCallback

• Component: DOM
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Add a initial version of PeerConnection and SignalingCallback.

[Tommy Widenflycht]

Created attachment 101860 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101860 [details]
Patch

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

New failing tests:
fast/dom/prototype-inheritance.html

[Tommy Widenflycht]

Created attachment 102005 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 102005 [details]
Patch

Once the build bots succeed, LGTM with a few things to fix:

View in context: https://bugs.webkit.org/attachment.cgi?id=102005&action=review

> Source/WebCore/dom/PeerConnection.cpp:54
> +        (m_object.get()->*m_callback)(m_data);

Is .get required here?

> Source/WebCore/dom/PeerConnection.cpp:80
> +        (m_object.get()->*m_callback)(m_arg1, m_arg2);

Same as before.

> Source/WebCore/dom/PeerConnection.cpp:347
> +    dispatchEvent(MessageEvent::create(adoptPtr<MessagePortArray>(0), d));

I think the first param should be PassOwnPtr<MessagePortArray>() in this case.

> Source/WebCore/dom/PeerConnection.h:31
> +#include "MediaStream.h"

Are you sure this is required in the header? If you don't instantiate any PassRefPtr in this file you shouldn't need it here.

> Source/WebCore/dom/PeerConnection.h:33
> +#include "ScriptExecutionContext.h"

Same with this. It's already forward-declared.

> Source/WebCore/dom/PeerConnection.h:50
> +        NEW = 0,

This is not a patch problem, but I'm wondering why the spec starts the PeerConnection status values with 0 (NEW) and the MediaStream status values with 1 (LIVE). Perhaps a minor inconsistency?

> Source/WebCore/page/MediaStreamFrameController.cpp:435
> +    ASSERT(!streamLabel.isNull());

This will pass with non-null empty strings. Maybe you meant isEmpty?

> Source/WebCore/page/MediaStreamFrameController.cpp:447
> +    ASSERT(!streamLabel.isNull());

Same as before.

> Source/WebCore/page/MediaStreamFrameController.h:146
> +        virtual ~PeerConnectionClient() { }

This is ok, but you won't be able to do PeerConnection-specific unregistration operations if you leave the destructor like this. The problem is, as in MediaStreams, that isPeerConnection will always return false from GenericClient's destructor. Consequently you won't be able to tell if your client is a PeerConnection client from the unregister(GenericClient*) method.

If you want to perform specific unregistering operations in the controller for any reason (eg. MediaStreams do so to stop any live streams) you should mimic MediaStreamClient or GenericClient and invoke an specific unregister(PeerConnectionClient*) method. Keep in mind that if you inherit from GenericClient then you should not remove the client from the map as GenericClient's destructor will already do so.

> Source/WebCore/page/MediaStreamFrameController.h:215
> +    void onAddStream(int peerConnectionId, const AtomicString& streamLabel, PassRefPtr<MediaStreamTrackList> tracks);

'tracks' is unnecessary here as MediaStreamTrackList is supposed to provide enough information about the argument.

[Tommy Widenflycht]

Created attachment 102012 [details]
Patch

[Adam Barth]

Comment on attachment 102012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review

This patch contains lots of small mistakes w.r.t. RefPtr, style, and general project organization.  I haven't noted every instance of every issue, but hopefully you can generalize from the comments below.  It's a bit hard to understand what the patch actually does because these issues are somewhat distracting.  Perhaps we should break this patch up into smaller pieces so each piece can receive a high-quality review.

Finally, this patch contains no tests.  I presume that's because we haven't landed enough of this feature to test it.  What's the plan for testing?  (Please feel free to refer me to another bug or to a wiki page if this question has already been asked and answered).

Thanks!

> Source/WebCore/CMakeLists.txt:2009
> +        dom/PeerConnection.cpp

PeerConnection shouldn't be in the "DOM" folder.  DOM is for DOM Core.  In all likelihood, we should have a new folder in WebCore for p2p code.

> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
> +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)

Why do we need custom bindings for this object?  Is there some feature missing from the code generator?

> Source/WebCore/dom/PeerConnection.cpp:42
> +template <typename T, typename D>

Please use more descriptive template variables.

> Source/WebCore/dom/PeerConnection.cpp:43
> +class SimpleDispatchTask : public ScriptExecutionContext::Task {

This object seems more general than just PeerConnection.  Should it move to a more general location where it can be used by other code?

> Source/WebCore/dom/PeerConnection.cpp:69
> +template <typename T, typename A1, typename A2>
> +class DispatchTask : public ScriptExecutionContext::Task {

Dittto.

> Source/WebCore/dom/PeerConnection.cpp:99
> +    // TODO: Verify configuration

TODO => FIXME.  Please make comments be complete sentences.

> Source/WebCore/dom/PeerConnection.cpp:102
> +    pc->init();

Why is this init function separate from construction?

> Source/WebCore/dom/PeerConnection.cpp:103
> +    return pc;

pc.release().

Also, please use complete words in variable names.

> Source/WebCore/dom/PeerConnection.cpp:137
> +PassRefPtr<MediaStreamList> PeerConnection::localStreams()
> +{
> +    return m_localStreamList;
> +}
> +
> +PassRefPtr<MediaStreamList> PeerConnection::remoteStreams()
> +{
> +    return m_remoteStreamList;
> +}

Do these functions transfer ownership?  If not, they should return raw pointers.  If you haven't read http://www.webkit.org/coding/RefPtr.html you might find it informative.

> Source/WebCore/dom/PeerConnection.cpp:139
> +// Sends the message down to the user agent implementation.

Please remove this comment.  Perhaps we should rename the function to say what this comment says?

> Source/WebCore/dom/PeerConnection.cpp:151
> +void PeerConnection::send(const AtomicString& text, ExceptionCode& ec)

Why AtomicString?  I would have expected String.

> Source/WebCore/dom/PeerConnection.cpp:177
> +    RefPtr<MediaStream> s = stream;

Typically we'd call "stream" prpStream and then rename "s" to stream.

> Source/WebCore/dom/PeerConnection.cpp:283
> +    if (!scriptExecutionContext())

Typically we'd store the scriptExecutionContext in a local variable because it's non-trivial to retrieve it.

> Source/WebCore/dom/PeerConnection.cpp:288
> +    scriptExecutionContext()->postTask(DispatchTask<PeerConnection, int, SerializedScriptValue>::create(this, &PeerConnection::dispatchMessageEvent, 0, data));

data.release()

> Source/WebCore/dom/PeerConnection.cpp:307
> +    RefPtr<MediaStream> s = stream;
> +    scriptExecutionContext()->postTask(DispatchTask<PeerConnection, AtomicString, MediaStream>::create(this, &PeerConnection::dispatchStreamEvent, eventNames().addstreamEvent, s));

What's the point of converting stream to a RefPtr?  We should just pass the PassReftPtr on to postTask.

> Source/WebCore/dom/PeerConnection.h:46
> +class PeerConnection : public RefCounted<PeerConnection>,
> +    public EventTarget,
> +    public MediaStreamFrameController::PeerConnectionClient {

One line pls.

> Source/WebCore/dom/PeerConnection.h:48
> +    // Must match the constants in the .idl file.

Complete sentences pls.

> Source/WebCore/dom/PeerConnection.h:60
> +    PassRefPtr<MediaStreamList> localStreams();
> +    PassRefPtr<MediaStreamList> remoteStreams();

These don't transfer ownership and so should return raw pointers.

> Source/WebCore/dom/PeerConnection.idl:31
> +        ConstructorParameters=2,

Can we really not provide the type signature for the constructor?  That seems like something we should fix in the code generator.

> Source/WebCore/dom/PeerConnection.idl:33
> +        EventTarget

Shouldn't we just have this interface inherit from EventTarget instead of using this attribute?

> Source/WebCore/dom/PeerConnection.idl:46
> +        [StrictTypeChecking] void addStream(in MediaStream stream)

I've never seen this StrictTypeChecking before.  What does that mean?

> Source/WebCore/dom/PeerConnection.idl:69
> +        // EventTarget interface
> +        void addEventListener(in DOMString type,
> +                              in EventListener listener,
> +                              in boolean useCapture);
> +        void removeEventListener(in DOMString type,
> +                                 in EventListener listener,
> +                                 in boolean useCapture);

The useCapture argument should be marked [Optional]

> Source/WebCore/dom/SignalingCallback.idl:25
> +module core {

Why the "Core" module?  This API isn't part of DOM Core.

> Source/WebCore/page/MediaStreamClient.h:77
> +    // Signaling message from peer connection.
> +    virtual void processSignalingMessage(int peerConnectionId, const String& message) = 0;
> +
> +    // Message from peer connection.
> +    virtual void message(int peerConnectionId, const String& message) = 0;
> +
> +    // Add a stream to a peer connection.
> +    virtual void addStream(int peerConnectionId, const String& streamLabel) = 0;
> +
> +    // Remove a stream from a peer connection.
> +    virtual void removeStream(int peerConnectionId, const String& streamLabel) = 0;
> +
> +    // A PeerConnection object has been created.
> +    virtual void newPeerConnection(int peerConnectionId, const String& configuration) = 0;
> +
> +    // Peer connection is being closed.
> +    virtual void closePeerConnection(int peerConnectionId) = 0;
> +
> +    // Start negotiation.
> +    virtual void startNegotiation(int peerConnectionId) = 0;

Why are these calls going through the client?  I would have expected them to go through the platform.

> Source/WebCore/page/MediaStreamController.cpp:37
> +// TODO: This is used for holding list of PeerConnection as well,

TODO -> FIXME

> Source/WebCore/page/MediaStreamController.cpp:96
> +    // TODO: Add for peer connection

TODO -> FIXME.  Please use complete sentences for comments.

> Source/WebCore/page/MediaStreamController.cpp:172
> +    int pagePeerConnectionId(-1);

Please use the assignment form of the constructor.

> Source/WebCore/page/MediaStreamController.cpp:193
> +    int pagePeerConnectionId = frameToPagePeerConnectionId(frameController, framePeerConnectionId);
> +    if (pagePeerConnectionId != -1)

Generally WebKit frowns upon in-band signaling.  Is there a better design here?  For example, perhaps frameToPagePeerConnectionId should return a bool and have an out parameter for the ID.

> Source/WebCore/page/MediaStreamFrameController.cpp:372
> +    return peerConnection;

peerConnection.release()

> Source/WebCore/page/MediaStreamFrameController.cpp:442
> +    peerConnection->streamAdded(stream);

stream.release()

> Source/WebCore/page/MediaStreamFrameController.h:173
> +    // Creates a new PeerConnection object.

These comments should be removed.  Probably all the comments in this file are useless and should be removed, but I haven't checked them all.

[Leandro Graciá Gil]

A few meta-comments for Adam about some reasons behind the actual code. Please Tommy feel free to correct or complete me.

(In reply to comment #6)
> (From update of attachment 102012 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review
> > Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
> > +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
> 
> Why do we need custom bindings for this object?  Is there some feature missing from the code generator?

I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.

> > Source/WebCore/dom/PeerConnection.cpp:137
> > +PassRefPtr<MediaStreamList> PeerConnection::localStreams()
> > +{
> > +    return m_localStreamList;
> > +}
> > +
> > +PassRefPtr<MediaStreamList> PeerConnection::remoteStreams()
> > +{
> > +    return m_remoteStreamList;
> > +}
> 
> Do these functions transfer ownership?  If not, they should return raw pointers.  If you haven't read http://www.webkit.org/coding/RefPtr.html you might find it informative.
> 

According to the spec, these lists need to be accessible by Javascript. That's why they are reference-counted objects and raw pointers are not used.

Here's the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#peerconnection

> > Source/WebCore/dom/PeerConnection.cpp:283
> > +    if (!scriptExecutionContext())
> 
> Typically we'd store the scriptExecutionContext in a local variable because it's non-trivial to retrieve it.

Doing this here would lead to errors since you could detach the frame associated to the context but keep a reference to the PeerConnection object. This method avoids the tricky cases I mentioned before and ensures that the proper execution context is returned if available.

 
> > Source/WebCore/dom/PeerConnection.h:60
> > +    PassRefPtr<MediaStreamList> localStreams();
> > +    PassRefPtr<MediaStreamList> remoteStreams();
> 
> These don't transfer ownership and so should return raw pointers.

Need to be accessible by Javascript as before. 


> > Source/WebCore/dom/PeerConnection.idl:33
> > +        EventTarget
> 
> Shouldn't we just have this interface inherit from EventTarget instead of using this attribute?

Almost all existing DOM code does it like this instead of inheriting in the idl.


> > Source/WebCore/page/MediaStreamClient.h:77
> > +    // Signaling message from peer connection.
> > +    virtual void processSignalingMessage(int peerConnectionId, const String& message) = 0;
> > +
> > +    // Message from peer connection.
> > +    virtual void message(int peerConnectionId, const String& message) = 0;
> > +
> > +    // Add a stream to a peer connection.
> > +    virtual void addStream(int peerConnectionId, const String& streamLabel) = 0;
> > +
> > +    // Remove a stream from a peer connection.
> > +    virtual void removeStream(int peerConnectionId, const String& streamLabel) = 0;
> > +
> > +    // A PeerConnection object has been created.
> > +    virtual void newPeerConnection(int peerConnectionId, const String& configuration) = 0;
> > +
> > +    // Peer connection is being closed.
> > +    virtual void closePeerConnection(int peerConnectionId) = 0;
> > +
> > +    // Start negotiation.
> > +    virtual void startNegotiation(int peerConnectionId) = 0;
> 
> Why are these calls going through the client?  I would have expected them to go through the platform.

It goes through a WebKit page client just as the other OWP APIs (Geolocation, Device Orientation, Speech Input and such).

[Adam Barth]

(In reply to comment #7)
> A few meta-comments for Adam about some reasons behind the actual code. Please Tommy feel free to correct or complete me.
> 
> (In reply to comment #6)
> > (From update of attachment 102012 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review
> > > Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
> > > +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
> > 
> > Why do we need custom bindings for this object?  Is there some feature missing from the code generator?
> 
> I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.

That seems like something we can do in WebCore proper without the need for custom bindings.

> > > Source/WebCore/dom/PeerConnection.cpp:137
> > > +PassRefPtr<MediaStreamList> PeerConnection::localStreams()
> > > +{
> > > +    return m_localStreamList;
> > > +}
> > > +
> > > +PassRefPtr<MediaStreamList> PeerConnection::remoteStreams()
> > > +{
> > > +    return m_remoteStreamList;
> > > +}
> > 
> > Do these functions transfer ownership?  If not, they should return raw pointers.  If you haven't read http://www.webkit.org/coding/RefPtr.html you might find it informative.
> > 
> 
> According to the spec, these lists need to be accessible by Javascript. That's why they are reference-counted objects and raw pointers are not used.
> 
> Here's the spec:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#peerconnection

I understand why they're reference-counted objects.  However, you should return raw pointers.  The JavaScript engine can happily take a reference to the objects even if it receive the pointer in the form of a raw pointer.

> > > Source/WebCore/dom/PeerConnection.cpp:283
> > > +    if (!scriptExecutionContext())
> > 
> > Typically we'd store the scriptExecutionContext in a local variable because it's non-trivial to retrieve it.
> 
> Doing this here would lead to errors since you could detach the frame associated to the context but keep a reference to the PeerConnection object. This method avoids the tricky cases I mentioned before and ensures that the proper execution context is returned if available.

I'm talking about a local variable.  If it's possible for the scriptExecutionContext to become detached between the time you null check it and the time you use it, then you've got a crash.  If it can't happen, then you should use a local variable like we do everywhere else where this pattern arises.

> > > Source/WebCore/dom/PeerConnection.h:60
> > > +    PassRefPtr<MediaStreamList> localStreams();
> > > +    PassRefPtr<MediaStreamList> remoteStreams();
> > 
> > These don't transfer ownership and so should return raw pointers.
> 
> Need to be accessible by Javascript as before. 

Same comment as above.  You don't need to return PassRefPtrs in order for JavaScript to be able to take a reference.  PassRefPtrs are for transferring ownership of objects.  You are not transferring ownership at this call site.  Therefore, you should not use PassRefPtr.  Please read http://www.webkit.org/coding/RefPtr.html if you haven't already.

> > > Source/WebCore/dom/PeerConnection.idl:33
> > > +        EventTarget
> > 
> > Shouldn't we just have this interface inherit from EventTarget instead of using this attribute?
> 
> Almost all existing DOM code does it like this instead of inheriting in the idl.

Almost all?  Does that mean inheritance doesn't work?  In any case, if this is a common pattern, we can fix it separately.

> > > Source/WebCore/page/MediaStreamClient.h:77
> > > +    // Signaling message from peer connection.
> > > +    virtual void processSignalingMessage(int peerConnectionId, const String& message) = 0;
> > > +
> > > +    // Message from peer connection.
> > > +    virtual void message(int peerConnectionId, const String& message) = 0;
> > > +
> > > +    // Add a stream to a peer connection.
> > > +    virtual void addStream(int peerConnectionId, const String& streamLabel) = 0;
> > > +
> > > +    // Remove a stream from a peer connection.
> > > +    virtual void removeStream(int peerConnectionId, const String& streamLabel) = 0;
> > > +
> > > +    // A PeerConnection object has been created.
> > > +    virtual void newPeerConnection(int peerConnectionId, const String& configuration) = 0;
> > > +
> > > +    // Peer connection is being closed.
> > > +    virtual void closePeerConnection(int peerConnectionId) = 0;
> > > +
> > > +    // Start negotiation.
> > > +    virtual void startNegotiation(int peerConnectionId) = 0;
> > 
> > Why are these calls going through the client?  I would have expected them to go through the platform.
> 
> It goes through a WebKit page client just as the other OWP APIs (Geolocation, Device Orientation, Speech Input and such).

Some APIs go through the client and some go though the platform.  It's not related to whether they APIs are part of the open web platform.  It's based on whether the methods ask the platform to perform some action or whether they ask the client to make some decision.  These methods clearly look like they're instructing the callee to perform some actions on low-level object (addStream, closePeerConnection) rather than asking for policy (e.g., shouldClosePeerConnection) or notifying the client about what happened (e.g., didClosePeerConnection).  I'm pretty sure these should be part of the platform.

Another way to understand the distinction is to ask yourself how you would implement this feature in another embedding of WebKit.  Would you link in a low-level library to perform these tasks (e.g., an ICE library) or are these decisions/notifications that require application-layer reasoning?

[Adam Barth]

To be clear, we don't need to block this patch on the client/platform distinction.  It would be good to get it right from the start because it can be somewhat of a pain to fix later, but it's not required.

[Tommy Widenflycht]

Comment on attachment 102012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review

>> Source/WebCore/CMakeLists.txt:2009
>> +        dom/PeerConnection.cpp
> 
> PeerConnection shouldn't be in the "DOM" folder.  DOM is for DOM Core.  In all likelihood, we should have a new folder in WebCore for p2p code.

Done.

>>>> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
>>>> +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
>>> 
>>> Why do we need custom bindings for this object?  Is there some feature missing from the code generator?
>> 
>> I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.
> 
> That seems like something we can do in WebCore proper without the need for custom bindings.

Can you give me a hint how to do it? I'll happily get rid of any custom bindings.

>> Source/WebCore/dom/PeerConnection.cpp:42
>> +template <typename T, typename D>
> 
> Please use more descriptive template variables.

Done.

>> Source/WebCore/dom/PeerConnection.cpp:43
>> +class SimpleDispatchTask : public ScriptExecutionContext::Task {
> 
> This object seems more general than just PeerConnection.  Should it move to a more general location where it can be used by other code?

Moved it to a separate file to start with. Where do you suggest the file should live?

>> Source/WebCore/dom/PeerConnection.cpp:69
>> +class DispatchTask : public ScriptExecutionContext::Task {
> 
> Dittto.

Ditto.

>> Source/WebCore/dom/PeerConnection.cpp:99
>> +    // TODO: Verify configuration
> 
> TODO => FIXME.  Please make comments be complete sentences.

Done.

>> Source/WebCore/dom/PeerConnection.cpp:102
>> +    pc->init();
> 
> Why is this init function separate from construction?

Because the init function needs to queue a DispatchTask which doesn't work inside constructors and destructors.

>> Source/WebCore/dom/PeerConnection.cpp:103
>> +    return pc;
> 
> pc.release().
> 
> Also, please use complete words in variable names.

Fixed * 2.

>>>> Source/WebCore/dom/PeerConnection.cpp:137
>>>> +}
>>> 
>>> Do these functions transfer ownership?  If not, they should return raw pointers.  If you haven't read http://www.webkit.org/coding/RefPtr.html you might find it informative.
>> 
>> According to the spec, these lists need to be accessible by Javascript. That's why they are reference-counted objects and raw pointers are not used.
>> 
>> Here's the spec:
>> http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#peerconnection
> 
> I understand why they're reference-counted objects.  However, you should return raw pointers.  The JavaScript engine can happily take a reference to the objects even if it receive the pointer in the form of a raw pointer.

Done.

>> Source/WebCore/dom/PeerConnection.cpp:139
>> +// Sends the message down to the user agent implementation.
> 
> Please remove this comment.  Perhaps we should rename the function to say what this comment says?

Function can't be renamed since it is an JavaScript function.

>> Source/WebCore/dom/PeerConnection.cpp:151
>> +void PeerConnection::send(const AtomicString& text, ExceptionCode& ec)
> 
> Why AtomicString?  I would have expected String.

Have removed all usages of AtomicString.

>>>> Source/WebCore/dom/PeerConnection.cpp:283
>>>> +    if (!scriptExecutionContext())
>>> 
>>> Typically we'd store the scriptExecutionContext in a local variable because it's non-trivial to retrieve it.
>> 
>> Doing this here would lead to errors since you could detach the frame associated to the context but keep a reference to the PeerConnection object. This method avoids the tricky cases I mentioned before and ensures that the proper execution context is returned if available.
> 
> I'm talking about a local variable.  If it's possible for the scriptExecutionContext to become detached between the time you null check it and the time you use it, then you've got a crash.  If it can't happen, then you should use a local variable like we do everywhere else where this pattern arises.

Done.

>> Source/WebCore/dom/PeerConnection.cpp:288
>> +    scriptExecutionContext()->postTask(DispatchTask<PeerConnection, int, SerializedScriptValue>::create(this, &PeerConnection::dispatchMessageEvent, 0, data));
> 
> data.release()

Done.

>> Source/WebCore/dom/PeerConnection.cpp:307
>> +    scriptExecutionContext()->postTask(DispatchTask<PeerConnection, AtomicString, MediaStream>::create(this, &PeerConnection::dispatchStreamEvent, eventNames().addstreamEvent, s));
> 
> What's the point of converting stream to a RefPtr?  We should just pass the PassReftPtr on to postTask.

Done, was just a leftover from refactored code.

>>>> Source/WebCore/dom/PeerConnection.h:60
>>>> +    PassRefPtr<MediaStreamList> remoteStreams();
>>> 
>>> These don't transfer ownership and so should return raw pointers.
>> 
>> Need to be accessible by Javascript as before.
> 
> Same comment as above.  You don't need to return PassRefPtrs in order for JavaScript to be able to take a reference.  PassRefPtrs are for transferring ownership of objects.  You are not transferring ownership at this call site.  Therefore, you should not use PassRefPtr.  Please read http://www.webkit.org/coding/RefPtr.html if you haven't already.

Done.

>> Source/WebCore/dom/PeerConnection.idl:31
>> +        ConstructorParameters=2,
> 
> Can we really not provide the type signature for the constructor?  That seems like something we should fix in the code generator.

Have not found a way to do so.

>>>> Source/WebCore/dom/PeerConnection.idl:33
>>>> +        EventTarget
>>> 
>>> Shouldn't we just have this interface inherit from EventTarget instead of using this attribute?
>> 
>> Almost all existing DOM code does it like this instead of inheriting in the idl.
> 
> Almost all?  Does that mean inheritance doesn't work?  In any case, if this is a common pattern, we can fix it separately.

Dunno about if it doesn't work or not, I have just followed the pattern in surrounding code.

>> Source/WebCore/dom/PeerConnection.idl:46
>> +        [StrictTypeChecking] void addStream(in MediaStream stream)
> 
> I've never seen this StrictTypeChecking before.  What does that mean?

It checks that that the pointer is non-null and indeed a MediaStream.

>> Source/WebCore/dom/PeerConnection.idl:69
>> +                                 in boolean useCapture);
> 
> The useCapture argument should be marked [Optional]

Done.

>> Source/WebCore/dom/SignalingCallback.idl:25
>> +module core {
> 
> Why the "Core" module?  This API isn't part of DOM Core.

Fixed.

>>>> Source/WebCore/page/MediaStreamClient.h:77
>>>> +    virtual void startNegotiation(int peerConnectionId) = 0;
>>> 
>>> Why are these calls going through the client?  I would have expected them to go through the platform.
>> 
>> It goes through a WebKit page client just as the other OWP APIs (Geolocation, Device Orientation, Speech Input and such).
> 
> Some APIs go through the client and some go though the platform.  It's not related to whether they APIs are part of the open web platform.  It's based on whether the methods ask the platform to perform some action or whether they ask the client to make some decision.  These methods clearly look like they're instructing the callee to perform some actions on low-level object (addStream, closePeerConnection) rather than asking for policy (e.g., shouldClosePeerConnection) or notifying the client about what happened (e.g., didClosePeerConnection).  I'm pretty sure these should be part of the platform.
> 
> Another way to understand the distinction is to ask yourself how you would implement this feature in another embedding of WebKit.  Would you link in a low-level library to perform these tasks (e.g., an ICE library) or are these decisions/notifications that require application-layer reasoning?

Our first approach is to route everything to the client and there use the webrtc/libjingle/existing chrome APIs to implement this feature. It's not ideal for those ports that want to do this in platform. I have been contacted by the Ericsson team and we will meetup as soon as the vacations is over in Sweden to make a plan how to solve this in a way that will work for both approaches.

>> Source/WebCore/page/MediaStreamController.cpp:37
>> +// TODO: This is used for holding list of PeerConnection as well,
> 
> TODO -> FIXME

Done.

>> Source/WebCore/page/MediaStreamController.cpp:96
>> +    // TODO: Add for peer connection
> 
> TODO -> FIXME.  Please use complete sentences for comments.

Done.

>> Source/WebCore/page/MediaStreamController.cpp:172
>> +    int pagePeerConnectionId(-1);
> 
> Please use the assignment form of the constructor.

Done.

>> Source/WebCore/page/MediaStreamController.cpp:193
>> +    if (pagePeerConnectionId != -1)
> 
> Generally WebKit frowns upon in-band signaling.  Is there a better design here?  For example, perhaps frameToPagePeerConnectionId should return a bool and have an out parameter for the ID.

Thanks for the input. Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:372
>> +    return peerConnection;
> 
> peerConnection.release()

Done.

>> Source/WebCore/page/MediaStreamFrameController.h:173
>> +    // Creates a new PeerConnection object.
> 
> These comments should be removed.  Probably all the comments in this file are useless and should be removed, but I haven't checked them all.

Useless comments removed.

[Tommy Widenflycht]

Created attachment 102362 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 102362 [details]
Patch

Attachment 102362 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9268466

[Adam Barth]

(In reply to comment #10)
> (From update of attachment 102012 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review
> 
> >>>> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
> >>>> +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
> >>> 
> >>> Why do we need custom bindings for this object?  Is there some feature missing from the code generator?
> >> 
> >> I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.
> > 
> > That seems like something we can do in WebCore proper without the need for custom bindings.
> 
> Can you give me a hint how to do it? I'll happily get rid of any custom bindings.

Is the issue that you need to find the Frame?  You can try CallWith=ScriptExecutionContext (or maybe one of the other CallWith options).  Maybe we need to generalize those to work with constructors?

> >> Source/WebCore/dom/PeerConnection.cpp:43
> >> +class SimpleDispatchTask : public ScriptExecutionContext::Task {
> > 
> > This object seems more general than just PeerConnection.  Should it move to a more general location where it can be used by other code?
> 
> Moved it to a separate file to start with. Where do you suggest the file should live?

I would put it next to ScriptExecutionContext::Task.

> >> Source/WebCore/dom/PeerConnection.cpp:102
> >> +    pc->init();
> > 
> > Why is this init function separate from construction?
> 
> Because the init function needs to queue a DispatchTask which doesn't work inside constructors and destructors.

Can we rename the function to something like "queueDispatchTask" ?

> >> Source/WebCore/dom/PeerConnection.cpp:139
> >> +// Sends the message down to the user agent implementation.
> > 
> > Please remove this comment.  Perhaps we should rename the function to say what this comment says?
> 
> Function can't be renamed since it is an JavaScript function.

Ok.

> >> Source/WebCore/dom/PeerConnection.idl:31
> >> +        ConstructorParameters=2,
> > 
> > Can we really not provide the type signature for the constructor?  That seems like something we should fix in the code generator.
> 
> Have not found a way to do so.

Seems like something we should add.  You can specify types in WebIDL.

> >>>> Source/WebCore/dom/PeerConnection.idl:33
> >>>> +        EventTarget
> >>> 
> >>> Shouldn't we just have this interface inherit from EventTarget instead of using this attribute?
> >> 
> >> Almost all existing DOM code does it like this instead of inheriting in the idl.
> > 
> > Almost all?  Does that mean inheritance doesn't work?  In any case, if this is a common pattern, we can fix it separately.
> 
> Dunno about if it doesn't work or not, I have just followed the pattern in surrounding code.

Ok.

[Adam Barth]

Comment on attachment 102362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102362&action=review

> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:56
> +    Frame* frame = asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame();
> +    MediaStreamFrameController* frameController = frame ? frame->mediaStreamFrameController() : 0;

Is this the right MediaStreamFrameController ?  Why the dynamicGlobalObject ?  I might have expected the MediaStreamFrameController associated with the frame associated with the DOMWindow from which we got the constructor object.

Is the choice of controller observable?  If so, does the spec say which controller to use?

> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:64
> +    Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
> +    MediaStreamFrameController* frameController = frame ? frame->mediaStreamFrameController() : 0;

So, here you're grabbing the current context, which isn't the same as the dynamic global object.  That means your JSC and V8 implementations aren't the same.

> Source/WebCore/page/MediaStreamClient.h:77
> +    // Start negotiation.
> +    virtual void startNegotiation(int peerConnectionId) = 0;

These comments are really silly.

> Source/WebCore/page/MediaStreamController.cpp:173
> +    // TODO: Is there a better way to find the global ID in the map?

TODO => FIXME

[Adam Barth]

Modulo the issues / questions about the custom constructor, I'm happy with this patch.  I'm happy to r+ it, but I don't actually know anything about the substance.  :)

Do you want something how knows about PeerConnection in specific to look before landing?

[Tommy Widenflycht]

Comment on attachment 102012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review

>>>>>> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
>>>>>> +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
>>>>> 
>>>>> Why do we need custom bindings for this object?  Is there some feature missing from the code generator?
>>>> 
>>>> I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.
>>> 
>>> That seems like something we can do in WebCore proper without the need for custom bindings.
>> 
>> Can you give me a hint how to do it? I'll happily get rid of any custom bindings.
> 
> Is the issue that you need to find the Frame?  You can try CallWith=ScriptExecutionContext (or maybe one of the other CallWith options).  Maybe we need to generalize those to work with constructors?

Is it OK if I address this in a follow-up patch?

>>>> Source/WebCore/dom/PeerConnection.cpp:43

>>> 
>>> This object seems more general than just PeerConnection.  Should it move to a more general location where it can be used by other code?
>> 
>> Moved it to a separate file to start with. Where do you suggest the file should live?
> 
> I would put it next to ScriptExecutionContext::Task.

Done.

>>>> Source/WebCore/dom/PeerConnection.cpp:102
>>>> +    pc->init();
>>> 
>>> Why is this init function separate from construction?
>> 
>> Because the init function needs to queue a DispatchTask which doesn't work inside constructors and destructors.
> 
> Can we rename the function to something like "queueDispatchTask" ?

The "init() after contructor" pattern is quite common and the function does register the new PeerConnection object with the controllers as well as queue a new Task. Would very muck like to keep the name.

>> Source/WebCore/dom/PeerConnection.cpp:177
>> +    RefPtr<MediaStream> s = stream;
> 
> Typically we'd call "stream" prpStream and then rename "s" to stream.

Done.

>> Source/WebCore/dom/PeerConnection.h:46
>> +    public MediaStreamFrameController::PeerConnectionClient {
> 
> One line pls.

Done.

>> Source/WebCore/dom/PeerConnection.h:48
>> +    // Must match the constants in the .idl file.
> 
> Complete sentences pls.

Done.

>> Source/WebCore/page/MediaStreamFrameController.cpp:442
>> +    peerConnection->streamAdded(stream);
> 
> stream.release()

Done.

[Adam Barth]

(In reply to comment #16)
> (From update of attachment 102012 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102012&action=review
> 
> >>>>>> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:43
> >>>>>> +EncodedJSValue JSC_HOST_CALL JSPeerConnectionConstructor::constructJSPeerConnection(ExecState* exec)
> >>>>> 
> >>>>> Why do we need custom bindings for this object?  Is there some feature missing from the code generator?
> >>>> 
> >>>> I think it's because it needs to register itself to the MediaStreamFrameController. This controller tracks all the script objects related to the local frame and notifies them in case the access to the embedder client is broken (for example, the frame gets detached from the page or transferred to another one). This is part of the main core of the API design and tries to solve nasty corner cases that have produced many headaches to the Geolocation API.
> >>> 
> >>> That seems like something we can do in WebCore proper without the need for custom bindings.
> >> 
> >> Can you give me a hint how to do it? I'll happily get rid of any custom bindings.
> > 
> > Is the issue that you need to find the Frame?  You can try CallWith=ScriptExecutionContext (or maybe one of the other CallWith options).  Maybe we need to generalize those to work with constructors?
> 
> Is it OK if I address this in a follow-up patch?

Sure.  Please make the V8 bindings and JSC bindings consistent though.  The easiest way to do that is to change the V8 bindings to use the Entered context (which is the same as the dynamic context you're using in the JSC bindings).  That's not really the right thing, but it will let us make progress.

[Adam Barth]

Comment on attachment 102362 [details]
Patch

I'm making this r+, but there are still a couple of open issues to address (making the JSC and V8 custom implementations match and fixing the GTK compile issue).

[Tommy Widenflycht]

Thanks, a new patch will come later today.

(In reply to comment #18)
> (From update of attachment 102362 [details])
> I'm making this r+, but there are still a couple of open issues to address (making the JSC and V8 custom implementations match and fixing the GTK compile issue).

[Tommy Widenflycht]

Created attachment 102633 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 102362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102362&action=review

>> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:56
>> +    MediaStreamFrameController* frameController = frame ? frame->mediaStreamFrameController() : 0;
> 
> Is this the right MediaStreamFrameController ?  Why the dynamicGlobalObject ?  I might have expected the MediaStreamFrameController associated with the frame associated with the DOMWindow from which we got the constructor object.
> 
> Is the choice of controller observable?  If so, does the spec say which controller to use?

I am out of my depth here, can you suggest how to do this so that it matches the v8 implementation?

>> Source/WebCore/page/MediaStreamClient.h:77
>> +    virtual void startNegotiation(int peerConnectionId) = 0;
> 
> These comments are really silly.

More silly comments removed, and instead replaced with two hopefully less silly ones.

>> Source/WebCore/page/MediaStreamController.cpp:173
>> +    // TODO: Is there a better way to find the global ID in the map?
> 
> TODO => FIXME

Done.

[Adam Barth]

> I am out of my depth here, can you suggest how to do this so that it matches the v8 implementation?

Sure.  The easiest way to do that is to change the V8 bindings to use the Entered context (which is the same as the dynamic context you're using in the JSC bindings).  That's not really the right thing, but it will let us make progress.

[Collabora GTK+ EWS bot]

Comment on attachment 102633 [details]
Patch

Attachment 102633 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9258020

[Gustavo Noronha (kov)]

Comment on attachment 102633 [details]
Patch

Attachment 102633 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9290818

[Tommy Widenflycht]

Created attachment 102753 [details]
Patch

[Tommy Widenflycht]

(In reply to comment #22)
> > I am out of my depth here, can you suggest how to do this so that it matches the v8 implementation?
> 
> Sure.  The easiest way to do that is to change the V8 bindings to use the Entered context (which is the same as the dynamic context you're using in the JSC bindings).  That's not really the right thing, but it will let us make progress.

Done. I clearly need to learn a lot more about this, can you recommend some documentation?

And btw I looked through the code generators and there doesn't seem to be any mechanism to specify types to a constructor; only constructors without any parameters with the exception of an optional ScripExecutionContext. So I guess the custom constructors are here to stay, which makes the previous paragraph even more important.

Also I think I found the GTK issue finally but the trybots for it are really slow today :/

Lastly: A huge thank you for the thorough review!

[Adam Barth]

> > Sure.  The easiest way to do that is to change the V8 bindings to use the Entered context (which is the same as the dynamic context you're using in the JSC bindings).  That's not really the right thing, but it will let us make progress.
> 
> Done. I clearly need to learn a lot more about this, can you recommend some documentation?

There isn't really any documentation for this sort of thing.  We've been sharing the knowledge via discussions and code reviews, but that's starting to have scaling problems.  Generally, the WebKit project's philosophy isn't to create documentation because it gets out of date as it's not tested.  (Most folks who are new to the project find this philosophy nutty, but it's worked for us pretty well up until now.)

> And btw I looked through the code generators and there doesn't seem to be any mechanism to specify types to a constructor; only constructors without any parameters with the exception of an optional ScripExecutionContext. So I guess the custom constructors are here to stay, which makes the previous paragraph even more important.

Not really.  We just need to make the code generator smarter.

[Adam Barth]

Comment on attachment 102753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102753&action=review

The Frame-finding code still isn't the same between JSC and V8, but I feel bad bouncing your patch again.  I've also noted some minor nits that you shouldn't feel obligated to fix.  If you'd like, you should feel free to update this patch or deal with the remaining (minor) issues in a follow-up patch.

> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:64
> +    Frame* frame = V8Proxy::retrieveFrameForEnteredContext();
> +    MediaStreamFrameController* frameController = frame ? frame->mediaStreamFrameController() : 0;

It looks like you changed both of these.

retrieveFrameForEnteredContext <=> asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame()

but you're using lexicalGlobalObject for the JSC bindings...  I'm sorry these names are confusing.  The problem is that the V8 names come from the V8 API and the JSC names come from the programming languages literature.  Neither group wants to change their names to match the other.

> Source/WebCore/p2p/PeerConnection.cpp:104
> +    CString utf8 = text.utf8();
> +    if (utf8.length() > 504) {

Should we make 504 a named constant?  This check is strange, but I assume it's from the spec.

> Source/WebCore/p2p/PeerConnection.cpp:182
> +    RefPtr<MediaStream> s = stream;

prpStream

> Source/WebCore/page/MediaStreamFrameController.cpp:407
> +    RefPtr<MediaStream> stream = streamRef;

streamRef => prpStream

> Source/WebCore/page/MediaStreamFrameController.cpp:416
> +    RefPtr<MediaStream> stream = streamRef;

ditto

[Tommy Widenflycht]

Created attachment 102798 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 102753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102753&action=review

>> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:64
>> +    MediaStreamFrameController* frameController = frame ? frame->mediaStreamFrameController() : 0;
> 
> It looks like you changed both of these.
> 
> retrieveFrameForEnteredContext <=> asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame()
> 
> but you're using lexicalGlobalObject for the JSC bindings...  I'm sorry these names are confusing.  The problem is that the V8 names come from the V8 API and the JSC names come from the programming languages literature.  Neither group wants to change their names to match the other.

After an conversation with abarth we have agreed to land it like this and Adam will then make sure both custom functions are correct in a followup patch.

>> Source/WebCore/p2p/PeerConnection.cpp:104
>> +    if (utf8.length() > 504) {
> 
> Should we make 504 a named constant?  This check is strange, but I assume it's from the spec.

Done. Yes the spec says 504 bytes.

>> Source/WebCore/p2p/PeerConnection.cpp:182
>> +    RefPtr<MediaStream> s = stream;
> 
> prpStream

Done.

>> Source/WebCore/page/MediaStreamFrameController.cpp:407
>> +    RefPtr<MediaStream> stream = streamRef;
> 
> streamRef => prpStream

Done.

>> Source/WebCore/page/MediaStreamFrameController.cpp:416
>> +    RefPtr<MediaStream> stream = streamRef;
> 
> ditto

Done.

[WebKit Review Bot]

Comment on attachment 102798 [details]
Patch

Clearing flags on attachment: 102798

Committed r92304: <http://trac.webkit.org/changeset/92304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.