Intent to Deprecate and Remove: RTCPeerConnection.updateIce

[Taylor]

Primary eng email

dead...@chromium.org

Summary

RTCPeerConnection.updateIce is being renamed to setConfiguration for WebRTC standards compliance.

Motivation

The current WebRTC spec includes the renamed method.

Old version with updateIce: http://w3c.github.io/webrtc-pc/archives/20150306/webrtc.html#interface-definition

Current version with setConfiguration: http://w3c.github.io/webrtc-pc//webrtc.html#interface-definition

Compatibility Risk

In Chrome, updateIce is currently unimplemented; it always throws a SyntaxError with a message indicating that the configuration couldn't be applied.

In Firefox, updateIce throws a NotSupportedError with the message "updateIce not yet implemented".

So, we don't expect that any developers are trying to use this method, and the impact of renaming it should be very small.

Alternative implementation suggestion for web developers

Use setConfiguration instead.

Usage information from UseCounter

Haven't yet instrumented.

OWP launch tracking bug

None.

Entry on the feature dashboard

Don't think this is necessary, as the change is pretty small.

Requesting approval to remove too?

Yes.

[TAMURA, Kent]

LGTM1.

> So, we don't expect that any developers are trying to use this method, and the impact of renaming it should be very small.

I think it's safe to remove it without deprecation.

--

TAMURA Kent
Software Engineer, Google

[Chris Harrelson]

LGTM2

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[PhistucK]

Note that this is a change to a prefixed API.

It is not RTCPeerConnection, it is webkitRTCPeerConnection -

(new webkitRTCPeerConnection(null)).updateIce

☆PhistucK

[PhistucK]

Also, using Chrome 45, I cannot find (new webkitRTCPeerConnection(null)).setConfiguration

So, how would a developer use the alternative?

☆PhistucK

[Philip Jägenstedt]

LGTM3. If any call to the method, regardless of the arguments, throws an exception, then removing it entirely without deprecation should be very low risk.

I can't see setConfiguration in any IDL file, but that doesn't really matter if the exiting path always throws.

[Taylor]

Sorry, I should have clarified: the plan is to add setConfiguration at the same time as we remove updateIce. Aside from removing the optional MediaConstraints argument, this is effectively just a renaming.

☆PhistucK

☆PhistucK

LGTM2

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Philip Jägenstedt]

Oh, I see. Just to be very clear, is it really true that updateIce()
always throws an exception? I looked at the implementation and it
isn't obviously so.

>>>>>> an email to blink-dev+...@chromium.org.

>>>>>
>>>>>
>>>>> To unsubscribe from this group and stop receiving emails from it, send

>>>>> an email to blink-dev+...@chromium.org.

>>>>
>>>>
>>>
>>> To unsubscribe from this group and stop receiving emails from it, send an

>>> email to blink-dev+...@chromium.org.

>>
>>
> To unsubscribe from this group and stop receiving emails from it, send an

> email to blink-dev+...@chromium.org.

[PhistucK]

This goes against the policy of improving prefixed APIs...

☆PhistucK

[Philip Jägenstedt]

Taylor, Harald, is unprefixing RTCPeerConnection on the roadmap? The interface in the spec has quite a few differences to Blink's IDL, so it's clearly some work.

[Taylor]

Philip: At least in Chrome, I'm quite confident it always throws an exception. You may be seeing that the blink method only throws an exception if the "handler" returns false. But if you follow the series of calls downwards, the underlying implementation always returns false.

There's another PeerConnection::UpdateIce method that does have an implementation, but it's never called. This is what will become setConfiguration.

As for unprefixing RTCPeerConnection, I'll let Harald answer that.

PhistucK: Can you elaborate? This seems like an improvement to me.

☆PhistucK

>>>>>> an email to blink-dev+unsubscribe@chromium.org.

>>>>>
>>>>>
>>>>> To unsubscribe from this group and stop receiving emails from it, send

>>>>> an email to blink-dev+unsubscribe@chromium.org.

