-
Notifications
You must be signed in to change notification settings - Fork 45
Conversation
based on pc1 sample and the following two PRs: webrtc/samples#1009 webrtc/samples#996
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments
audio: true, | ||
video: true | ||
}) | ||
.then(gotStream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove extra indentation of then()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you but eslint does not sadly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:'( saddest story
).then( | ||
onCreateOfferSuccess, | ||
onCreateSessionDescriptionError | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a bunch of helper functions could this be more compactly written as?
try {
let offer = await pc1.createOffer();
await pc1.setLocalDescription(offer);
await pc2.setRemoteDescription(offer);
let answer = pc2.createAnswer();
await pc2.setLocalDescription(answer);
await pc1.setRemoteDescription(answer);
} catch (e) {
trace('Offer/Answer exchange failed: ' + e.message);
}
If more detailed error is desired, more try-catches or a variable keeping track of which stage we're at can be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how comfortable @KaptenJansson feels with es6 and async/await. But I wouldn't want to start the migration here and end up with a weird mix of styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
); | ||
trace(getName(pc) + ' ICE candidate: \n' + (event.candidate ? | ||
event.candidate.candidate : '(null)')); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using async function and await here too?
Ok LGTM still |
based on pc1 sample and the following two PRs:
webrtc/samples#1009
webrtc/samples#996