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

Fri Nov 13 09:27:58 PST 2015

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

--- Comment #34 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(In reply to comment #31)
> Comment on attachment 265283 [details]
> Updated patch
> 
> [...]
>
> Please find some additional comments below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265283&action=review
> 
> > Source/WebCore/ChangeLog:34
> > +        Notable changes:
> 
> Maybe for future developments, you could split each notable change in its
> own bug/patch?

Yes. The size of this patch is far from ideal. I plan to do follow-up patches on a more per-feature basis.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:40
> > +    return nullptr;
> 
> I guess this function is not yet implemented.
> Would it be useful to use notImplemented()?

This is the create function used when no proper implementation is available. The nullptr will result in an exception being thrown when an RTCPeerConnection is created. I borrowed this pattern from RTCPeerConnectionHandler.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:71
> > +    virtual ScriptExecutionContext* context() const = 0;
> 
> XHR and others are naming it scriptExecutionContext().
> Would it be useful to rename it that way?
> 

The scriptExecutionContext() inherited from ActiveDOMObject seems to satisfy the interface, so let's use that one.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:89
> > +    virtual void setLocalDescription(RTCSessionDescription*, PeerConnection::VoidPromise&&) = 0;
> 
> Is there a specific reason to using a pointer here?
> Also true for setRemoteDescription, addIceCandidate and getStats.

Null shouldn't get passed the bindings so we can use refs here. Added ASSERTs as well.

> > Source/WebCore/Modules/mediastream/PeerConnectionStates.h:40
> > +enum class SignalingState {
> 
> It seems SignalingState is mainly used in RTCPeerConnection.
> Would it make sense to move the definition within RTCPeerConnection as a
> public enum class?
> Or is it supposed to be used in other contexts?
> 
> Might apply to other enum classes as well.

These states used to be part of a platform interface to be accessible by the WebRTC backend. They are not used that way currently but other backends may want to access them in the future.

> > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:102
> > +
> 
> Remove empty line here.

Fixed.

> You might also consider moving those getters/setters to header.

Moved.

> > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:37
> > +    : m_voiceActivityDetection(true)
> 
> Move to header if that stays simple?

I think the cpp is fine in this case since the other constructor with an init list is there.

> > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:57
> > +    virtual ~RTCOfferOptions() { }
> 
> Is it needed?

Nope. Same goes for the one in RTCAnswerOptions.

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:138
> > +    if (!streams.size()) {
> 
> Is it possible for streams to have a null MediaStream pointer?

The binding catches that.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:223
> > +    return m_backend->localDescription();
> 
> Move it to header?

I think the header looks nicer without these longer expressions inlined. I don't have a strong opinion though.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:228
> > +    return m_backend->currentLocalDescription();
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:231
> > +RefPtr<RTCSessionDescription> RTCPeerConnection::pendingLocalDescription() const
> 
> Ditto?
>
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:411
> > +    scriptExecutionContext()->postTask([=](ScriptExecutionContext&) {
> 
> First time I see [=]() use in WebKit. It is nice to see it :)

I believe it was in this file already so I can't take credit for introducing it. :)

> > 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?

> > 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.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:102
> > +        if (selector != null && !(selector instanceof MediaStreamTrack))
> 
> Why checking selector != null?
> Having a test might be good to validate the behavior.

Null is allowed. We should be able to test this rather soon.

> > 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.

> > 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. :/

> In the future, tooling should be improved to not require that.

That would be great.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:50
> > +    return new Promise(function (resolve, reject) {
> 
> "return new @Promise" would be safer.

Fixed.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:72
> > +            return targetFunction. at apply(peerConnection, [options]);
> 
> or @call(peerConnection, options)?

That's better (not having to create the arguments list). Fixed.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:84
> > +        return targetFunction. at apply(peerConnection, [options]).then(successCallback, errorCallback);
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:114
> > +        return targetFunction. at apply(peerConnection, [description]).then(successCallback, errorCallback);
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:129
> > +    return typeof object === "undefined" || object == null || (typeof object === "object");
> 
> This function is generic.
> Maybe we should move it to GlobalObject.js

Fixed.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:45
> > +RTCRtpSenderReceiverBase::~RTCRtpSenderReceiverBase()
> 
> Move to header?

Lets skip the cpp file all together here.

> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:49
> > +MediaStreamTrack* RTCRtpSenderReceiverBase::track() const
> 
> Ditto?
>
>
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:-35
> > -
> 
> Not sure about the style, but usually there is an empty line between #if and
> #include
>

Seems to be more common with an empty line. Seems to work both ways for style checker.

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:73
> > +    return adoptRef(*new RTCSessionDescription(type, sdp));
> 
> Move to header?

There are three create functions and one is pretty big. My thinking was to keep the all in the cpp even though two are one-liners.

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:88
> > +    return m_type;
> 
> Ditto?
>

Fixed (and 2 more)

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:101
> > +    return m_sdp;
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:106
> > +    m_sdp = sdp;
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.h:36
> >  #include "ExceptionBase.h"
> 
> IIRC, if ExceptionCode is the only one used, it is preferred to remove that
> #include and add a typedef int ExceptionCode instead.

Fixed.

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:63
> > +RTCTrackEvent::RTCTrackEvent()
> 
> Move to header?

I can't keep my forward declaration of RTCRtpReceiver if I move it.

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:81
> > +RTCTrackEvent::~RTCTrackEvent()
> 
> Is it needed? Move to header?

Removed

-- 
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/20151113/85591221/attachment-0001.html>