Bug 133298 - Add mock DTMFSender support
Summary: Add mock DTMFSender support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-26 23:19 PDT by Kiran
Modified: 2014-06-03 01:14 PDT (History)
12 users (show)

See Also:


Attachments
Implemented mock RTCDTMFSender (12.20 KB, patch)
2014-05-28 23:26 PDT, Kiran
eric.carlson: review-
Details | Formatted Diff | Diff
Implemented mock RTCDTMFSender (12.59 KB, patch)
2014-05-29 23:58 PDT, Kiran
eric.carlson: review+
Details | Formatted Diff | Diff
Implemented mock RTCDTMFSender (12.56 KB, patch)
2014-06-01 21:29 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff
Implemented mock RTCDTMFSender (12.56 KB, patch)
2014-06-01 21:34 PDT, Kiran
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran 2014-05-26 23:19:06 PDT
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.
Comment 1 Kiran 2014-05-28 23:26:38 PDT
Created attachment 232235 [details]
Implemented mock RTCDTMFSender

Implemented mock RTCDTMFSender
Comment 2 Eric Carlson 2014-05-29 08:50:55 PDT
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 3 Eric Carlson 2014-05-29 13:55:18 PDT
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.
Comment 4 Kiran 2014-05-29 23:58:12 PDT
Created attachment 232282 [details]
Implemented mock RTCDTMFSender

Modified as per the review comments :).
Comment 5 Eric Carlson 2014-05-30 11:59:15 PDT
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.
Comment 6 Kiran 2014-06-01 21:29:33 PDT
Created attachment 232364 [details]
Implemented mock RTCDTMFSender

Modified loop and now ready for landing.
Comment 7 WebKit Commit Bot 2014-06-01 21:32:11 PDT
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.
Comment 8 Kiran 2014-06-01 21:34:41 PDT
Created attachment 232365 [details]
Implemented mock RTCDTMFSender

Updated the loop. Now ready for landing.
Comment 9 WebKit Commit Bot 2014-06-02 03:56:07 PDT
Comment on attachment 232365 [details]
Implemented mock RTCDTMFSender

Clearing flags on attachment: 232365

Committed r169528: <http://trac.webkit.org/changeset/169528>