[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





--- Comment #6 from Adam Barth <abarth at webkit.org>  2011-10-11 10:43:03 PST ---
> > > 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.

Yep.  I found the bug report about this issue, and it is in the process of being fixed.

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

Yep.  I've encouraged the person working on constructors to fix this.  There's a slight rights your change will conflict with that one, but we'll be able to resolve that if that happens.

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

I can understand the aesthetic issue, but we use these various types to communicate different ownership patterns.  We use this type (somewhat rarely) to mean that it's essential that the caller explicitly hold a reference to the object (e.g., on the stack) because this function could trigger a cascade of dereferences.

In the end, it's just a style thing and not a big deal, but it's slightly more consistent with the rest of WebKit to just take a raw pointer here.

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

Ok.  I must have missed the other ones.

> > Why do we need this function if we just call through to our superclass?
> 
> It's needed because it's pure virtual in EventTarget.

Ah.

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

Even better.

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

Please do.  That's part of why we like that style.  :)

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

The folks who have those compilers must have this feature turned off.  :)

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