<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189">bug 158189</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #280145 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189#c8">Comment # 8</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189">bug 158189</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=280145&action=diff" name="attach_280145" title="Updated patch">attachment 280145</a> <a href="attachment.cgi?id=280145&action=edit" title="Updated patch">[details]</a></span>
Updated patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280145&action=review">https://bugs.webkit.org/attachment.cgi?id=280145&action=review</a>
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:195
> + transceiver->setDirection(static_cast<RTCRtpTransceiver::Direction>(init.direction));</span >
This typecast should not be needed. It’s currently only needed because we are defining the enum twice (see my other comments).
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:67
> Vector<RefPtr<RTCRtpSender>> getSenders() const override { return m_senderSet; }
> Vector<RefPtr<RTCRtpReceiver>> getReceivers() const { return m_receiverSet; }</span >
It’s expensive to have functions that return an existing Vector by copying it. We should not do that unless we have to.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:78
> + enum class RtpTransceiverDirection {
> + Sendrecv,
> + Sendonly,
> + Recvonly,
> + Inactive
> + };</span >
Instead of re-defining this here, we should write this:
using RtpTransceiverDirection = RTCRtpTransceiver::Direction;
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:82
> + struct RtpTransceiverInit {
> + RtpTransceiverDirection direction;
> + };</span >
Eventually, we probably also want to define this in RTCRtpTransceiver and use "using" to pull it in here with the appropriate name.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:125
> +enum RTCRtpTransceiverDirection {
> + "sendrecv",
> + "sendonly",
> + "recvonly",
> + "inactive"
> +};</span >
Eventually we will make the bindings script smarter so we don’t have to repeat this in multiple IDL files. In the mean time we might want to comment so anyone modifying one knows they should modify the other.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCRtpSender.h:60
> + bool isStopped() const { return m_client == nullptr; }</span >
WebKit coding style says we write this !m_client rather than m_client == nullptr.
<span class="quote">> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:80
> + return emptyString();</span >
Maybe return inactiveString() here instead of emptyString()?
<span class="quote">> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.h:52
> + enum class Direction {
> + Sendrecv,
> + Sendonly,
> + Recvonly,
> + Inactive
> + };</span >
I find short enumerations like this really nice when writing out all on one line.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:50
> + Ref<MockRealtimeAudioSource> source = adoptRef(*new MockRealtimeAudioSource(name));</span >
I think auto works better here than writing the type a second time. I’d also omit the empty line below.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
> + Ref<MockRealtimeVideoSource> source = adoptRef(*new MockRealtimeVideoSource(name));</span >
I think auto works better here than writing the type a second time. I’d also omit the empty line below.</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>