Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove multiple ready events. #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

druuu
Copy link

@druuu druuu commented Jun 15, 2018

fixes #87
fixes #85

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@druuu
Copy link
Author

druuu commented Jun 15, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor

@nitobuendia nitobuendia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@druuu Thanks a lot for sending the PR. It looks good to me. I have made just one comment to understand your change.

@samdutton I have tested the changes and they seem to work. There are some issues related that you can read on the "Conversation" of the PR.

I was not able to reproduce the issues on the bugs though. However, the changes on index.js do make sense in theory to me.

@@ -62,7 +62,6 @@ socket.on('created', function(room, clientId) {
socket.on('joined', function(room, clientId) {
console.log('This peer has joined room', room, 'with client ID', clientId);
isInitiator = false;
createPeerConnection(isInitiator, configuration);
Copy link
Contributor

@nitobuendia nitobuendia Jun 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@druuu Was this causing issues too or what's the logic to change it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: It works; just want to be sure we are not changing more than we should.

Copy link
Author

@druuu druuu Jun 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitobuendia
the changes are made because it looks a bit confusing, here we go:

before the changes:
user1 joins, 'isinitiator' is set to true for user1.
user2 joins, 'isinitiator' is set to false for user2 and also peerconnection is established for user2
user2 sends READY event, now user1 peerconnection is created

After the changes:
user1 joins, 'isinitiator' is set to true for user1.
user2 joins, 'isinitiator' is set to false for user2.
user2 sends READY event, peerconnection is created for both user1 and user2.

So, it looks more simple in the second case.
If you dont want these changes, i can send you another PR, or you can just remove https://github.com/googlecodelabs/webrtc-web/blob/master/step-06/index.js#L45

thank you for these tutorials, they are really helpful and simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

step-06, OperationError: Failed to set local offer sdp send "ready" twice in step-06
3 participants