We are not able to test DTMFSender because of the lack of both actual implementation and MOCK implementation. This bug is to implement a mock implementation for DTMF sender for testing the default basic behavior.
Created attachment 232235 [details] Implemented mock RTCDTMFSender Implemented mock RTCDTMFSender
Comment on attachment 232235 [details] Implemented mock RTCDTMFSender 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
Comment on attachment 232235 [details] Implemented mock RTCDTMFSender View in context: https://bugs.webkit.org/attachment.cgi?id=232235&action=review >> Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:61 >> +} > > Two problems: this should be asynchronous, and it should process each character in tones. Actually you don't need to make this asynchronous because RTCDTMFSender::didPlayTone will post each RTCDTMFToneChangeEvent asynchronously, but you should call m_client->didPlayTone() for each character in "tones". See http://dev.w3.org/2011/webrtc/editor/webrtc.html#methods-4.
Created attachment 232282 [details] Implemented mock RTCDTMFSender Modified as per the review comments :).
Comment on attachment 232282 [details] Implemented mock RTCDTMFSender View in context: https://bugs.webkit.org/attachment.cgi?id=232282&action=review > Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:55 > + while (m_toneBuffer.length() > 0) { > + m_client->didPlayTone(m_toneBuffer.substring(0, 1)); > + m_toneBuffer.remove(0, 1); > + } This is an extremely inefficient way to do this. If this processing was done asynchronously it would be necessary to modify m_toneBuffer each time through the loop, but nothing can access m_toneBuffer while this runs so something like this would be better: for (size_t i = 0; i < m_toneBuffer.length(); ++i) m_client->didPlayTone(m_toneBuffer[i]); m_toneBuffer = emptyString(); m_client->didPlayTone(emptyString()); > Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:56 > + if (m_toneBuffer.isEmpty()) Why this test, m_toneBuffer is empty because you just emptied it.
Created attachment 232364 [details] Implemented mock RTCDTMFSender Modified loop and now ready for landing.
Attachment 232364 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mock/RTCDTMFSenderHandlerMock.cpp:53: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 232365 [details] Implemented mock RTCDTMFSender Updated the loop. Now ready for landing.
Comment on attachment 232365 [details] Implemented mock RTCDTMFSender Clearing flags on attachment: 232365 Committed r169528: <http://trac.webkit.org/changeset/169528>