[Webkit-unassigned] [Bug 150166] WebRTC: Update RTCPeerConnection API and introduce PeerConnectionBackend

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150166

youenn fablet <youennf at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|FIXED                       |---
     Ever confirmed|1                           |0

--- Comment #38 from youenn fablet <youennf at gmail.com> ---
Nice to see this patch landed.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:36
> > > +
> > 
> > For all these functions, we should somehow check that "this" has the right
> > type.
> > We should work on that in the future.
> 
> Would it be sufficient to do @isObject() and check for one of the [Private]
> function symbols? For example @queuedCreateOffer? Perhaps we can do this is
> a follow-up patch?

Checking that @queuedCreateOffer exists currently means that the object prototype is not modified by user scripts.
I think C++ implemented APIs work on instances even if their prototypes are changed.
I am not sure whether we should move these private functions to instance slots or not.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:74
> > > +    if (arguments.length == 1) {
> > 
> > It would be good to have a test for that behavior (one arg -> promise, more
> > than one arg, legacy callbacks).
> > It may happen that future patches break that behavior without noticing it.
> 
> We can check the return value to see which version was called. I can make a
> bug on it so it's not forgotten.

Sounds good, especially if it ends up being part of WebRTC W3C test suite.
This would ensure every browser has the same understanding.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:118
> > > +    peerConnection. at privateGetStats(selector).then(successCallback, errorCallback);
> > 
> > You might want to check whether the direct use of then is appropriate.
> > A user script may modify Promise prototype and its then function.
> > @Promise.prototype. at then.@call is (unfortunately less readable but) safer.
> 
> This is indeed a problem. However I suspect using
> @Promise.prototype.then. at call only helps if the user has modifies then() on
> an instance, not if the actual prototype function is modified. And since the
> bindings only calls then() on internal promise instances created inside the
> binding, using the prototype-call only complicates syntax without any real
> benefit. We should have a general solution for this.

A user script may decide to change the Promise prototype, in which case all promises instances will be affected, hence the unsafe use of then.

We could use InternalPromise for private methods.
But we need to ensure that we never expose any InternalPromise instance to user scripts.

Agreed on the need for a better more general solution.

> 
> > > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:32
> > > +// @internal
> > 
> > How do you expose these internal functions to JS built-ins?
> > I would expect changes within JSDOMWindowBase::finishCreation to do that
> > explicitly.
>  
> The plan is to have that done in a follow-up patch. The license of
> JSDOMWindowBase prevents me from changing it. :/

If that is stopping you from progressing, please drop me a word.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151116/8254ba63/attachment-0001.html>