[Webkit-unassigned] [Bug 68462] Update PeerConnection to use WebCore platform interfaces

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68462


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110350|review?                     |review-
               Flag|                            |




--- Comment #4 from Adam Barth <abarth at webkit.org>  2011-10-10 09:53:43 PST ---
(From update of attachment 110350)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.