[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

--- Comment #26 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(In reply to comment #22)
> Comment on attachment 264278 [details]
> Rebased patch
> 
> Some comments below mostly related to js builtins.

A lot of good points below. Thanks for reviewing.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264278&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:31
> > +// @optional=MEDIA_STREAM
> 
> The annotation scheme changed and is now more inline with other generators.
> One can use @conditional=ENABLE(MEDIA_STREAM)

Updated

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:40
> > +    if (arguments.length > 1 && arguments[0] instanceof Function && arguments[1] instanceof Function) {
> 
> If arguments[0] is not a function, the function switches to promise mode,
> even if arguments[1] is a function and options a dictionary.
> It may be easy to make an error when using that function, and switching from
> callback to promise version.
> Just wondering whether that is a good pattern for users.

This is not ideal indeed. The promise and callback versions are specified as overloaded functions so I've rewritten the bindings to behave as such.

> On another point, most built-ins use (typeof arg === "function").

Fixed. At first glance I thought it would be safe to always assume a string return value from this native operator, but it seems that it can return "implementation-dependent" values. So === is safest.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:48
> > +                throw new @TypeError("Argument 3 ('options') to RTCPeerConnection.createOffer must be a Dictionary");
> 
> As discussed with adam in another bug patch, it might be good to add
> routines for this js binding kind of error throwing.
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:52
> > +            return peerConnection.internalCreateOffer(options).then(successCallback, errorCallback);
> 
> Might want to use [ Private ] keyword to have @internalCreateOffer.

Fixed. Great to have [ Private ] in place now.

> Might also want to use ImplementedAs so that the C++ createOffer function is
> called directly.

I don't think that can be used in this case since the C++ function never gets called directly (but always via the operations queue)

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:69
> > +function createAnswer()
> 
> It seems that all these functions share a lot of code.
> It would be good if this could be factored out in some way.

True. The createOffer/Answer() and setLocal/RemoteDescription() "pairs" now share more code.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:35
> > +function runQueuedOperation(peerConnection, operation)
> 
> Rename to runQueuedOperations?

This function is called for every call to a "queued operation". I renamed it to enqueueOperation().

> Also would it make sense to take directly @operations as input parameter?
> That would allow this function to be reusable in other contexts in the
> future.

Right now I ensure the existence of @operations at a single point inside this function. Passing the operations array to the function would require ensuring @operation before the call and that's done from multiple locations. I'm not sure how reusable this function is since it implements the quite specific "operations queue" from the WebRTC 1.0 spec.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:62
> > +    return typeof object == "undefined" || object == null || @isObject(object);
> 
> Might want to use ===

Fixed.

> Also, this function is generic.
> In the future, we should ensure to reuse existing JS built-ins, so probably
> move these two functions in some other place.

I totally agree. Leaving it as is for this change though.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38
> > +Ref<RTCRtpReceiver> RTCRtpReceiver::create(RefPtr<MediaStreamTrack>&& track)
> 
> Would it make sense to move these methods to the header file, or are you
> expecting to add more code in them later on?

Moved. This makes the cpp file rather slim, but I'm keeping since it will get more content before it's properly implemented.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:50
> > +}
> 
> Is this destructor needed?

Fixed.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:46
> > +    RTCRtpReceiver(RefPtr<MediaStreamTrack>&&);
> 
> Use explicit?

Fixed

-- 
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/20151110/87dbd632/attachment.html>