[Webkit-unassigned] [Bug 133298] Add mock DTMFSender support

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 29 08:50:35 PDT 2014


https://bugs.webkit.org/show_bug.cgi?id=133298


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232235|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from Eric Carlson <eric.carlson at apple.com>  2014-05-29 08:50:55 PST ---
(From update of attachment 232235)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list