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

Add RTCIceTransportState Enum #533

Closed
wants to merge 5 commits into from
Closed

Conversation

aboba
Copy link
Contributor

@aboba aboba commented Mar 3, 2016

Add definitions for RTCIceTransport.state

Relates to Issue #457

Add definitions for RTCIceTransport.state
Update the indentation.
<dt>connected</dt>
<dd>
<p>
The <code><a>RTCIceTransport</a></code> has received found a usable connection

Choose a reason for hiding this comment

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

"has found"

Fixed a typo.
Add statement that a consent check failure in "completed" causes a transition to "failed".
Added statement that a consent check failure in "connected" causes a transition to "disconnected".
@aboba
Copy link
Contributor Author

aboba commented Mar 10, 2016

@nils-ohlmeier Added clarifications responding to your comments.

@elagerway
Copy link
Contributor

@taylor-b would mind taking a look and providing some comment here?

@taylor-b
Copy link
Contributor

I just made PR #557 which does what I talked about in the virtual interim: move the ICE state definitions into the RTCIceTransport section, and define RTCIceConnectionState in the same fashion that RTCPeerConnectionState is defined.

Could we try to merge that PR before merging this one? That way, when viewing the diff for this one, it will be clear how the ICE state definitions are changing. My PR doesn't change the definitions, it just moves them around.

@aboba
Copy link
Contributor Author

aboba commented Mar 17, 2016

@taylor-b PR #557 covers everything in this PR other than the handling of ICE consent failure. So it makes sense to merge 557 first and then just focus on the additional consent-related text.

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

Successfully merging this pull request may close these issues.

4 participants