Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531333005:
Unit tests of AudioInputRendererHost. Some cleanups. (Closed)
|
Created:
4 years ago by Max Morin Modified:
4 years ago CC:
audio-mojo-cl_google.com, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, o1ka, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnit tests of AudioInputRendererHost. Some cleanups.
This change enables refactoring of AudioInputRendererHost and
surrounding code.
In this CL:
* Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
* Sanitize volume in AudioInputRendererHost::OnSetVolume.
* Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController.
* Removed skipping authorization check for fake devices in AudioInputRendendererHost
* Removed unused weak_factory member of AudioInputRendererHost.
* Added unit tests for AudioInputRendererHost.
BUG=653861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/69cdc9514c495f7612463039885efeb8000d5184
Cr-Commit-Position: refs/heads/master@{#438539}
Patch Set 1 #Patch Set 2 : Tab capture test. #Patch Set 3 : . #
Total comments: 29
Patch Set 4 : . #Patch Set 5 : Bad message. Handle flag in audio manager. #Patch Set 6 : . #Patch Set 7 : Windows compile. #
Total comments: 6
Patch Set 8 : Dale's comments. Windows compile. #
Total comments: 2
Patch Set 9 : Clamy's comments. #
Total comments: 4
Patch Set 10 : Small comments describing tests. #Messages
Total messages: 52 (25 generated)
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitise volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from * AudioInputController::Factory::Create. * Remove OnRecording message from AudioInputController event interface since it is not used in any meaningful way. BUG=654861 ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitise volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from * AudioInputController::Factory::Create. * Remove OnRecording message from AudioInputController event interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitise volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from * AudioInputController::Factory::Create. * Remove OnRecording message from AudioInputController event interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from * AudioInputController::Factory::Create. * Remove OnRecording message from AudioInputController event interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from * AudioInputController::Factory::Create. * Remove OnRecording message from AudioInputController event interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from AudioInputController::Factory::Create. * Remove OnRecording message from the AudioInputController event handler interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Made AudioInputRendererHosts implementation of AudioInputController::EventHandler private. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from AudioInputController::Factory::Create. * Remove OnRecording message from the AudioInputController event handler interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from AudioInputController::Factory::Create. * Remove OnRecording message from the AudioInputController event handler interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
maxmorin@chromium.org changed reviewers: + guidou@chromium.org
Guido: PTAL at these tests, especially the tab capture stream test. Also, since you know MediaStreamManager, are there cases these tests are missing?
Awesome, it's a perfect moment to write the tests, since we are going to make changes around AIRH. Sorry, could not help some drive-by comments :) But I resisted looking into tests themselves. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (left): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:192: if (!PeerHandle()) { You change the behavior here and below when DCHECK is off (continue instead of return). This does no look safe. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:51: std::string message = oss.str(); const https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:304: if (LookupById(stream_id) != nullptr) { if (LookupById(stream_id)) if you decided to touch it :) https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:322: const std::string device_id = info->device.id; Why do you need these locals? https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:423: std::string log_message = oss.str(); const https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) Why? And is it ok to silently ignore it? https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.h (left): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.h:139: // TODO(henrika): extend test suite (compare AudioRenderHost) Awesome that you do that!
I'll wait with making a new patch set until Guido has reviewed. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (left): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:192: if (!PeerHandle()) { On 2016/12/01 11:17:49, o1ka wrote: > You change the behavior here and below when DCHECK is off (continue instead of > return). This does no look safe. We know all of the things will not happen; if we are worried about them happening anyways, how about making them CHECKs, and if they doesn't crash in a while make them DCHECKs? https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:322: const std::string device_id = info->device.id; On 2016/12/01 11:17:49, o1ka wrote: > Why do you need these locals? One reason is that the rest of the function don't need to be rewritten :). I should change them to const& though. I'm sure there's a good, well-planned refactoring to do here, so I'll wait with big changes until another CL. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) On 2016/12/01 11:17:49, o1ka wrote: > Why? And is it ok to silently ignore it? This function takes volumes between 0 and 1 (should be documented, but I'm not sure where). This number is scaled by the max volume of the underlying stream in AudioInpuController. https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?sq.... This should technically be a bad_message, but I don't see any security implication of silently ignoring it. I have no idea what actually happened in the old code if the volume was <0 or >1, depends on AudioInputStream implementation.
I think you should remove the future cleanups from the description and file a bug instead. Also, the bug number is wrong. It leads to "Consider dropping support for hover:on-demand" https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (left): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:192: if (!PeerHandle()) { On 2016/12/01 11:35:02, Max Morin Chromium wrote: > On 2016/12/01 11:17:49, o1ka wrote: > > You change the behavior here and below when DCHECK is off (continue instead of > > return). This does no look safe. > > We know all of the things will not happen; if we are worried about them > happening anyways, how about making them CHECKs, and if they doesn't crash in a > while make them DCHECKs? According to the style guide, these should be DCHECKs. I say let's keep them as DCHECKs and revisit if there is a problem. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. Is it a goal of this CL to remove support for the kUserFakeDeviceForMediaStream command-line flag, or is it automatically handled somewhere else? https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) On 2016/12/01 11:35:02, Max Morin Chromium wrote: > On 2016/12/01 11:17:49, o1ka wrote: > > Why? And is it ok to silently ignore it? > > This function takes volumes between 0 and 1 (should be documented, but I'm not > sure where). This number is scaled by the max volume of the underlying stream in > AudioInpuController. > https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?sq.... > This should technically be a bad_message, but I don't see any security > implication of silently ignoring it. I have no idea what actually happened in > the old code if the volume was <0 or >1, depends on AudioInputStream > implementation. If it's a bad message, why not call received_bad_message? Or put some DCHECKs to help identify bugs? https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:524: controls.audio.device_ids.push_back(device_id); You'll have to rebase this because https://codereview.chromium.org/2538033003/ recently changed |device_ids| to a single |device_id|. https://codereview.chromium.org/2531333005/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2531333005/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:131: return factory_->Create(audio_manager->GetTaskRunner(), nullptr, nit: document what's null here
https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (left): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:192: if (!PeerHandle()) { On 2016/12/02 10:22:25, Guido Urdaneta wrote: > On 2016/12/01 11:35:02, Max Morin Chromium wrote: > > On 2016/12/01 11:17:49, o1ka wrote: > > > You change the behavior here and below when DCHECK is off (continue instead > of > > > return). This does no look safe. > > > > We know all of the things will not happen; if we are worried about them > > happening anyways, how about making them CHECKs, and if they doesn't crash in > a > > while make them DCHECKs? > > According to the style guide, these should be DCHECKs. > I say let's keep them as DCHECKs and revisit if there is a problem. SGTM https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:51: std::string message = oss.str(); On 2016/12/01 11:17:49, o1ka wrote: > const Done. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:304: if (LookupById(stream_id) != nullptr) { On 2016/12/01 11:17:49, o1ka wrote: > if (LookupById(stream_id)) if you decided to touch it :) Done. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. On 2016/12/02 10:22:25, Guido Urdaneta wrote: > Is it a goal of this CL to remove support for the kUserFakeDeviceForMediaStream > command-line flag, or is it automatically handled somewhere else? I figured that the renderer would get fake devices with fake audio parameters when enumerating, in which case we could treat them like any other device and it would just work (but they would need a session_id, like any other device). Maybe this is not the case? The old code bypasses the code I actually want to test when using fake devices, so I want to get rid of it somehow. I'll think more about it. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:423: std::string log_message = oss.str(); On 2016/12/01 11:17:49, o1ka wrote: > const Done. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) On 2016/12/02 10:22:25, Guido Urdaneta wrote: > On 2016/12/01 11:35:02, Max Morin Chromium wrote: > > On 2016/12/01 11:17:49, o1ka wrote: > > > Why? And is it ok to silently ignore it? > > > > This function takes volumes between 0 and 1 (should be documented, but I'm not > > sure where). This number is scaled by the max volume of the underlying stream > in > > AudioInpuController. > > > https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?sq.... > > This should technically be a bad_message, but I don't see any security > > implication of silently ignoring it. I have no idea what actually happened in > > the old code if the volume was <0 or >1, depends on AudioInputStream > > implementation. > > If it's a bad message, why not call received_bad_message? > Or put some DCHECKs to help identify bugs? DCHECKs here are not ok, since we then let an invalid message from the renderer through to the AudioInputDevice, and we don't know what they will do with it. According to https://www.chromium.org/Home/chromium-security/education/security-tips-for-i..., CHECKs are not good since we give the renderer an easy way to kill the browser (though this point is a bit nonsensical, since it's very easy anyways). Bad message makes sense, but requires some bureaucracy (extra owners review for both metrics and content) and extra lines. I added a DCHECK on the renderer side in audio_input_message_filter.cc. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:524: controls.audio.device_ids.push_back(device_id); On 2016/12/02 10:22:25, Guido Urdaneta wrote: > You'll have to rebase this because https://codereview.chromium.org/2538033003/ > recently changed |device_ids| to a single |device_id|. Done. https://codereview.chromium.org/2531333005/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2531333005/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:131: return factory_->Create(audio_manager->GetTaskRunner(), nullptr, On 2016/12/02 10:22:25, Guido Urdaneta wrote: > nit: document what's null here Done.
https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. On 2016/12/02 14:24:52, Max Morin Chromium wrote: > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > Is it a goal of this CL to remove support for the > kUserFakeDeviceForMediaStream > > command-line flag, or is it automatically handled somewhere else? > > I figured that the renderer would get fake devices with fake audio parameters > when enumerating, in which case we could treat them like any other device and it > would just work (but they would need a session_id, like any other device). Maybe > this is not the case? The old code bypasses the code I actually want to test > when using fake devices, so I want to get rid of it somehow. I'll think more > about it. I guess a manual test with the command-line flag on should clarify everything about this. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) On 2016/12/02 14:24:51, Max Morin Chromium wrote: > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > On 2016/12/01 11:35:02, Max Morin Chromium wrote: > > > On 2016/12/01 11:17:49, o1ka wrote: > > > > Why? And is it ok to silently ignore it? > > > > > > This function takes volumes between 0 and 1 (should be documented, but I'm > not > > > sure where). This number is scaled by the max volume of the underlying > stream > > in > > > AudioInpuController. > > > > > > https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?sq.... > > > This should technically be a bad_message, but I don't see any security > > > implication of silently ignoring it. I have no idea what actually happened > in > > > the old code if the volume was <0 or >1, depends on AudioInputStream > > > implementation. > > > > If it's a bad message, why not call received_bad_message? > > Or put some DCHECKs to help identify bugs? > > DCHECKs here are not ok, since we then let an invalid message from the renderer > through to the AudioInputDevice, and we don't know what they will do with it. > According to > https://www.chromium.org/Home/chromium-security/education/security-tips-for-i..., > CHECKs are not good since we give the renderer an easy way to kill the browser > (though this point is a bit nonsensical, since it's very easy anyways). Bad > message makes sense, but requires some bureaucracy (extra owners review for both > metrics and content) and extra lines. I added a DCHECK on the renderer side in > audio_input_message_filter.cc. There is no bureaucracy for this. If receive_bad_message is what makes sense, then use it and have the appropriate reviewers take a look at it.
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. Future cleanups: * Rename AudioInputMsg_NotifyStreamStateChanged AudioInputMsg_NotifyStreamError (since only error is used). * Limit the size of the buffer that the renderer is allowed to create. * Pass the task runner into the AudioInputController constructor. * Return scoped_refptr<AudioInputController> from AudioInputController::Factory::Create. * Remove OnRecording message from the AudioInputController event handler interface since it is not used in any meaningful way. BUG=654861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Guido: PTAL. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. On 2016/12/02 14:56:01, Guido Urdaneta wrote: > On 2016/12/02 14:24:52, Max Morin Chromium wrote: > > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > > Is it a goal of this CL to remove support for the > > kUserFakeDeviceForMediaStream > > > command-line flag, or is it automatically handled somewhere else? > > > > I figured that the renderer would get fake devices with fake audio parameters > > when enumerating, in which case we could treat them like any other device and > it > > would just work (but they would need a session_id, like any other device). > Maybe > > this is not the case? The old code bypasses the code I actually want to test > > when using fake devices, so I want to get rid of it somehow. I'll think more > > about it. > > I guess a manual test with the command-line flag on should clarify everything > about this. Ok, so actually this layer is responsible for fake devices. It doesn't really make sense, since this layer is responsible for IPC. It would make more sense to handle fake devices in the layer that handles device (audio manager). This would also allow browser tests and other tests to get a accurate behavior of the production code all the way down to that layer. WDYT? I uploaded a change to audio manager to show what I mean. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:473: if (volume < 0 || volume > 1) On 2016/12/02 14:56:01, Guido Urdaneta wrote: > On 2016/12/02 14:24:51, Max Morin Chromium wrote: > > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > > On 2016/12/01 11:35:02, Max Morin Chromium wrote: > > > > On 2016/12/01 11:17:49, o1ka wrote: > > > > > Why? And is it ok to silently ignore it? > > > > > > > > This function takes volumes between 0 and 1 (should be documented, but I'm > > not > > > > sure where). This number is scaled by the max volume of the underlying > > stream > > > in > > > > AudioInpuController. > > > > > > > > > > https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?sq.... > > > > This should technically be a bad_message, but I don't see any security > > > > implication of silently ignoring it. I have no idea what actually happened > > in > > > > the old code if the volume was <0 or >1, depends on AudioInputStream > > > > implementation. > > > > > > If it's a bad message, why not call received_bad_message? > > > Or put some DCHECKs to help identify bugs? > > > > DCHECKs here are not ok, since we then let an invalid message from the > renderer > > through to the AudioInputDevice, and we don't know what they will do with it. > > According to > > > https://www.chromium.org/Home/chromium-security/education/security-tips-for-i..., > > CHECKs are not good since we give the renderer an easy way to kill the browser > > (though this point is a bit nonsensical, since it's very easy anyways). Bad > > message makes sense, but requires some bureaucracy (extra owners review for > both > > metrics and content) and extra lines. I added a DCHECK on the renderer side in > > audio_input_message_filter.cc. > > There is no bureaucracy for this. If receive_bad_message is what makes sense, > then use it and have the appropriate reviewers take a look at it. Done.
https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. On 2016/12/05 14:11:16, Max Morin Chromium wrote: > On 2016/12/02 14:56:01, Guido Urdaneta wrote: > > On 2016/12/02 14:24:52, Max Morin Chromium wrote: > > > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > > > Is it a goal of this CL to remove support for the > > > kUserFakeDeviceForMediaStream > > > > command-line flag, or is it automatically handled somewhere else? > > > > > > I figured that the renderer would get fake devices with fake audio > parameters > > > when enumerating, in which case we could treat them like any other device > and > > it > > > would just work (but they would need a session_id, like any other device). > > Maybe > > > this is not the case? The old code bypasses the code I actually want to test > > > when using fake devices, so I want to get rid of it somehow. I'll think more > > > about it. > > > > I guess a manual test with the command-line flag on should clarify everything > > about this. > > Ok, so actually this layer is responsible for fake devices. It doesn't really > make sense, since this layer is responsible for IPC. It would make more sense to > handle fake devices in the layer that handles device (audio manager). This would > also allow browser tests and other tests to get a accurate behavior of the > production code all the way down to that layer. WDYT? I uploaded a change to > audio manager to show what I mean. I think you are right. I just want to make sure that this CL doesn't introduce a regression. You can do the refactoring you propose in another CL.
How about still changing the format of the audio parameters to fake in AIRH, but start checking if session id is valid? I don't think there are supported uses of the flag other than tests, and if the tests break, we notice it before the patch is submitted. https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_renderer_host.cc:310: // Check if we have the permission to open the device and which device to use. On 2016/12/05 16:36:27, Guido Urdaneta wrote: > On 2016/12/05 14:11:16, Max Morin Chromium wrote: > > On 2016/12/02 14:56:01, Guido Urdaneta wrote: > > > On 2016/12/02 14:24:52, Max Morin Chromium wrote: > > > > On 2016/12/02 10:22:25, Guido Urdaneta wrote: > > > > > Is it a goal of this CL to remove support for the > > > > kUserFakeDeviceForMediaStream > > > > > command-line flag, or is it automatically handled somewhere else? > > > > > > > > I figured that the renderer would get fake devices with fake audio > > parameters > > > > when enumerating, in which case we could treat them like any other device > > and > > > it > > > > would just work (but they would need a session_id, like any other device). > > > Maybe > > > > this is not the case? The old code bypasses the code I actually want to > test > > > > when using fake devices, so I want to get rid of it somehow. I'll think > more > > > > about it. > > > > > > I guess a manual test with the command-line flag on should clarify > everything > > > about this. > > > > Ok, so actually this layer is responsible for fake devices. It doesn't really > > make sense, since this layer is responsible for IPC. It would make more sense > to > > handle fake devices in the layer that handles device (audio manager). This > would > > also allow browser tests and other tests to get a accurate behavior of the > > production code all the way down to that layer. WDYT? I uploaded a change to > > audio manager to show what I mean. > > I think you are right. I just want to make sure that this CL doesn't introduce a > regression. You can do the refactoring you propose in another CL. Agree.
On 2016/12/06 07:45:15, Max Morin Chromium wrote: > How about still changing the format of the audio parameters to fake in AIRH, but > start checking if session id is valid? I don't think there are supported uses of > the flag other than tests, and if the tests break, we notice it before the patch > is submitted. > Sounds good.
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed special-casing for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed skipping authorization check for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
On 2016/12/06 12:37:35, Guido Urdaneta wrote: > On 2016/12/06 07:45:15, Max Morin Chromium wrote: > > How about still changing the format of the audio parameters to fake in AIRH, > but > > start checking if session id is valid? I don't think there are supported uses > of > > the flag other than tests, and if the tests break, we notice it before the > patch > > is submitted. > > > > Sounds good. Cool. Do you have more comments or should I go ahead and add owners? Also, please click dry run so I can see if any tests fail due to the fake device behavior change (I cannot since I'm not a committer).
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/06 14:55:53, Max Morin Chromium wrote: > On 2016/12/06 12:37:35, Guido Urdaneta wrote: > > On 2016/12/06 07:45:15, Max Morin Chromium wrote: > > > How about still changing the format of the audio parameters to fake in AIRH, > > but > > > start checking if session id is valid? I don't think there are supported > uses > > of > > > the flag other than tests, and if the tests break, we notice it before the > > patch > > > is submitted. > > > > > > > Sounds good. > > Cool. Do you have more comments or should I go ahead and add owners? Also, > please click dry run so I can see if any tests fail due to the fake device > behavior change (I cannot since I'm not a committer). I think this is ready for owners review, provided the dry run does not reveal any obvious error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
maxmorin@chromium.org changed reviewers: + asvitkine@chromium.org, clamy@chromium.org, dalecurtis@chromium.org
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dale: PTAL at these tests, and the changes listed in the description. There is a change in behaviour in that before, when using fake input devices, authorization wasn't needed. This makes tests bad, so I changed it, and if the tests still pass I think the change is ok since fake devices mostly (only?) concern tests. Clamy: PTAL at content/browser/bad_message.h and content/test/BUILD.gn. Alexei: PTAL at histograms.xml.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm % nits. https://codereview.chromium.org/2531333005/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/2531333005/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host.h:55: enum ErrorCode { Are these histogrammed? If not doesn't seem necessary to keep them the same values. https://codereview.chromium.org/2531333005/diff/120001/media/audio/audio_inpu... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2531333005/diff/120001/media/audio/audio_inpu... media/audio/audio_input_controller.h:150: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, non const&? https://codereview.chromium.org/2531333005/diff/120001/media/audio/test_audio... File media/audio/test_audio_input_controller_factory.cc (right): https://codereview.chromium.org/2531333005/diff/120001/media/audio/test_audio... media/audio/test_audio_input_controller_factory.cc:55: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, Ditto. https://codereview.chromium.org/2531333005/diff/120001/media/audio/test_audio... File media/audio/test_audio_input_controller_factory.h (right): https://codereview.chromium.org/2531333005/diff/120001/media/audio/test_audio... media/audio/test_audio_input_controller_factory.h:103: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, Ditto.
https://codereview.chromium.org/2531333005/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/2531333005/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host.h:55: enum ErrorCode { On 2016/12/06 21:33:42, DaleCurtis wrote: > Are these histogrammed? If not doesn't seem necessary to keep them the same > values. I'll just remove them. The comment implies that the values are read from logs (and then supposedly looked up here to find the actual error) but I don't think anyone actually does that. Looking through all the logging in this class to see what's actually used would be nice to do at some point. https://codereview.chromium.org/2531333005/diff/120001/media/audio/audio_inpu... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2531333005/diff/120001/media/audio/audio_inpu... media/audio/audio_input_controller.h:150: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, On 2016/12/06 21:33:42, DaleCurtis wrote: > non const&? Done (all of them).
Wow ,this is a nice article.The language is so simple ,so everyone can understand.There are so many courses offering on computerscience,A student should study about it so deeply. The media is advancing day today life.Researchers are going on this field.As a staff of http://shabdhaclinic.com/ i like to say few points about it.
Thanks for the cleanup! Here are a few comments below, I still need to do a full pass on the unit test file, will do so tomorrow. https://codereview.chromium.org/2531333005/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host.cc (left): https://codereview.chromium.org/2531333005/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host.cc:338: switches::kUseFakeDeviceForMediaStream)) { Per style-guide, the curly braces should remain if the if condition spans more than one line. https://codereview.chromium.org/2531333005/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/2531333005/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host.cc:323: const MediaStreamType& type = info->device.type; nit: blank line above.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Guido, Alexei, Clamy: Ping!
lgtm
Thanks! I've done a pass through the unit tests. I'm unfamiliar with the code though, so I would prefer if you got a review from an owner of content/browser/renderer_host/media/ before I sign this off. https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc (right): https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:326: TEST_F(AudioInputRendererHostTest, CreateWithDefaultDevice) { It'd be nice to have comments describing what these tests are testing. https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:331: NotifyStreamCreated(kStreamId, _, _, _, kSharedMemoryCount)); What do the underscores in this function call mean? Using code search I can't find usage in C++ code using code search.
histograms.xml lgtm
On 2016/12/12 15:16:37, clamy wrote: > Thanks! I've done a pass through the unit tests. I'm unfamiliar with the code > though, so I would prefer if you got a review from an owner of > content/browser/renderer_host/media/ before I sign this off. > > https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... > File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc > (right): > > https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... > content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:326: > TEST_F(AudioInputRendererHostTest, CreateWithDefaultDevice) { > It'd be nice to have comments describing what these tests are testing. > > https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... > content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:331: > NotifyStreamCreated(kStreamId, _, _, _, kSharedMemoryCount)); > What do the underscores in this function call mean? Using code search I can't > find usage in C++ code using code search. Thank you. Dale, who already had a look, is a media owner.
Sorry, I realize I didn't publish my comments. https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc (right): https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:326: TEST_F(AudioInputRendererHostTest, CreateWithDefaultDevice) { On 2016/12/12 15:16:37, clamy wrote: > It'd be nice to have comments describing what these tests are testing. I've added some small comments to the tests. https://codereview.chromium.org/2531333005/diff/160001/content/browser/render... content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:331: NotifyStreamCreated(kStreamId, _, _, _, kSharedMemoryCount)); On 2016/12/12 15:16:37, clamy wrote: > What do the underscores in this function call mean? Using code search I can't > find usage in C++ code using code search. It's testing::_, indicating that any argument is fine for this mock call.
Thanks! Lgtm.
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, guidou@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2531333005/#ps180001 (title: "Small comments describing tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481731003333410,
"parent_rev": "2bcba5b3018fa24ead785e6104fff53d88bd2e5f", "commit_rev":
"71a86660ce7bd654dfe8b1758cd7703b713c5a8f"}
Message was sent while issue was closed.
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed skipping authorization check for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed skipping authorization check for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2531333005 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed skipping authorization check for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2531333005 ========== to ========== Unit tests of AudioInputRendererHost. Some cleanups. This change enables refactoring of AudioInputRendererHost and surrounding code. In this CL: * Cleaned up instances of handling DCHECK errors in AudioInputRendererHost, see style guide https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... * Sanitize volume in AudioInputRendererHost::OnSetVolume. * Generalized AudioInputController::Factory interface and made sure the factory is used for all the factory methods of AudioInputController. * Removed skipping authorization check for fake devices in AudioInputRendendererHost * Removed unused weak_factory member of AudioInputRendererHost. * Added unit tests for AudioInputRendererHost. BUG=653861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/69cdc9514c495f7612463039885efeb8000d5184 Cr-Commit-Position: refs/heads/master@{#438539} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/69cdc9514c495f7612463039885efeb8000d5184 Cr-Commit-Position: refs/heads/master@{#438539} |
