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

Mentioning the end-of-candidates indication in a couple sections. #254

Merged
merged 3 commits into from Mar 21, 2016

Conversation

taylor-b
Copy link
Collaborator

Specifically, "addIceCandidate" and "Applying a Remote Description".

Specifically, "addIceCandidate" and "Applying a Remote Description".
@@ -1243,6 +1243,10 @@ candidate:1 1 UDP 1694498815 192.0.2.33 10000 typ host
and/or pending remote description according to the rules defined for
Trickle ICE. Connectivity checks will be sent to the new candidate.</t>

<t>This method can also be used to provide an end-of-candidates
indication to the ICE Agent, as defined in
<xref target="I-D.ietf-ice-trickle"/>.</t>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be more specific about it being the null value that mean end-of-candidates?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this kind of redundant because the ICE agent on side A automatically produces a null candidate, so if you just proxy stuff from A to B, you get what you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might also want to mention that this applies to all m= sections.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pthatcherg I wasn't sure if JSEP was supposed to mention specifics about the API. But I'll change it and see if anyone objects.

@ekr Redundant because "candidate" from the above paragraph can include a null candidate? I assume the meaning of a null candidate needs to be defined here though, since it's not defined in Trickle ICE.

@juberti Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ekr, I think this is worth spelling out since it wasn't always this way, at least not in the w3c spec.

@taylor-b / @pthatcherg not sure that we should specify null, since that's a bit of a w3c-ism; rather, I think the generic notion that we can supply end-of-candidates here should be sufficient. This would match the advice given in https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-13#section-3.4.1, where it simply says that an event is generated to indicate end-of-candidates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@juberti That's what I thought initially. I changed it back.

@fluffy
Copy link
Contributor

fluffy commented Mar 18, 2016

I'm fine this however it ends up. I will r+ this and people can merge it once it is done. Thanks

@fluffy
Copy link
Contributor

fluffy commented Mar 18, 2016

Just adding a still LGTM

@juberti
Copy link
Contributor

juberti commented Mar 21, 2016

lgtm once my comment from today is addressed

juberti added a commit that referenced this pull request Mar 21, 2016
Mentioning the end-of-candidates indication in a couple sections.
@juberti juberti merged commit 1a9b326 into rtcweb-wg:master Mar 21, 2016
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.

None yet

5 participants