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

icecandidate_event update the meaning of null candidate #2450

Closed
1 of 2 tasks
bardiharborow opened this issue Nov 24, 2019 · 7 comments · Fixed by #27392
Closed
1 of 2 tasks

icecandidate_event update the meaning of null candidate #2450

bardiharborow opened this issue Nov 24, 2019 · 7 comments · Fixed by #27392
Labels
area: WebRTC Needs help from someone with WebRTC domain knowledge Content:WebAPI Web API docs effort: medium This task is a medium effort. help wanted If you know something about this topic, we would love your help! p2 We want to address this but may have other higher priority items.

Comments

@bardiharborow
Copy link

Request type

  • New documentation
  • Correction or update

Details

See icecandidate event.

The code sample in "Sharing a new candidate" checks if event.candidate is truthy before delivering it to the remote peer. "Indicating the end of a generation of candidates" says that events where event.candidate == "" should be delivered, but the empty string is falsy in JavaScript and this signal would not be delivered. "Indicating that ICE gathering is complete" says that events where event.complete == null should not be delivered but then refers to the event.candidate == true test.

There is clearly a nuance here that I'm missing, but in any case this may need to be clarified. An uninformed reader would form the conclusion that actually a event.complete != null test is needed.

There's also mixed references to RTCPeerConnectionIceEvent (bluelinked) and RTCPeerIceCandidateEvent (redlinked), not sure if that's intentional.

@a2sheppy
Copy link
Contributor

a2sheppy commented Jan 7, 2020

Yeah, the meaning of a null candidate changed a while back and the document clearly has not fully caught up. That needs to be cleared up. And yes, the references to RTCPeerIceCanddiateEvent are wrong and should be RTCPeerConnectionIceEvent.

@bardiharborow
Copy link
Author

@chrisdavidmills the main issue here doesn't appear to have been resolved, though the incorrect reference to RTCPeerIceCanddiateEvent seems to be gone.

@chrisdavidmills
Copy link
Contributor

@bardiharborow OK, thanks for the heads up. I'll reopen this and move it to our current content repo.

@chrisdavidmills chrisdavidmills transferred this issue from mdn/sprints Feb 18, 2021
@Rumyra
Copy link
Collaborator

Rumyra commented Mar 15, 2021

To whoever fixes this issue, it looks like the following is needed:

  • Check the meaning of null candidate and update examples accordingly
  • Update any text to reflect changes

@Rumyra Rumyra added effort: medium This task is a medium effort. help wanted If you know something about this topic, we would love your help! p2 We want to address this but may have other higher priority items. labels Mar 15, 2021
@github-actions github-actions bot added the idle label Nov 24, 2021
@teoli2003 teoli2003 reopened this May 29, 2022
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 29, 2022
@sideshowbarker sideshowbarker removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 30, 2022
@sideshowbarker
Copy link
Collaborator

@dontcallmedom This seems like something that a contributor with WebRTC domain knowledge could make short work of. Do you think there’s any chance we could rope in somebody to take a look at it?

@sideshowbarker sideshowbarker added the area: WebRTC Needs help from someone with WebRTC domain knowledge label Jun 25, 2022
@fippo
Copy link
Contributor

fippo commented Jul 12, 2022

the recommendation to only signal non-null candidates is a pretty old one (2013?) which predates the icegatheringstatechange event. You should actually signal when you have gathered all candidates so the other side knows.

"Indicating the end of a generation of candidates" says that events where event.candidate == "" should be delivered,

This part actually works as intended since that candidate has event.candidate set to {candidate: "", sdpMid: ...}
Tagging @docfaraday who knows most about that end-of-candidates candidate but I might take this one

@Rumyra Rumyra changed the title /en-US/docs/Web/API/RTCPeerConnection/icecandidate_event icecandidate_event update the meaning of null candidate Aug 19, 2022
@Rumyra
Copy link
Collaborator

Rumyra commented Aug 19, 2022

@danjenkins would you happen to be able to help with the above issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: WebRTC Needs help from someone with WebRTC domain knowledge Content:WebAPI Web API docs effort: medium This task is a medium effort. help wanted If you know something about this topic, we would love your help! p2 We want to address this but may have other higher priority items.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants