Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2970053002:
Verify that AudioInputDeviceManage stores valid parameters if there are no audio devices in the sys… (Closed)
|
Created:
3 years, 5 months ago by o1ka Modified:
3 years, 5 months ago Reviewers:
Guido Urdaneta CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionVerify that AudioInputDeviceManage stores valid parameters if there are no audio devices in the system
BUG=738350
Review-Url: https://codereview.chromium.org/2970053002
Cr-Commit-Position: refs/heads/master@{#484278}
Committed: https://chromium.googlesource.com/chromium/src/+/1d1b41b50ba5cdb8b3c5739c1c9193c466cb5cf9
Patch Set 1 #
Total comments: 3
Patch Set 2 : nit fixes #Messages
Total messages: 17 (8 generated)
The CQ bit was checked by olka@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...
olka@chromium.org changed reviewers: + guidou@chromium.org
Guido - PTAL. I'm not quite sure how AudioInputDeviceManager is supposed to treat an absence of the device?
https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:313: devices_.emplace_back(MEDIA_TAB_AUDIO_CAPTURE, "Tab capture", If this is about no devices, why are there three devices here, including a MEDIA_DEVICE_AUDIO_CAPTURE one? Also, can this be done as just another test in AudioInputDeviceManagerTest?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:313: devices_.emplace_back(MEDIA_TAB_AUDIO_CAPTURE, "Tab capture", On 2017/07/05 09:37:12, Guido Urdaneta wrote: > If this is about no devices, why are there three devices here, including a > MEDIA_DEVICE_AUDIO_CAPTURE one? > There are no physical devices: we use a mock audio manager. (See the bug description which refers to https://bugs.chromium.org/p/chromium/issues/detail?id=733356) > Also, can this be done as just another test in AudioInputDeviceManagerTest? Because we are mocking absence of physical devices, and AudioInputDeviceManagerTest uses fake devices.
https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2970053002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:313: devices_.emplace_back(MEDIA_TAB_AUDIO_CAPTURE, "Tab capture", On 2017/07/05 10:10:31, o1ka wrote: > On 2017/07/05 09:37:12, Guido Urdaneta wrote: > > If this is about no devices, why are there three devices here, including a > > MEDIA_DEVICE_AUDIO_CAPTURE one? > > > > There are no physical devices: we use a mock audio manager. > (See the bug description which refers to > https://bugs.chromium.org/p/chromium/issues/detail?id=733356) > > > Also, can this be done as just another test in AudioInputDeviceManagerTest? > > Because we are mocking absence of physical devices, and > AudioInputDeviceManagerTest uses fake devices. Add a comment in the initialization of |audio_manager_| saying that this mock manager simulates a manager without devices. Also explain that you use |devices_| as application-provided (not manager-controlled) devices. Also consider renaming |devices_| since my first thought was that it was a list of devices managed by |audio_manager_|. Even better, don't use |devices_| at all in Initialize() and just define an array/vector of devices in ParametersValidWithoutAudioDevices. Optional: rename |manager_| to |audio_input_device_manager_| since it is not the only manager in the test.
PTAL
lgtm
On 2017/07/05 14:37:02, Guido Urdaneta wrote: > lgtm Thanks!
The CQ bit was checked by olka@chromium.org
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": 20001, "attempt_start_ts": 1499265447097500,
"parent_rev": "58b9a6fd073f5185834c049e8cf45caedf29bdd7", "commit_rev":
"1d1b41b50ba5cdb8b3c5739c1c9193c466cb5cf9"}
Message was sent while issue was closed.
Description was changed from ========== Verify that AudioInputDeviceManage stores valid parameters if there are no audio devices in the system BUG=738350 ========== to ========== Verify that AudioInputDeviceManage stores valid parameters if there are no audio devices in the system BUG=738350 Review-Url: https://codereview.chromium.org/2970053002 Cr-Commit-Position: refs/heads/master@{#484278} Committed: https://chromium.googlesource.com/chromium/src/+/1d1b41b50ba5cdb8b3c5739c1c91... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1d1b41b50ba5cdb8b3c5739c1c91... |
