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

Adding additional members to RTCIceCandidate dictionary. #325

Merged
merged 3 commits into from Oct 15, 2015

Conversation

Projects
None yet
4 participants
@taylor-b
Copy link
Contributor

taylor-b commented Oct 9, 2015

These attributes aren't used in addIceCandidate; they're just provided
in candidates signaled by onicecandidate, or retrieved from an
RTCIceCandidatePair. This will allow interested applications to get more
information about an ICE candidate, without parsing the candidate
string.

This is the change Peter suggested, which we agreed to at the Redmond f2f.

Adding additional members to RTCIceCandidate dictionary.
These attributes aren't used in addIceCandidate; they're just provided
in candidates signaled by onicecandidate, or retrieved from an
RTCIceCandidatePair. This will allow interested applications to get more
information about an ICE candidate, without parsing the candidate
string.
webrtc.html Outdated
@@ -1271,7 +1271,9 @@
<code>none</code>. This call will result in a change to the
connection state of the ICE Agent, and may result in a change to
media state if it results in different connectivity being
established.</p>
established. Note that the only members of the candidate attribute

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

I think you can remove the "note that" and just say "The only ...

webrtc.html Outdated
TCP candidates and only <code>active</code> TCP candidates. Servers and other
endpoints MAY gather <code>active</code>, <code>passive</code> or <code>so</code>
candidates.</p>

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

I don't think we need the text about gathering in this part of the spec.

This comment has been minimized.

Copy link
@taylor-b

taylor-b Oct 9, 2015

Author Contributor

Where should it go then? Or is this explained in another spec that can simply be referenced?

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

I'm guessing it's in JSEP or is a TODO of JSEP. In either case, it wouldn't be appropriate in this PR since this PR is about providing more information, not about defining behavior.


<dt>relay</dt>

<dd>A relay candidate.</dd>

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

This should reference [[!ICE]].

established.</p>
established. The only members of the candidate attribute
used by this method are <var>candidate</var>, <var>sdpMid</var> and
<var>sdpMLineIndex</var>; the rest are ignored.</p>

This comment has been minimized.

Copy link
@alvestrand

alvestrand Oct 9, 2015

Contributor

If sdpMid is given and sdpMLineIndex is not, what will happen?

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

The same thing as before this PR, I think.

<dt>unsigned long priority</dt>

<dd>The assigned priority of the candidate. This is automatically
populated by the browser.</dd>

This comment has been minimized.

Copy link
@alvestrand

alvestrand Oct 9, 2015

Contributor

Didn't we agree that all the attributes are read-only? I don't want anything to give the impression that modifying these values makes any sense at all.

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 9, 2015

Contributor

The only place where this has any effect on behavior is in addIceCandidate, and that says it ignores these values.

This comment has been minimized.

Copy link
@martinthomson

martinthomson Oct 10, 2015

Member

If addIceCandidate() ignores these values, then they don't need to be added here. They can be added as attributes on a new dictionary that inherits from RTCIceCandidate, a new type that would be an argument to onicecandidate events only. That avoids the implication that these parameters are required, or that they might need to be consistent with the main candidate string.

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 12, 2015

Contributor

I like that idea. What would you call this new type?

This comment has been minimized.

Copy link
@martinthomson

This comment has been minimized.

Copy link
@pthatcherg

pthatcherg Oct 13, 2015

Contributor

Then I propose we make RTCIceCandidate the "full" one coming from onicecandidate and RTCSignalledIceCandidate the "signalled" one that goes into addIceCandidate.

But if anyone can come up with a better pair of names, I'd welcome them.

alvestrand added a commit that referenced this pull request Oct 15, 2015

Merge pull request #325 from taylor-b/ice_candidate_attributes
Adding additional members to RTCIceCandidate dictionary.

@alvestrand alvestrand merged commit fafb770 into w3c:master Oct 15, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ipr PR deemed acceptable.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.