Skip to content
This repository was archived by the owner on May 13, 2024. It is now read-only.

Conversation

@fippo
Copy link
Contributor

@fippo fippo commented Feb 13, 2018

based on pc1 sample and the following two PRs:
webrtc/samples#1009
webrtc/samples#996

based on pc1 sample and the following two PRs: webrtc/samples#1009 webrtc/samples#996
Copy link
Member

@henbos henbos left a 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)
Copy link
Member

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()

Copy link
Contributor Author

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

Copy link
Member

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
);
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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)'));
}
Copy link
Member

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?

@henbos
Copy link
Member

henbos commented Mar 1, 2018

Ok LGTM still

@henbos henbos merged commit 63a46f7 into webrtc:gh-pages Mar 1, 2018
@fippo fippo deleted the replaceTrack branch March 1, 2018 08:45
@fippo fippo mentioned this pull request Mar 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants