[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 #31 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 265283
  --> https://bugs.webkit.org/attachment.cgi?id=265283
Updated patch

This looks like a great move forward :)

Two general comments come to mind:
- There is some refactoring as part of this patch that could have been splitted so as to ease the review. This may be good for future developments.
- I am surprised that no test is added/few tests are rebased. How is WebRTC tested? It would be great to add/activate tests, even if they are mostly failing at the beginning.
Without tests, it might be so easy to inadvertently break some behavior (like the promise/legacy switch for instance).
There are a few tests available at https://github.com/w3c/web-platform-tests/tree/master/webrtc/ that can be easily imported (see http://trac.webkit.org/wiki/WebKitW3CTesting), maybe some other tests can be imported from Blink/Mozilla.

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?

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:40
> +    return nullptr;

I guess this function is not yet implemented.
Would it be useful to use notImplemented()?

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

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

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

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:102
> +

Remove empty line here.
You might also consider moving those getters/setters to header.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:37
> +    : m_voiceActivityDetection(true)

Move to header if that stays simple?

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

Is it needed?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:138
> +    if (!streams.size()) {

Is it possible for streams to have a null MediaStream pointer?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:223
> +    return m_backend->localDescription();

Move it to header?

> 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 :)

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

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

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

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

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

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

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

"return new @Promise" would be safer.

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

or @call(peerConnection, options)?

> 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

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:45
> +RTCRtpSenderReceiverBase::~RTCRtpSenderReceiverBase()

Move to header?

> 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

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

Move to header?

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

Ditto?

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

> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:63
> +RTCTrackEvent::RTCTrackEvent()

Move to header?

> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:81
> +RTCTrackEvent::~RTCTrackEvent()

Is it needed? Move to header?

-- 
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/20151112/8bc00e16/attachment.html>