Created attachment 108421 [details]
proposed patch

Created attachment 109783 [details]
Updated patch

Created attachment 110350 [details]
Patch 3

Comment on attachment 110350 [details]
Patch 3

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?

Created attachment 110529 [details]
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.

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

Comment on attachment 110529 [details]
Patch 4

Would you be willing to make the small tweaks above?  Then I'll through the patch one more time in detail and hopefully we'll be able to land it.

Created attachment 110603 [details]
Patch 5

(In reply to comment #6)

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

Let's go with raw pointers for consistency.
 
> > 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.  :)

Fixed.

I'll upload the next patch which updates MediaStream/LocalMediaStream in http://webkit.org/b/68464

Comment on attachment 110603 [details]
Patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=110603&action=review

> Source/WebCore/p2p/PeerConnection.cpp:39
> -PassRefPtr<PeerConnection> PeerConnection::create(MediaStreamFrameController* frameController, int id, const String& configuration, PassRefPtr<SignalingCallback> signalingCallback)
> +PassRefPtr<PeerConnection> PeerConnection::create(const String& serverConfiguration, PassRefPtr<SignalingCallback> signalingCallback, ScriptExecutionContext* context)

I think the patch to move this to the front has landed.  We need to check that this compiles when landing.

> Source/WebCore/p2p/PeerConnection.cpp:169
> +    MediaStreamDescriptor* streamDescriptor = 0;
> +    size_t i = m_pendingAddStreams.find(streamDescriptor);
> +    if (i != notFound) {
> +        m_pendingAddStreams.remove(i);
>          return;
> +    }
> +
> +    m_pendingRemoveStreams.append(streamDescriptor);
> +    if (m_iceStarted)
> +        ensureStreamChangeScheduled();

This code appears duplicated.  Maybe break out into a helper function?

Constructor change is Bug 69799.  I'm going to land that bug and then land an updated version of this patch to account for any merge conflicts.

Comment on attachment 110603 [details]
Patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=110603&action=review

>> Source/WebCore/p2p/PeerConnection.cpp:169
>> +        ensureStreamChangeScheduled();
> 
> This code appears duplicated.  Maybe break out into a helper function?

Ah, i see it's different now.

Created attachment 110722 [details]
Patch for landing

Comment on attachment 110722 [details]
Patch for landing

Rejecting attachment 110722 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
es are: WebCore::PeerConnection::PeerConnection(WebCore::ScriptExecutionContext*, const WTF::String&, WTF::PassRefPtr<WebCore::SignalingCallback>)
Source/WebCore/p2p/PeerConnection.h:44: note:                 WebCore::PeerConnection::PeerConnection(const WebCore::PeerConnection&)
  CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/page/Chrome.o
make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/p2p/PeerConnection.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/9999040

Created attachment 110743 [details]
Patch for landing

Comment on attachment 110743 [details]
Patch for landing

Clearing flags on attachment: 110743

Committed r97305: <http://trac.webkit.org/changeset/97305>

All reviewed patches have been landed.  Closing bug.