Bug 125655 – Checking RTCPeerConnection signalingState before setting local/remoteDescription

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

Before setting a session description RTCPeerConnection must check if it is in valid state for that SDP type.

[Thiago de Barros Lacerda]

Created attachment 219120 [details]
Patch

[Eric Carlson]

Comment on attachment 219120 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        * Modules/mediastream/RTCPeerConnection.cpp:
> +        (WebCore::RTCPeerConnection::validateSetLocalDescription):
> +        (WebCore::RTCPeerConnection::validateSetRemoteDescription):
> +        (WebCore::RTCPeerConnection::setLocalDescription):
> +        (WebCore::RTCPeerConnection::setRemoteDescription):

Nit: as before, I think it is helpful to include a brief description of what changed for each method listed in the ChangeLog.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:215
> +bool RTCPeerConnection::validateSetLocalDescription(RTCSessionDescription* localDescription)

Nit: I don't think "validateSetLocalDescription" is a great name, what is "Set"? Maybe something like "validateLocalSessionDescription", or "validateLocalDescriptionState"?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:227
> +bool RTCPeerConnection::validateSetRemoteDescription(RTCSessionDescription* remoteDescription)

Ditto about "Set", it doesn't help me understand what the method does.

[Thiago de Barros Lacerda]

(In reply to comment #2)
> (From update of attachment 219120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219120&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        * Modules/mediastream/RTCPeerConnection.cpp:
> > +        (WebCore::RTCPeerConnection::validateSetLocalDescription):
> > +        (WebCore::RTCPeerConnection::validateSetRemoteDescription):
> > +        (WebCore::RTCPeerConnection::setLocalDescription):
> > +        (WebCore::RTCPeerConnection::setRemoteDescription):
> 
> Nit: as before, I think it is helpful to include a brief description of what changed for each method listed in the ChangeLog.

OK.

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:215
> > +bool RTCPeerConnection::validateSetLocalDescription(RTCSessionDescription* localDescription)
> 
> Nit: I don't think "validateSetLocalDescription" is a great name, what is "Set"? Maybe something like "validateLocalSessionDescription", or "validateLocalDescriptionState"?

The "set" is because the action to set the SDP, this method is called when setLocalDescription is executed. But I will rename it to validadeLocalDescriptionState :)

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:227
> > +bool RTCPeerConnection::validateSetRemoteDescription(RTCSessionDescription* remoteDescription)
> 
> Ditto about "Set", it doesn't help me understand what the method does.

[Thiago de Barros Lacerda]

Created attachment 219284 [details]
Patch

[Thiago de Barros Lacerda]

I renamed the methods to checkStateForLocal/RemoteDescription, which I found more meaningful.

[WebKit Commit Bot]

Comment on attachment 219284 [details]
Patch

Rejecting attachment 219284 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 219284, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
pranswer.html
patching file LayoutTests/fast/mediastream/RTCPeerConnection-remoteDescription-expected.txt
patching file LayoutTests/fast/mediastream/RTCPeerConnection-remoteDescription.html
patching file LayoutTests/fast/mediastream/RTCPeerConnection-stable-expected.txt
patching file LayoutTests/fast/mediastream/RTCPeerConnection-stable.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Eric Carlson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/49908112

[Thiago de Barros Lacerda]

Created attachment 219382 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 219382 [details]
Patch for landing

Clearing flags on attachment: 219382

Committed r160693: <http://trac.webkit.org/changeset/160693>