Closed
Bug 1342579
Opened 7 years ago
Closed 7 years ago
Test to verify we can connect to different certs and ICE credentials
Categories
(Core :: WebRTC, defect, P2)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: drno, Assigned: drno)
Details
Attachments
(1 file)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8841120 [details] Bug 1342579: verify that a single PC can connect to different endpoints. https://reviewboard.mozilla.org/r/115456/#review117578 Just a few suggestions for making this a little cleaner. ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:40 (Diff revision 2) > + if (state == "connected") { > + ok(true, str + " ICE connected"); > + resolve(); > + } else if (state == "failed") { > + ok(false, str + " ICE failed") > + resolve(); The resolve call can be placed outside the if statement. ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:71 (Diff revision 2) > + offer = pc1.localDescription; > + info("Original OFFER: " + JSON.stringify(offer)); > + offer.sdp = sdputils.removeBundle(offer.sdp); > + //info("OFFER w/o BUNDLE: " + JSON.stringify(offer)); > + offerAudio = new RTCSessionDescription(JSON.parse(JSON.stringify(offer))); > + offerAudio.sdp = offerAudio.sdp.replace(/m=video 9/, 'm=video 0'); `.replace('m=video 9', 'm=video 0')` works fine. ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:77 (Diff revision 2) > + //info("offerAudio: " + JSON.stringify(offerAudio)); > + offerVideo = new RTCSessionDescription(JSON.parse(JSON.stringify(offer))); > + offerVideo.sdp = offerVideo.sdp.replace(/m=audio 9/, 'm=audio 0'); > + //info("offerVideo: " + JSON.stringify(offerVideo)); > + }) > + .then(() => pc2.setRemoteDescription(offerVideo)) You can factor this whole block out. ``` var videoAnswer = {}; async function generatePartialAnswer(pc, offer, answer) { await pc.setRemoteDescription(offer); var a = await pc.createAnswer(); await pc.setLocalDescription(a); sdp = pc.localDescription.sdp.split('\r\n'); answer.sessionSection = sdp.slice(...); answer.fingerprint = sdp[ifp]; ... see below ``` ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:81 (Diff revision 2) > + }) > + .then(() => pc2.setRemoteDescription(offerVideo)) > + .then(() => pc2.createAnswer({})) // check that createAnswer accepts arg. > + .then(answer2 => pc2.setLocalDescription(answer2)) > + .then(() => { > + answer2 = pc2.localDescription; ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:83 (Diff revision 2) > + .then(() => pc2.createAnswer({})) // check that createAnswer accepts arg. > + .then(answer2 => pc2.setLocalDescription(answer2)) > + .then(() => { > + answer2 = pc2.localDescription; > + //info("ANSWER2: " + JSON.stringify(answer2)); > + videoFP = answer2.sdp.match(/^a=fingerprint:(.)*$/gm)[0]; If you put the * inside the parentheses, this works better. ::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:85 (Diff revision 2) > + .then(() => { > + answer2 = pc2.localDescription; > + //info("ANSWER2: " + JSON.stringify(answer2)); > + videoFP = answer2.sdp.match(/^a=fingerprint:(.)*$/gm)[0]; > + //info("Answer2 fingerprint: " + videoFP); > + sessionSection = answer2.sdp.match(/^[\s\S]*?m=audio/g)[0].replace('m=audio', '').replace(/a=fingerprint.*\r\n/g, ''); I wonder if this might not be easier if you split the answers into lists of lines and used array functions on them. For instance: ``` a2lines = answer2.sdp.split('\r\n'); const m = a2lines.findIndex(l => l.match(/^m=audio /)); const afp = a2lines.findIndex(l => l.match(/^a=fingerprint/)); audioFP = a2lines[afp]; sessionSection = a2lines.slice(0, m).splice(afp, 1); audioMsection = a2lines.slice(m); ``` Then you can do this at the end: ``` fakeAnswer = sessionSection.concat(audioMsection, [fp], videoMsection, [fp]).join('\r\n'); ```
Attachment #8841120 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841120 [details] Bug 1342579: verify that a single PC can connect to different endpoints. https://reviewboard.mozilla.org/r/115456/#review117578 > The resolve call can be placed outside the if statement. No it can't because I don't want to resolve if e.g. "checking" is reached.
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841120 [details] Bug 1342579: verify that a single PC can connect to different endpoints. https://reviewboard.mozilla.org/r/115456/#review117578 > No it can't because I don't want to resolve if e.g. "checking" is reached. *facepalm*. Of course.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Pushed by drno@ohlmeier.org: https://hg.mozilla.org/integration/autoland/rev/830770d5c2b8 verify that a single PC can connect to different endpoints. r=mt
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/830770d5c2b8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/830770d5c2b8#l2.12 >+<script type="application/javascript;version=1.8"> Please do not add a new versioned JavaScript. Currently ESLint ignores versioned scripts. See bug 1342144 for details.
Flags: needinfo?(drno)
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cedd1aab52c76a8f39bc9bf57a3798c0129353c Bug 1342579: remove js version from threeUnbundledConnections test (really bug 1342144). r=mt
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2cedd1aab52c
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(drno)
Updated•7 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•