<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Update RTCPeerConnection API and introduce PeerConnectionBackend"
href="https://bugs.webkit.org/show_bug.cgi?id=150166#c31">Comment # 31</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Update RTCPeerConnection API and introduce PeerConnectionBackend"
href="https://bugs.webkit.org/show_bug.cgi?id=150166">bug 150166</a>
from <span class="vcard"><a class="email" href="mailto:youennf@gmail.com" title="youenn fablet <youennf@gmail.com>"> <span class="fn">youenn fablet</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=265283&action=diff" name="attach_265283" title="Updated patch">attachment 265283</a> <a href="attachment.cgi?id=265283&action=edit" title="Updated patch">[details]</a></span>
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 <a href="https://github.com/w3c/web-platform-tests/tree/master/webrtc/">https://github.com/w3c/web-platform-tests/tree/master/webrtc/</a> that can be easily imported (see <a href="http://trac.webkit.org/wiki/WebKitW3CTesting">http://trac.webkit.org/wiki/WebKitW3CTesting</a>), maybe some other tests can be imported from Blink/Mozilla.
Please find some additional comments below.
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=265283&action=review">https://bugs.webkit.org/attachment.cgi?id=265283&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:34
> + Notable changes:</span >
Maybe for future developments, you could split each notable change in its own bug/patch?
<span class="quote">> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:40
> + return nullptr;</span >
I guess this function is not yet implemented.
Would it be useful to use notImplemented()?
<span class="quote">> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:71
> + virtual ScriptExecutionContext* context() const = 0;</span >
XHR and others are naming it scriptExecutionContext().
Would it be useful to rename it that way?
<span class="quote">> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:89
> + virtual void setLocalDescription(RTCSessionDescription*, PeerConnection::VoidPromise&&) = 0;</span >
Is there a specific reason to using a pointer here?
Also true for setRemoteDescription, addIceCandidate and getStats.
<span class="quote">> Source/WebCore/Modules/mediastream/PeerConnectionStates.h:40
> +enum class SignalingState {</span >
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.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:102
> +</span >
Remove empty line here.
You might also consider moving those getters/setters to header.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:37
> + : m_voiceActivityDetection(true)</span >
Move to header if that stays simple?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:57
> + virtual ~RTCOfferOptions() { }</span >
Is it needed?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:138
> + if (!streams.size()) {</span >
Is it possible for streams to have a null MediaStream pointer?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:223
> + return m_backend->localDescription();</span >
Move it to header?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:228
> + return m_backend->currentLocalDescription();</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:231
> +RefPtr<RTCSessionDescription> RTCPeerConnection::pendingLocalDescription() const</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:411
> + scriptExecutionContext()->postTask([=](ScriptExecutionContext&) {</span >
First time I see [=]() use in WebKit. It is nice to see it :)
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:36
> +</span >
For all these functions, we should somehow check that "this" has the right type.
We should work on that in the future.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:74
> + if (arguments.length == 1) {</span >
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.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:102
> + if (selector != null && !(selector instanceof MediaStreamTrack))</span >
Why checking selector != null?
Having a test might be good to validate the behavior.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:118
> + peerConnection.@privateGetStats(selector).then(successCallback, errorCallback);</span >
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.@then.@call is (unfortunately less readable but) safer.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:32
> +// @internal</span >
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.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:50
> + return new Promise(function (resolve, reject) {</span >
"return new @Promise" would be safer.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:72
> + return targetFunction.@apply(peerConnection, [options]);</span >
or @call(peerConnection, options)?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:84
> + return targetFunction.@apply(peerConnection, [options]).then(successCallback, errorCallback);</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:114
> + return targetFunction.@apply(peerConnection, [description]).then(successCallback, errorCallback);</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:129
> + return typeof object === "undefined" || object == null || (typeof object === "object");</span >
This function is generic.
Maybe we should move it to GlobalObject.js
<span class="quote">> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:45
> +RTCRtpSenderReceiverBase::~RTCRtpSenderReceiverBase()</span >
Move to header?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:49
> +MediaStreamTrack* RTCRtpSenderReceiverBase::track() const</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:-35
> -</span >
Not sure about the style, but usually there is an empty line between #if and #include
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:73
> + return adoptRef(*new RTCSessionDescription(type, sdp));</span >
Move to header?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:88
> + return m_type;</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:101
> + return m_sdp;</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:106
> + m_sdp = sdp;</span >
Ditto?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCSessionDescription.h:36
> #include "ExceptionBase.h"</span >
IIRC, if ExceptionCode is the only one used, it is preferred to remove that #include and add a typedef int ExceptionCode instead.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:63
> +RTCTrackEvent::RTCTrackEvent()</span >
Move to header?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:81
> +RTCTrackEvent::~RTCTrackEvent()</span >
Is it needed? Move to header?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>