>>>>
>>>>
>>>
>>> To unsubscribe from this group and stop receiving emails from it, send an

>>> email to blink-dev+unsubscribe@chromium.org.

>>
>>
> To unsubscribe from this group and stop receiving emails from it, send an

> email to blink-dev+unsubscribe@chromium.org.

[PhistucK]

This is exactly the point - you intend to improve a prefixed API. The policy is not to improve prefixed APIs. Instead, we should unprefix them (with all of the changes that it entails, of course) and only improve the unprefixed APIs, keeping the prefixed APIs intact and around shortly as appropriate

☆PhistucK

☆PhistucK

>>>>>> an email to blink-dev+...@chromium.org.

>>>>>
>>>>>
>>>>> To unsubscribe from this group and stop receiving emails from it, send

>>>>> an email to blink-dev+...@chromium.org.

>>>>
>>>>
>>>
>>> To unsubscribe from this group and stop receiving emails from it, send an

>>> email to blink-dev+...@chromium.org.

>>
>>
> To unsubscribe from this group and stop receiving emails from it, send an

> email to blink-dev+...@chromium.org.

[Harald Alvestrand]

Unprefixing all the code is definitely on the roadmap.

We've had pushback against adding unprefixed functions that weren't fully spec-conformant in the past (the discussion about mediaDevices.getUserMedia(), which was pushed back behind a flag because its "constraints" argument wasn't spec compliant), so we're not pushing for that right now. We'll move closer to conformance first.

Phistuck, your claimed policy makes no sense to me. If we can't unprefix because the code is nonconformant, and we can't improve because it's prefixed, we'll be stuck forever. That would be stupid.

[Rick Byers]

Incrementally improving a prefixed implementation to be more spec conformant as part of a larger effort to unprefix it makes sense to me.  In the past the scenarios have typically been simple enough that we could just do it all in one go.  But WebRTC is complex enough I doubt that's practical here.  Is there a meta-bug tracking all the different work items necessary to unprefix this?

The alternative is to build a (partly or wholly) separate unprefixed implementation in parallel with leaving the prefixed one unmodified.  That shouldn't be completely out of the question, but I doubt it's worth the extra implementation challenges involved.

Rick

[PhistucK]

You can find various intent threads (and others) that tried (or discussed) improving prefixed features and got pushbacks because of that. My "claimed" policy makes sense to the API owners. It may sound stupid to you, but prefixed features are harmful to the web and Blink has been officially moving away from them since its forking.

Regarding your view of the unprefixing and improving plan, here is the sensible plan (yours does sound a bit backwards due to missing pieces).

1. Implement the unprefixed API behind the experimental flag until it is mostly complete and specification compliant.

2. Enable it by default once it is ready and deprecate (and eventually remove, sooner rather than later, the prefixed API).

A good example of such an elaborate unprefixing, implementation and deprecation plan (and execution) is the Encrypted Media Extensions API.

☆PhistucK

[Rick Byers]

I think we can differentiate between adding new features and improving the standards conformance of existing features.  I have no interest in seeing new features added to prefixed APIs - those features should be blocked on shipping the unprefixed API.  But in this case we're talking about making a breaking change to get an existing feature of the prefixed API closer to the spec'd behavior.  I don't see any problem with that personally, as long as it's really part of a serious effort to invest in interoperability and unprefixing.

Other API owners may disagree though.

Rick

[Philip Jägenstedt]

It's not obvious to me how to weigh a case like this. I've been making changes to the prefixed fullscreen API to bring it closer to what is in the spec before unprefixing. On the other hand I didn't want -webkit-text-emphasis-position to be extended, but in that case the unprefixing was happening at the same time.

webkitRTCPeerConnection is dissimilar enough from the spec's RTCPeerConnection that treating them as aliases will not work, so it will be necessary to have both during at least a transition period. At a glance, it looks like the most straightforward way would be to have both as unrelated interfaces in the IDL, but have one of the classes inherit from the other in C++, implementing one in terms of the other, whichever is the least work.

Taylor, given that updateIce() always throws, how important is it to get this functionality working under a different API? If this is mostly about taking small steps towards unprefixing, then perhaps removing updateIce() and implementing setConfiguration() on a new unprefixed RTCPeerConnection interface behind a flag would be an option? (The rest of RTCPeerConnection could be left as TODOs.)

Simply shipping webkitRTCPeerConnection.setConfiguration() is definitely an option if the broken updateIce() is a real-world problem and unprefixing is too far in the future.

[Harald Alvestrand]

Guesstimates of timeframes:

If we apply "conformance to spec" as the criterion for unprefixing, we're looking at late 2016 or 2017. There are functions that require significant refactoring of underlying infrastructure before we can implement them properly.

If we apply "all functions implemented conform to spec" as the criterion, we're looking at early 2016.

We had a discussion yesterday about the various paths forward; the idea of maintaining two different objects with different (but overlapping) semantics (and different bugs) seemed singularly unappealing to us.

[Philip Jägenstedt]

On Thu, Sep 24, 2015 at 2:48 PM, Harald Alvestrand <h...@google.com> wrote:

We had a discussion yesterday about the various paths forward; the idea of maintaining two different objects with different (but overlapping) semantics (and different bugs) seemed singularly unappealing to us.

It sure is unappealing, but the prefixed API uses callback pairs while the spec uses promises, the constructor arguments differ, etc. If separate interfaces is not the way to go, what is the path forward?

Philip

[Rick Byers]

On Thu, Sep 24, 2015 at 8:48 AM, Harald Alvestrand <h...@google.com> wrote:

Guesstimates of timeframes:

If we apply "conformance to spec" as the criterion for unprefixing, we're looking at late 2016 or 2017. There are functions that require significant refactoring of underlying infrastructure before we can implement them properly.

If we apply "all functions implemented conform to spec" as the criterion, we're looking at early 2016.

It's a bit of an aside for this thread, but I think the choice above is worth discussing.

If you look at other recent examples (like fetch), I think we generally prefer shipping new APIs incrementally as they're ready over blocking waiting for an implementation to be "spec complete".  In particular, I think we should prefer shipping unprefixed APIs function-by-function when three criteria are met:

1. The shipped functions have substantial value on their own (no point shipping something that can't really be used due to missing pieces)
   
2. It's possible to feature-detect for the presence/absence of the missing pieces
   
3. There aren't known substantial compatibility issues with existing content that expects the presence of one API to imply the presence of another.  In some cases, if we think this may be likely, we may want to gather data (experiment, search httparchive, etc.) to try to estimate the extent of the issue.
   

In some cases we've intentionally shipped some portions of a spec without any intention of shipping other portions (eg. when we shipped touch-action without any plan to ship the rest of pointer events).  So I definitely don't consider there to be any "all or nothing" requirement.

[Harald Alvestrand]

Actually both the spec and the shipping API have the callback functions, but one of them marks them as "for backwards compatibility only".

If we don't want separate interfaces, the alternative is to change the present interface (offering polyfills in adapter.js or backwards-compatible overloads in Blink as appropriate), and when we think the time is appropriate, remove the prefix.

[Philip Jägenstedt]

Am I looking at the wrong spec, perhaps? In https://w3c.github.io/webrtc-pc/#interface-definition createOffer() takes only an options dictionary, while the createOffer() in Blink's IDL takes a success and failure callback, as well as the options dictionary. It doesn't look like those two could co-exist on the same interface.

[PhistucK]

As far as I know, navigator.getUserMedia is specified to be void getUserMedia(options, callback, callback) as well as Promise getUserMedia(options). I think this is already supported behind the experimental flag.

It does seem weird and never liked it, though.

I wonder whether Edge implemented both of the signatures as well.

☆PhistucK

[Philip Jägenstedt]

OK, but the question was about RTCPeerConnection.createOffer(), the unprefixing challenges for getUserMedia() could be entirely different.

[PhistucK]

Sorry, I thought you meant there is no precedent for Promise and non Promise based method on the same interface.

☆PhistucK

[Harald Alvestrand]

The spec specifies both, in two partial interfaces (far apart in the text). Webidl says we cannot mix promise-returning and non-promise-returning in an overload. Firefox solves this by returning a never-resolved promise from the callback-based function. We have not tried to resolve it yet. But obviously there is a spec issue here as well.

[Philip Jägenstedt]

To find all of the differences between spec and implementation, I gave RTCPeerConnection the "IDL spec sync" treatment: https://codereview.chromium.org/1375533002/

There are quite a lot of differences, some of which I suspect will require spec changes, but overall it looks feasible to converge with the spec and then unprefix. (I was too pessimistic.) Separate interfaces, on the other hand, has the risk of becoming permanent if content comes to depend on the differences.

Therefore, LGTM to replace updateIce() with setConfiguration()!

P.S. When doing changes like this, please be pedantic about using [TypeChecking=Interface] and the correct dictionary types, there's a lot of custom dictionary handling here that could potentially hide small deviations from the per-spec behavior.

[Harald Alvestrand]

Philip,

can you take a look at web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection.idl and see if it covers the same grounds as the tests you did for this?

I'd like to make sure we get the correct behavior when we fix this.

I'm working (inbetween everything else) on getting constraints to be real dictionaries, but have hit a couple of snags - the outstanding one just now is that you can't have an union that includes a dictionary as one of the alternatives.

[Philip Jägenstedt]

That test uses idlharness.js, which is pretty great and covers many cases one is likely to overlook. However, it's not complete, for example making createDTMFSender always return null (never throwing exceptions) doesn't cause the test to fail. I had a chat with Ms2ger and it sounds like this is hard to solve in the general case, so one can't lean entirely on these tests.

What I would do is to paste the spec's IDL for a method and add [TypeChecking=Interface], then check the changes to the generated code. Not to see if it's correct, it probably is, but to see if there's some case worth submitting a test to web-platform-tests for.

As for unions with a dictionary type, where are those needed?

[Harald Alvestrand]

Until we go to provable code, there's no such thing as a complete test :-)

What does the [TypeChecking=Interface] modifier do?

I hope we get many more tests in web-platform-tests after a while - it's the only way to make sure we have tests that are run on both Firefox and Chrome.

WRT unions with dictionary:

dictionary MediaStreamConstraints {

             (boolean or MediaTrackConstraints) video = false;

             (boolean or MediaTrackConstraints) audio = false;

};

The bug to track is https://code.google.com/p/chromium/issues/detail?id=524424

[Philip Jägenstedt]

On Tue, Sep 29, 2015 at 11:16 AM, Harald Alvestrand <h...@google.com> wrote:

Until we go to provable code, there's no such thing as a complete test :-)

What does the [TypeChecking=Interface] modifier do?

I think you can find all of the differences by searching for "has_type_checking_interface" in third_party/WebKit/Source/bindings/templates/*.cpp. Basically, per WebIDL one should throw TypeError if the type isn't correct, but originally any invalid type would instead cause null to be passed to the implementation. For nullable types it would then be impossible to discern null from invalid types, so you couldn't throw exceptions where per WebIDL you should. Even for non-nullable types it becomes more likely that the manual error handling isn't quite right.

This is all rather corner-casey and [TypeChecking=Interface] should just be the default so that nobody has to know about it, but until it is it's yet another thing to keep in mind :(

 

I hope we get many more tests in web-platform-tests after a while - it's the only way to make sure we have tests that are run on both Firefox and Chrome.

WRT unions with dictionary:

dictionary MediaStreamConstraints {

             (boolean or MediaTrackConstraints) video = false;

             (boolean or MediaTrackConstraints) audio = false;

};

The bug to track is https://code.google.com/p/chromium/issues/detail?id=524424

Ah, that's precisely like a propsed addEventListener() change recently discussed:

https://github.com/RByers/EventListenerOptions/issues/20#issuecomment-143389372

Cameron says it's not a problem, so it should be good spec-wise, and it looks like Blink's bindings generator can handle it.

Philip