Re: [webrtc-pc] {iceRestart: true} works poorly with negotiationneeded (#2167)

>```
> RTCPeerConnection.prototype.restartIce = function() {
>   this.iceRestartNeeded = true;
>   pc.onnegotiationneeded();
> }
> ```

Like @fippo's [above](https://github.com/w3c/webrtc-pc/issues/2167#issuecomment-480880197), this polyfill will throw `InvalidStateError` if called in anything but `"stable"` state. Since the goal of this method is to abstract away knowledge of the signaling state, both of these polyfills fail miserably, proving my point that we need to fix this, and not leave it to JS.

The polyfills also severely underestimate all the corner-cases this method handles. Here are its [WPT tests](https://phabricator.services.mozilla.com/D30978#C975484NL1), which all pass in Firefox's implementation (which I haven't landed yet, as it would upstream tests to WPT prematurely, though I'm happy to do so if there's interest):
* }, "restartIce() returns whether state changed");
* }, "restartIce() fires negotiationneeded");
* }, "restartIce() causes fresh ufrags");
* }, "restartIce() survives remote offer");
* }, "restartIce() is satisfied by remote ICE restart");
* }, "{iceRestart: false} overrides and cancels local restartIce()");
* }, "restartIce() survives rollback");
* }, "restartIce() survives remote offer containing partial restart");

These are all desirable behaviors that are in the PR. They're desirable, because the opposite of any of these behaviors would be surprising, and probably lead to application bugs in real-world situations.

While it may be possible to write a polyfill that does all that, I doubt many would get it right.

> What I dislike about the new API surface is that it seems like it's doing an imperative action ("RestartIce"), while what it's actually doing is (probably) triggering a negotiation request, which will just call back out to user code.

The same can be said about `createDataChannel`, `addTrack`, `removeTrack`, `addTransceiver`, and `direction`. Yet this is the paradigm we've chosen for actions that require negotiation, as @henbos points out [above](https://github.com/w3c/webrtc-pc/issues/2167#issuecomment-481709826). `{iceRestart: true}` is the outlier.

That said, I'm happy to bikeshed. `pc.addIceRestart()`, `pc.restartIce = true;` etc.

> Why should the user call a method to call his own code?

I can appreciate that a working `negotiationneeded` being quite new with Chrome 73, that it might take a while to appreciate the benefits of separating out negotiation as a low-level detail from the rest of one's application logic.

To fully appreciate it, I imagine a hypothetical `RTCPeerConnection` API where all the negotiation methods are removed, `setLocalDescription`, `setRemoteDescription`, `createOffer`, `createAnswer`, `addIceCandidate`, `onicecandidate`, `onnegotiationneeded`, and negotiation is handled automagically for the application. This exercise forces us to consider where in the remaining API to put controls for things the application cares about, like the need to restart ICE.

This isn't merely hypothetical either. It's the premise of [perfect negotiation](https://blog.mozilla.org/webrtc/perfect-negotiation-in-webrtc/), my blog which I've added a new preface to. I highly recommend re-reading it before our upcoming virtual interim next week, as I'll be relying on it heavily. A lightbulb went on, is what I'm trying to share.

-- 
GitHub Notification of comment by jan-ivar
Please view or discuss this issue at https://github.com/w3c/webrtc-pc/issues/2167#issuecomment-500014086 using your GitHub account

Received on Friday, 7 June 2019 19:41:04 UTC