Skip to content

add url attribute to RTCIceCandidate - #2773

Merged
alvestrand merged 2 commits into
w3c:mainfrom
fippo:move-url
Oct 27, 2022
Merged

add url attribute to RTCIceCandidate#2773
alvestrand merged 2 commits into
w3c:mainfrom
fippo:move-url

Conversation

@fippo

@fippo fippo commented Sep 22, 2022

Copy link
Copy Markdown
Contributor

aligning with how this is defined in webrtc-stats
https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats-url
and add deprecation note to the url attribute on the RTCIceEvent

@youennf since you mentioned this was implemented in Safari, can you please check if it actually shows the correct value? Given the trouble I had reconstructing the url in the lowest layers I would not be surprised if this was the empty strings.


Preview | Diff

@fippo

fippo commented Sep 23, 2022

Copy link
Copy Markdown
Contributor Author

looks like Safari is actually setting a value but returning IP addresses when asked for stun servers with a hostname. That doesn't look like anyone is actually using this so this should still be safe to remove.

@alvestrand
alvestrand requested a review from youennf October 6, 2022 14:42
@alvestrand

Copy link
Copy Markdown
Contributor

Needs a bug filed, or reference to the meeting where we agreed to it.

@youennf

youennf commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

Since Safari is implementing it, I would prefer we keep the current API and add it to candidate. Plus add a note that we plan to deprecate the event url.
This will leave some time for Safari to try doing the migration, at which point, if it succeeds, we can remove it altogether.

@alvestrand

Copy link
Copy Markdown
Contributor

Note - I checked the test for RTCPeerConnectionIceEvent-constructor, and it does check that you can construct an RTCPeerConnectionIceEvent with an URL argument, but never checks that the resulting event has an URL in it.

Labelling "test needed" ....

@alvestrand alvestrand added the Needs Test Needs a WPT test label Oct 6, 2022
@fippo

fippo commented Oct 12, 2022

Copy link
Copy Markdown
Contributor Author

Safari still implementing something that was removed isn't a big problem, no (unless we add a negative test)? Nobody cares about chrome still implementing addStream or RTCIceServer.url.
I can help adding a polyfill to adapter if needed

@alvestrand

Copy link
Copy Markdown
Contributor

Seems worth doing a 2-step approach.

@jan-ivar

Copy link
Copy Markdown
Member

@fippo can you update this PR to do a two-stage approach?

@youennf

youennf commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

Discussed at today's editor meeting.
@fippo, are you ok with the two step approach?
If not, we probably need more discussion at a next WebRTC interim.

@fippo fippo changed the title move url from RTCIceEvent to the RTCIceCandidate add url attribute to RTCIceCandidate Oct 25, 2022
@fippo
fippo force-pushed the move-url branch 2 times, most recently from c2a93e7 to 6d19262 Compare October 25, 2022 13:02
@fippo

fippo commented Oct 25, 2022

Copy link
Copy Markdown
Contributor Author

done. I think I got the amendment right (as a separate commit)

@alvestrand

Copy link
Copy Markdown
Contributor

apparently the generation is failing because the framesDropped error hasn't been fixed yet.

fippo added 2 commits October 26, 2022 20:49
aligning with how this is defined in webrtc-stats
  https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats-url

and add deprecation note to the url attribute on the RTCIceEvent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Test Needs a WPT test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants