Bug 125655

Summary:

Checking RTCPeerConnection signalingState before setting local/remoteDescription

Product:

WebKit

Reporter:

Thiago de Barros Lacerda <thiago.lacerda>

Component:

WebCore Misc.

Assignee:

Thiago de Barros Lacerda <thiago.lacerda>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric.carlson, glenn, hta, jer.noble, pnormand, tommyw

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

125574

Bug Blocks:

124288

Attachments:

Description	Flags
Patch	none
Patch	eric.carlson: review+, commit-queue: commit-queue-
Patch for landing	none

Thiago de Barros Lacerda

Reported 2013-12-12 14:29:11 PST

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

Attachments
Patch (68.61 KB, patch) 2013-12-12 14:32 PST, Thiago de Barros Lacerda	no flags	Details Formatted Diff Diff
Patch (69.38 KB, patch) 2013-12-15 18:16 PST, Thiago de Barros Lacerda	eric.carlson: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Patch for landing (69.46 KB, patch) 2013-12-16 19:00 PST, Thiago de Barros Lacerda	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Thiago de Barros Lacerda

Comment 1 2013-12-12 14:32:02 PST

Created attachment 219120 [details] Patch

Eric Carlson

Comment 2 2013-12-13 13:31:59 PST

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

Comment 3 2013-12-15 17:36:53 PST

(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

Comment 4 2013-12-15 18:16:12 PST

Created attachment 219284 [details] Patch

Thiago de Barros Lacerda

Comment 5 2013-12-15 18:17:28 PST

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

WebKit Commit Bot

Comment 6 2013-12-16 09:40:00 PST

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

Comment 7 2013-12-16 19:00:13 PST

Created attachment 219382 [details] Patch for landing

WebKit Commit Bot

Comment 8 2013-12-16 20:09:05 PST

Comment on attachment 219382 [details] Patch for landing Clearing flags on attachment: 219382 Committed r160693: <http://trac.webkit.org/changeset/160693>

Note You need to log in before you can comment on or make changes to this bug.