Summary: | Add mock DTMFSender support | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kiran <kiran.guduru> | ||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bunhere, commit-queue, eric.carlson, glenn, gyuyoung.kim, hta, jer.noble, lucas.de.marchi, philipj, rakuco, sergio, tommyw | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Kiran
2014-05-26 23:19:06 PDT
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> |