step-06 make Firefox compatible #33
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g.
|
|
2017-06-15 10:17 GMT+08:00 googlebot <[email protected]>:
I signed it!
I signed it!
…--
Best Regards,
Luke Lai
|
|
CLAs look good, thanks! |
|
@limhi Thanks a lot for the work and sorry for the delay in the response. I was wondering if this is trying to fix a bug on the codelab. If yes, it would be nice to document it to know why these changes are made. There's currently 56 (not linking on purpose) which is exposing some x-browser issues, so perhaps it is related, but I wanted to verify with you what you were trying to fix. Thank you! |
|
@nitobuendia https://developer.mozilla.org/zh-TW/docs/Web/API/RTCIceCandidate Thanks! |
|
Thanks a lot for the clarifications, @limhi and sorry for the delay here. I am going to add my notes on the code itself :) |
There was a problem hiding this comment.
@limhi Thanks a lot for the PR and the waiting time.
I have tested this and it seems to work fine. I would make some small adjustments. Adding comments on the file below.
//cc @samdutton
| @@ -160,8 +160,10 @@ function signalingMessageCallback(message) { | |||
|
|
|||
| } else if (message.type === 'candidate') { | |||
| peerConn.addIceCandidate(new RTCIceCandidate({ | |||
| sdpMLineIndex: message.label, | |||
There was a problem hiding this comment.
nit: Order properties alphabetically.
candidate: message.candidate,
sdpMid: message.id,
sdpMLineIndex: message.label,
| candidate: message.candidate | ||
| })); | ||
| }));// Firefox compatible |
There was a problem hiding this comment.
No need to add "Firefox compatible". We want to make it compatible with all browsers.
| @@ -160,8 +160,10 @@ function signalingMessageCallback(message) { | |||
|
|
|||
| } else if (message.type === 'candidate') { | |||
| peerConn.addIceCandidate(new RTCIceCandidate({ | |||
There was a problem hiding this comment.
This same logic is present in step-05/js/main.js -- could you add the change there as well?
|
Hello @limhi , Thanks a lot again for your contribution. I sent a couple of comments to make this PR better. Let me know when you have some time to have a look. Thanks a lot! |
|
Hey @limhi -- Thanks again for your work. Did you have a chance to see the comments I sent on your PR? |
|
Hey I've just found this was why the example I was trying out was flaky, any chance of a merge? I could implement @nitobuendia 's comments and make a new PR if that would help. |
|
@platy It's been almost 6mo with no activity. I think that's a fair thing. Happy to review it, but it would need to be approved by @samdutton |
|
Approved #92. @nitobuendia — if you're happy with that, feel free to merge. |
No description provided.