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

fix rtcicecandidate url description #368

Merged
merged 2 commits into from
Oct 11, 2018

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Sep 12, 2018

also restrict it to local candidates

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

lgtm with nits.

webrtc-stats.html Show resolved Hide resolved
<code>RTCPeerConnectionIceEvent</code>.
For local candidates this is the URL of the ICE server from
which the candidate was obtained. It is the same as the
<code>url</code> surfaced in the
Copy link
Member

Choose a reason for hiding this comment

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

Please link to http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnectioniceevent-url otherwise we get a markup loop.

also restrict it to local candidates
<a href="https://w3c.github.io/webrtc-pc/#rtcpeerconnectioniceevent">url surfaced in the RTCPeerConnectionIceEvent</a>.
</p>
<p>
For remote candidates, this property is not applicable.
Copy link
Member

@jan-ivar jan-ivar Sep 12, 2018

Choose a reason for hiding this comment

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

Can we s/applicable/present/ ? Present has specific meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change in both places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hrm:

When a feature is not applicable to an instance of an object (for example audioLevel
          on a video stream), omit the dictionary member. Do NOT report a count of zero, -1 or
          "empty string".

We use "applicable" four times (up from 3).
We do not use it for things like pliCount.

Keep as is an file an issue to be more consistent?

@fippo
Copy link
Contributor Author

fippo commented Sep 12, 2018

changed "not applicable" which is not used in many places. The usage of "applicable" is not consistent anyway, see e.g. pliCount

@vr000m
Copy link
Contributor

vr000m commented Sep 19, 2018

I will open another issue for not applicable/not present. Otherwise LGTM.

@alvestrand alvestrand merged commit 613ec19 into w3c:master Oct 11, 2018
@fippo fippo deleted the the-that-what-the-heck branch October 11, 2018 15:20
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

4 participants