[webkit-reviews] review denied: [Bug 65101] MediaStream API: Implement PeerConnection and SignalingCallback : [Attachment 102012] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 65101: MediaStream API: Implement PeerConnection and SignalingCallback
https://bugs.webkit.org/show_bug.cgi?id=65101

Attachment 102012: Patch
https://bugs.webkit.org/attachment.cgi?id=102012&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.