Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(25)

Issue 17001: Bug 784519

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 8 months ago by ekr-webrtc
Modified:
7 years, 7 months ago
Reviewers:
abr, rescorla
Visibility:
Public.

Description

Bug 784519

Patch Set 1 #

Total comments: 17

Patch Set 2 : Updated draft #

Total comments: 7

Patch Set 3 : Adam's latest revision #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M dom/media/PeerConnection.js View 1 2 3 chunks +49 lines, -8 lines 2 comments Download
M dom/media/bridge/IPeerConnection.idl View 3 chunks +11 lines, -0 lines 0 comments Download
M media/webrtc/signaling/include/CC_CallInfo.h View 1 2 chunks +16 lines, -0 lines 0 comments Download
M media/webrtc/signaling/signaling.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp View 1 2 6 chunks +51 lines, -2 lines 1 comment Download
M media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h View 1 3 chunks +16 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M media/webrtc/signaling/src/sipcc/core/ccapp/CCProvider.h View 2 chunks +2 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c View 1 1 chunk +19 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c View 1 2 6 chunks +18 lines, -5 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/common/ui.c View 1 2 4 chunks +74 lines, -41 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c View 1 2 17 chunks +137 lines, -136 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c View 1 1 chunk +1 line, -1 line 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h View 2 chunks +1 line, -33 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/includes/sessionTypes.h View 1 chunk +1 line, -0 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/includes/uiapi.h View 2 chunks +8 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/include/ccapi_call_info.h View 1 chunk +8 lines, -0 lines 0 comments Download
A media/webrtc/signaling/src/sipcc/include/fsmdef_states.h View 1 2 1 chunk +44 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.cpp View 1 2 chunks +11 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallInfo.h View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 3
rescorla
http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): http://firefox-codereview.appspot.com/17001/diff/1/dom/media/PeerConnection.js#newcode253 dom/media/PeerConnection.js:253: // These should line up with the state machine ...
7 years, 8 months ago #1
ekr-webrtc
One bug and a bunch of style-type issues https://firefox-codereview.appspot.com/17001/diff/6001/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/17001/diff/6001/dom/media/PeerConnection.js#newcode997 dom/media/PeerConnection.js:997: case ...
7 years, 7 months ago #2
ekr-webrtc
7 years, 7 months ago #3
LGTM with one bug below.

https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnect...
File dom/media/PeerConnection.js (right):

https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnect...
dom/media/PeerConnection.js:1015:
this._dompc.changeIceConnectionState("completed");
This doesn't look correct. kIceWaiting means that all gathering is done.

I believe this is still "new"

https://firefox-codereview.appspot.com/17001/diff/23001/dom/media/PeerConnect...
dom/media/PeerConnection.js:1031:
this._dompc.changeIceConnectionState("connected");
So we never go to completed? I guess that's because nICEr doesn't...

https://firefox-codereview.appspot.com/17001/diff/23001/media/webrtc/signalin...
File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right):

https://firefox-codereview.appspot.com/17001/diff/23001/media/webrtc/signalin...
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1282:
CSFLogDebug(logTag, "%s: for %p", __FUNCTION__, (void *) this);
Suggest logging the handle rather than the pointer. Though I guess this is
another patch.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 905+:3555e821a5cd+