[webkit-reviews] review denied: [Bug 133298] Add mock DTMFSender support : [Attachment 232235] Implemented mock RTCDTMFSender
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 29 08:50:31 PDT 2014
Eric Carlson <eric.carlson at apple.com> has denied Kiran
<kiran.guduru at samsung.com>'s request for review:
Bug 133298: Add mock DTMFSender support
https://bugs.webkit.org/show_bug.cgi?id=133298
Attachment 232235: Implemented mock RTCDTMFSender
https://bugs.webkit.org/attachment.cgi?id=232235&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232235&action=review
> Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:36
> + : m_toneBuffer("")
This is unnecessary, the String default constructor will initialize this.
> Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:39
> + , m_insertDTMF(true)
Nit: this is never changed, why not just hard code canInsertDTMF() to return
true?
> Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:48
> + if (!client)
> + return;
> +
> + m_client = client;
This will leave m_client set to a bogus pointer when called from
RTCDTMFSender::stop!
> Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:61
> +bool RTCDTMFSenderHandlerMock::insertDTMF(const String& tones, long
duration, long interToneGap)
> +{
> + m_duration = duration;
> + m_interToneGap = interToneGap;
> + m_toneBuffer.append(tones);
> + m_client->didPlayTone(m_toneBuffer);
> + m_toneBuffer.remove(0, 1);
> + if (m_toneBuffer.isEmpty())
> + m_client->didPlayTone("");
> + return true;
> +}
Two problems: this should be asynchronous, and it should process each character
in tones.
> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:173
> // Requires a mock implementation of RTCDTMFSenderHandler.
> // This must be implemented once the mock implementation of
RTCDataChannelHandler is ready.
> notImplemented();
Are these still necessary?
> LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:58
> dtmfsender.insertDTMF("1", "6000");
You should also test a multi-character dtmf string.
> LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:65
> +//finishJSTest();
Oops :-O
More information about the webkit-reviews
mailing list