[webkit-reviews] review granted: [Bug 106782] MediaStream API: Implement DTMF support in RTCPeerConnection : [Attachment 186330] Patch (just rebased)

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 106782: MediaStream API: Implement DTMF support in RTCPeerConnection
https://bugs.webkit.org/show_bug.cgi?id=106782

Attachment 186330: Patch (just rebased)
https://bugs.webkit.org/attachment.cgi?id=186330&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186330&action=review


I think the main thing that I would like to change about this patch is the
names used in the API.	RTCDTMF is just a mouthful.  IMHO, the WebRTC working
group has gone a bit overboard in trying to structure these names.  However, I
realize that a code review isn't the right place to raise those sorts of issues
and that I'm just arguing about what color to paint the bike shed.

Thanks for your patience and your explanations above.

> Source/WebCore/CMakeLists.txt:227
> +    Modules/mediastream/RTCDTMFSender.idl
> +    Modules/mediastream/RTCDTMFToneChangeEvent.idl

Can we drop the "RTC" prefix from all these names?  We already have the DTMF
prefix....

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

APPLE -> GOOGLE

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:40
> +static const long minToneDuration = 70;

minToneDuration <-- Can you add a unit?  minToneDurationMS for example

> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.h:54
> +    RTCDTMFToneChangeEvent(const String& tone);
> +    RTCDTMFToneChangeEvent(const RTCDTMFToneChangeEventInit&);

I would have marked these explicit.

> Source/WebCore/Modules/mediastream/RTCDTMFToneChangeEvent.idl:28
> +] interface RTCDTMFToneChangeEvent : Event {

I probably would have called this event just ToneChangeEvent (or at least
DTMFToneChangeEvent)