[webkit-reviews] review granted: [Bug 200431] Support RTCRtpSender.dtmf : [Attachment 375512] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 200431: Support RTCRtpSender.dtmf
https://bugs.webkit.org/show_bug.cgi?id=200431

Attachment 375512: Patch

https://bugs.webkit.org/attachment.cgi?id=375512&action=review




--- Comment #7 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 375512
  --> https://bugs.webkit.org/attachment.cgi?id=375512
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375512&action=review

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:104
> +	   m_isPendingPlayoutTask = false;

It is a bit confusing that you clear this here, set to true in
startPlayingTone, and clear it again in onTonePlayed. It might be easier to
follow if playNextTone returned a bool to signal whether or not a tone was
played so you can only clear it here when a tone is not played.

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:117
> +    auto currentTone = m_tones.substring(0, 1);
> +    m_tones = m_tones.substring(1);

Nit: I think this would be clearer if you used m_tones.characterStartingAt()
and m_tones.remove(0) since you only send one tone at a time.

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:144
> +    m_backend = nullptr;

Nit: should this also clear the timer?

>
Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.cpp:88
> +    callOnMainThread([this, weakThis = m_weakThis, tone = toWTFString(tone)]
{
> +	   if (!weakThis)
> +	       return;
> +	   if (m_onTonePlayed)
> +	       m_onTonePlayed(tone);
> +    });

This is called on the signaling thread, is it safe to use a weak ptr?