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

Convert RTCIceCandidatePair dictionary to an interface #2961

Merged
merged 4 commits into from
May 30, 2024

Conversation

sam-vi
Copy link
Contributor

@sam-vi sam-vi commented Apr 18, 2024

Fixes #2930.

Change the RTCIceCandidatePair dictionary to an interface and change the required dictionary members to readonly attributes backed by internal slots.

No constructor is described in the IDL to prevent an application from constructing arbitrary pairs. Only the user agent can construct valid RTCIceCandidatePair in response to the ICE agent forming candidate pairs, and to represent the selected candidate pair.


Preview | Diff

Addresses: w3c#2930.

This commit changes the dictionary to an interface and required
dictionary members to readonly attributes backed by internal slots.

No constructor is described in the IDL, since RTCIceCandidatePair is
only created by the user agent as a result of the ICE agent forming an
ICE candidate pair. The application cannot construct arbitrary candidate
pairs.
@alvestrand
Copy link
Contributor

Since it's an observable change, it needs an amendments.json update.
@dontcallmedom may need to help.

@dontcallmedom
Copy link
Member

I have added the basic amendment description, but the CI will fail (as designed) until a test update can be referenced.

It may be that for this particular change, it would be enough to test it via the idl_harness change, which itself will be done automatically once this pull request gets merged (since WPT will automatically get the updated IDL extracted from the spec). If that is so, it would be appropriate to merge as is, and file an issue to update the amendment once the WPT update lands (which given the various intermediary steps involved would probably take a few days)

@sam-vi
Copy link
Contributor Author

sam-vi commented May 15, 2024

So if I understand correctly, if the test via idl_harness change is deemed sufficient, then there is no further action right now, correct?

Once the IDL is updated in WPT, the new rtcicecandidatepair amendment should be updated with

"tests": [
  "webrtc/idlharness.https.window.html"
],
"testUpdates": [
  "web-platform-tests/wpt#___"
],

In that case, could the PR be considered at the next editors' call?

@alvestrand
Copy link
Contributor

I'm not sure how to navigate the update of the WPT idlharness test to get a number to put into this PR.
May need a followup.

@alvestrand
Copy link
Contributor

@dontcallmedom help?

@dontcallmedom
Copy link
Member

this is the kind of PR that updates the IDL fragments on which the idl harness relies: web-platform-tests/wpt#46056

again, the WPT PR would only get generated some days after this WebRTC PR gets merged, so let's not block on it (but instead, let's file an issue when this PR gets merged)

@alvestrand alvestrand merged commit 39360d4 into w3c:main May 30, 2024
2 of 4 checks passed
@sam-vi sam-vi deleted the 2930-candidatepair-interface branch June 5, 2024 12:40
dontcallmedom added a commit that referenced this pull request Jun 7, 2024
@saschanaz
Copy link
Member

Have you filed implementation bugs for this?

@sam-vi
Copy link
Contributor Author

sam-vi commented Jul 3, 2024

Have you filed implementation bugs for this?

Filed https://issues.chromium.org/issues/350927786 for this.

@saschanaz
Copy link
Member

cc @jan-ivar, I think W3C in general needs WHATWG-like PR template to make sure bugs are filed for all browser engines.

@jan-ivar
Copy link
Member

jan-ivar commented Jul 3, 2024

Firefox does not implement getSelectedCandidatePair() yet so this is already covered by bug 1307994.

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.

Consider making RTCIceCandidatePair an interface
5 participants