[webkit-reviews] review denied: [Bug 68462] Update PeerConnection to use WebCore platform interfaces : [Attachment 110350] Patch 3

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 68462: Update PeerConnection to use WebCore platform interfaces
https://bugs.webkit.org/show_bug.cgi?id=68462

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110350&action=review


This looks great.  There are a bunch of nit-picky details below.  I'd like to
see another iteration of this patch before we land it, but it's very close.

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

You might be able do this with IDL now.  There's a
ConstructWith=ScriptExecutionContext attribute.

> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:46
> +    ScriptExecutionContext* context =
jsConstructor->scriptExecutionContext();
> +    if (!context)
> +	   return throwVMError(exec, createReferenceError(exec, "PeerConnection
constructor associated document is unavailable"));

Can this ever be null?

> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:59
> +    RefPtr<PeerConnection> peerConnection =
PeerConnection::create(serverConfiguration, signalingCallback.release(),
context);

context should probably be the first argument.

> Source/WebCore/dom/MediaStreamList.cpp:61
> +void MediaStreamList::remove(const RefPtr<MediaStream>& stream)

Why not a raw pointer?

> Source/WebCore/dom/MediaStreamList.cpp:68
> +bool MediaStreamList::contains(const RefPtr<MediaStream>& stream) const

Why not a raw pointer?

> Source/WebCore/p2p/PeerConnection.cpp:116
> -void PeerConnection::addStream(PassRefPtr<MediaStream> prpStream,
ExceptionCode& ec)
> +void PeerConnection::addStream(const PassRefPtr<MediaStream> prpStream,
ExceptionCode& ec)

Why const ?  Using a const PassRefPtr doesn't make a whole lot of sense.  The
point of a PassRefPtr is to be able to transfer the reference, which you can't
do if it's const.  :)

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

How does this compile with const?  It should transfer the ref out of the
prpStream and into stream.  It looks like we only use |stream| once (as the
argument to append).  We probably don't need to create a local RefPtr for it. 
We can just pass the prpStream directly to append.

> Source/WebCore/p2p/PeerConnection.cpp:250
>  ScriptExecutionContext* PeerConnection::scriptExecutionContext() const
>  {
> -    return mediaStreamFrameController() ?
mediaStreamFrameController()->scriptExecutionContext() : 0;
> +    return ActiveDOMObject::scriptExecutionContext();
> +}

Why do we need this function if we just call through to our superclass?

> Source/WebCore/p2p/PeerConnection.cpp:283
> +    if (!m_initialNegotiationTimer.isActive())
> +	   m_initialNegotiationTimer.startOneShot(0);

You might consider calling this function "ensure" XYZ because it doesn't always
schedule the initial negotiation.  It jus ensures that it is scheduled.

> Source/WebCore/p2p/PeerConnection.cpp:304
> +    if (!m_streamChangeTimer.isActive())
> +	   m_streamChangeTimer.startOneShot(0);

Same comment here.

> Source/WebCore/p2p/PeerConnection.cpp:307
> +void PeerConnection::streamChangeTimerFired(Timer<PeerConnection>*)

Usually we ASSERT_UNUSED that we get the right timer parameter.

> Source/WebCore/p2p/PeerConnection.cpp:332
> +void PeerConnection::readyStateChangeTimerFired(Timer<PeerConnection>*)

Same here.

> Source/WebCore/p2p/PeerConnection.cpp:358
> +    default:
> +	   ASSERT_NOT_REACHED();

We usually leave off the default case and let the compiler complain if we've
forgotten an enum value.

> Source/WebCore/p2p/PeerConnection.h:42
>  namespace WebCore {

We like a blank line after this line.

> Source/WebCore/p2p/PeerConnection.h:93
> +    static const size_t kMaxMessageUTF8Length = 504;

This we usually put as a static global in the cpp file because not all
compilers can handle this syntax.

> Source/WebCore/p2p/PeerConnection.idl:33
> +	   ConstructorWith=ScriptExecutionContext,

Oh, you do have this.  I thought this passed the ScriptExecutionContext as the
first argument.

> Source/WebCore/p2p/PeerConnection.idl:34
> +	   JSCustomConstructor,

Why does it need a JSCustomConstructor then?