w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
433 stars 115 forks source link

Convert RTCIceCandidatePair dictionary to an interface #2961

Closed sam-vi closed 1 month ago

sam-vi commented 2 months ago

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

alvestrand commented 2 months ago

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

dontcallmedom commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@dontcallmedom help?

dontcallmedom commented 1 month ago

this is the kind of PR that updates the IDL fragments on which the idl harness relies: https://github.com/web-platform-tests/wpt/pull/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)

saschanaz commented 1 week ago

Have you filed implementation bugs for this?

sam-vi commented 2 days ago

Have you filed implementation bugs for this?

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

saschanaz commented 2 days ago

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 commented 2 days ago

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