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

Tue Oct 11 10:06:25 PDT 2011

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

Adam Bergkvist <adam.bergkvist at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110350|0                           |1
        is obsolete|                            |
 Attachment #110529|                            |review?
               Flag|                            |

--- Comment #5 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-10-11 10:06:25 PST ---
Created an attachment (id=110529)
 --> (https://bugs.webkit.org/attachment.cgi?id=110529&action=review)
Patch 4

(In reply to comment #4)
> (From update of attachment 110350 [details])
> 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.

Good to hear. Thanks for reviewing this so quickly.

> > 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.

It works for V8, but the bindings generator for JSC doesn't seem to be able to deal with the constructor arguments nor the ScriptExecutionContext argument yet.

> > 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?

Apparently it can since all other JSC custom constructor functions have it. I added the check to align with those.

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

It has to come after the "real" constructor arguments for the generated V8 constructor function to work.

> > Source/WebCore/dom/MediaStreamList.cpp:61
> > +void MediaStreamList::remove(const RefPtr<MediaStream>& stream)
> 
> Why not a raw pointer?

MediaStreamList is never used with raw pointers and I think the API is nicer to use like streams->contains(stream) instead of streams->contains(stream.get()). I've seen references used for key arguments elsewhere in the project for "collection-like" objects. Is that OK or should I change to a raw pointer?

> > Source/WebCore/dom/MediaStreamList.cpp:68
> > +bool MediaStreamList::contains(const RefPtr<MediaStream>& stream) const
> 
> Why not a raw pointer?

See previous comment.

> > 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.  :)

Fixed.

> > 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.
>

You're right, it shouldn't be const. It compiles because of this: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/PassRefPtr.h#L68.

|stream| is used three times in addStream() (including the FIXME) so the local RefPtr is needed.

> > 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?

It's needed because it's pure virtual in EventTarget.

> > 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.

It's actually only called once. I've changed the "if" to an ASSERT instead.

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

Fixed (renamed to ensureStreamChangeScheduled()).

> > Source/WebCore/p2p/PeerConnection.cpp:307
> > +void PeerConnection::streamChangeTimerFired(Timer<PeerConnection>*)
> 
> Usually we ASSERT_UNUSED that we get the right timer parameter.

Fixed.

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

Fixed.

> > 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.

The initial readyState NEW is not handled in the switch. An alternative would be to add case NEW and move the ASSERT there.

> > Source/WebCore/p2p/PeerConnection.h:42
> >  namespace WebCore {
> 
> We like a blank line after this line.

Fixed.

> > 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.

This line was actually not introduced in this patch (there's both + and - lines for it). I've removed the constant since it's only used once in a clear context.

> > 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?

We can't generate a working constructor function for JSC yet.

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