Description
Kiran
2014-05-15 05:54:21 PDT
Created attachment 231509 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
The default ToneDuration and ToneGap values are modified in the updated spec.
The patch updates the default values as per the spec.
Comment on attachment 231509 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
The changes look fine, but please add a test. A new test would be fine, or you should be able to modify LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html to test the min, max, and defaults.
Created attachment 231512 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Updated RTCPeerConnection-dtmf.html to check for default values, as per the comments.
Comment on attachment 231512 [details] The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. View in context: https://bugs.webkit.org/attachment.cgi?id=231512&action=review > LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:47 > dtmfsender.insertDTMF("1"); > shouldBeEqualToString('dtmfsender.toneBuffer', "1"); > + shouldBeEqualToString('dtmfsender.duration', "100"); > + shouldBeEqualToString('dtmfsender.interToneGap', "70"); insertDTMF() has two optional parameters: [RaisesException] void insertDTMF(DOMString tones, optional long duration, optional long interToneGap); so you can also test the tone duration and inter-tone gap maximum and minimum. Created attachment 231519 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Added tests to check min and max values for duration and min value for duration as specified in review comments.
Comment on attachment 231519 [details] The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. View in context: https://bugs.webkit.org/attachment.cgi?id=231519&action=review > LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:61 > + dtmfsender.insertDTMF("1", "40", "30"); > + shouldBeEqualToString('dtmfsender.toneBuffer', "1"); > + shouldBeEqualToString('dtmfsender.duration', "40"); > + shouldBeEqualToString('dtmfsender.interToneGap', "30"); > + > + dtmfsender.insertDTMF("1", "6000"); > + shouldBeEqualToString('dtmfsender.toneBuffer', "1"); > + shouldBeEqualToString('dtmfsender.duration', "6000"); > + shouldBeEqualToString('dtmfsender.interToneGap', "70"); > + You should also test to ensure that exceptions are thrown when the limits are exceeded. Something like this should work: testDOMException("dtmfsender.insertDTMF('1', '20', '30')", "DOMException.SYNTAX_ERR"); testDOMException("dtmfsender.insertDTMF('1', '7000', '30')", "DOMException.SYNTAX_ERR"); Created attachment 231558 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Added tests for testing min and max limits as per the review comments.
Created attachment 231566 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Update the reviewers name in ChangeLog file
Comment on attachment 231566 [details] The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. Rejecting attachment 231566 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 231566, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: a/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://webkit-queues.appspot.com/results/5342596842913792 Created attachment 231567 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Uploaded the patch again.
Committed r168966: <http://trac.webkit.org/changeset/168966> Reopened because no updated test results were landed. Please attach a patch with an updated RTCPeerConnection-dtmf-expected.txt. Hi, The code without this patch is also not giving the expected data, The reason for this is 1. There is not actual implementation for DTMFSender. 2. Mock implementation is, by default, returning null. So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature. I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file. (In reply to comment #13) > Hi, > The code without this patch is also not giving the expected data, > The reason for this is > 1. There is not actual implementation for DTMFSender. > 2. Mock implementation is, by default, returning null. > > So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature. > > I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file. Why don't you open a bug about adding mock DTMFSender support and add a FIXME that references that bug. Ideally that would have been done with this patch, but that is my fault for not noticing :-) Created attachment 232111 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Added FIXME to implement mock DTMFSenderHandler inorder to test basic functionality of DTMFSender.
Created attachment 232114 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.
Removed empty line in ChangeLog.
(In reply to comment #14) > (In reply to comment #13) > > Hi, > > The code without this patch is also not giving the expected data, > > The reason for this is > > 1. There is not actual implementation for DTMFSender. > > 2. Mock implementation is, by default, returning null. > > > > So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature. > > > > I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file. > > Why don't you open a bug about adding mock DTMFSender support and add a FIXME that references that bug. Ideally that would have been done with this patch, but that is my fault for not noticing :-) HI, I raised a new bug for implementing RTCDTMFSenderHandlerMock (BUG: 133298). Since this patch is already landed (comment 11), it seems we can close this bug and track this on 133298. |