Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795933002:
cros: Using virtual audio device label for internal device (Closed)
|
Created:
3 years, 8 months ago by Qiang(Joe) Xu Modified:
3 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Using virtual audio device label for internal device
Changes:
Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node.
After discussion with UX writer and reviewer:
Decided using "Built-in-mic" for input device, and "Built-in-speaker" for output device.
BUG=658048
TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2795933002
Cr-Commit-Position: refs/heads/master@{#461854}
Committed: https://chromium.googlesource.com/chromium/src/+/188483ae0dc7746fd2bc8d8ebdc2a57b33769007
Patch Set 1 #
Total comments: 2
Patch Set 2 : Built-in-mic and Built-in-speaker #Messages
Total messages: 16 (8 generated)
Description was changed from ========== cros: Using virtual audio device label for internal device ========== to ========== cros: Using virtual audio device label for internal device CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== cros: Using virtual audio device label for internal device CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer: Decided using "Built-in microphone" for input device, and "Built-in speaker" for output device. BUG=658048 TEST=the bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
warx@chromium.org changed reviewers: + dgreid@chromium.org, tommi@chromium.org
Hi all, PTAL, thanks! Code is extracted from https://codereview.chromium.org/2516813002/, which was already reviewed once.
Description was changed from ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer: Decided using "Built-in microphone" for input device, and "Built-in speaker" for output device. BUG=658048 TEST=the bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer: Decided using "Built-in microphone" for input device, and "Built-in speaker" for output device. BUG=658048 TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... media/audio/cras/audio_manager_cras.cc:52: const char kInternalInputDevice[] = "Built-in microphone"; I'd prefer names that are intentionally not like what could be used in UI, since that should go through translation. What about "built-in-mic" and "built-in-speaker"?
logic looks right to me. No comment on the naming:)
https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... media/audio/cras/audio_manager_cras.cc:52: const char kInternalInputDevice[] = "Built-in microphone"; On 2017/04/04 10:50:08, tommi ooo - chröme wrote: > I'd prefer names that are intentionally not like what could be used in UI, since > that should go through translation. > What about "built-in-mic" and "built-in-speaker"? If that is the concern, sgtm. I think 'b' should be capitalized to match the styles of other labels. WDYT?
On 2017/04/04 18:46:24, Qiang(Joe) Xu wrote: > https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... > File media/audio/cras/audio_manager_cras.cc (right): > > https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... > media/audio/cras/audio_manager_cras.cc:52: const char kInternalInputDevice[] = > "Built-in microphone"; > On 2017/04/04 10:50:08, tommi ooo - chröme wrote: > > I'd prefer names that are intentionally not like what could be used in UI, > since > > that should go through translation. > > What about "built-in-mic" and "built-in-speaker"? > > If that is the concern, sgtm. I think 'b' should be capitalized to match the > styles of other labels. WDYT? Sgtm, lgtm. My only concern is that the string won't indicate that it's for UI use.
On 2017/04/04 20:10:16, tommi ooo - chröme wrote: > On 2017/04/04 18:46:24, Qiang(Joe) Xu wrote: > > > https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... > > File media/audio/cras/audio_manager_cras.cc (right): > > > > > https://codereview.chromium.org/2795933002/diff/1/media/audio/cras/audio_mana... > > media/audio/cras/audio_manager_cras.cc:52: const char kInternalInputDevice[] = > > "Built-in microphone"; > > On 2017/04/04 10:50:08, tommi ooo - chröme wrote: > > > I'd prefer names that are intentionally not like what could be used in UI, > > since > > > that should go through translation. > > > What about "built-in-mic" and "built-in-speaker"? > > > > If that is the concern, sgtm. I think 'b' should be capitalized to match the > > styles of other labels. WDYT? > > Sgtm, lgtm. My only concern is that the string won't indicate that it's for UI > use. I am not quite sure about your concern. The label should only be accessed by javascript on UI. Let us land it for now since it is on dev channel we could collect feedback. I am also going to have a sync with mnilsson@.
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer: Decided using "Built-in microphone" for input device, and "Built-in speaker" for output device. BUG=658048 TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer and reviewer: Decided using "Built-in-mic" for input device, and "Built-in-speaker" for output device. BUG=658048 TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491337392995720,
"parent_rev": "7158183baeda4bc1da23d319def6a2ebab11b3bb", "commit_rev":
"188483ae0dc7746fd2bc8d8ebdc2a57b33769007"}
Message was sent while issue was closed.
Description was changed from ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer and reviewer: Decided using "Built-in-mic" for input device, and "Built-in-speaker" for output device. BUG=658048 TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== cros: Using virtual audio device label for internal device Changes: Now that cras could remember last active audio node for pinning stream. crbug.com/658048 could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node. After discussion with UX writer and reviewer: Decided using "Built-in-mic" for input device, and "Built-in-speaker" for output device. BUG=658048 TEST=Test on Chrome OS 9426.0.0. The bug in crbug.com/658048 is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2795933002 Cr-Commit-Position: refs/heads/master@{#461854} Committed: https://chromium.googlesource.com/chromium/src/+/188483ae0dc7746fd2bc8d8ebdc2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/188483ae0dc7746fd2bc8d8ebdc2... |
