Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(254)

Issue 1339183002: Add localized default audio device names. (Closed)

Created:
5 years, 3 months ago by ajm
Modified:
5 years, 3 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add localized default audio device names. Add localized strings for the default audio device, the communications device on Windows and the beamforming virtual input devices on ChromeOS. Since the media layer can't access Chrome's resource bundle directly, this adds an indirect mechanism modeled on net/base/net_module.h. BUG=497001 TEST=The correct "Default" name appears on Linux in Chrome settings. On a Swanky (which has beamforming support), the correct virtual device names appear in both Chrome settings and the Hangouts settings dialog. Committed: https://crrev.com/943e1b33dac970c6a970d085b66c5c526d6b9870 Cr-Commit-Position: refs/heads/master@{#350070}

Patch Set 1 #

Patch Set 2 : Communications device and fix other platforms. #

Patch Set 3 : Fix descriptions. #

Total comments: 7

Patch Set 4 : Can use static methods. #

Patch Set 5 : Move strings to generated_resources.grd. #

Patch Set 6 : Qualify static method calls. #

Patch Set 7 : Add a media resource provider. #

Patch Set 8 : Add documentation. #

Patch Set 9 : Add default case (to appease the cros compiler). #

Patch Set 10 : Minor fixes. #

Total comments: 16

Patch Set 11 : Use base::Callback. #

Patch Set 12 : Use []. #

Patch Set 13 : Initialize in run_all_unittests. #

Total comments: 4

Patch Set 14 : Revert to function pointer. #

Patch Set 15 : Unneeded includes. #

Total comments: 1

Patch Set 16 : Fix cast_unittests. #

Total comments: 1

Patch Set 17 : Remove multiple initialization check. #

Patch Set 18 : Revert erroneous MEDIA_EXPORT. #

Patch Set 19 : Update strings and descriptions. #

Patch Set 20 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+303 lines, -90 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +18 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/chrome_common.gypi View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/chrome_renderer.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A chrome/common/media/media_resource_provider.h View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments Download
A chrome/common/media/media_resource_provider.cc View 1 2 3 4 5 6 7 8 1 chunk +36 lines, -0 lines 0 comments Download
M chrome/renderer/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/chrome_render_process_observer.cc View 1 2 3 4 5 6 7 11 12 13 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/audio_output_device_enumerator.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M media/audio/alsa/audio_manager_alsa.cc View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M media/audio/android/audio_android_unittest.cc View 1 2 3 4 5 7 chunks +18 lines, -18 lines 0 comments Download
M media/audio/android/audio_manager_android.cc View 1 2 3 1 chunk +9 lines, -7 lines 0 comments Download
M media/audio/audio_manager.h View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M media/audio/audio_manager.cc View 1 2 3 4 5 6 7 8 9 2 chunks +16 lines, -0 lines 0 comments Download
M media/audio/audio_manager_base.h View 1 2 3 4 5 1 chunk +6 lines, -9 lines 0 comments Download
M media/audio/audio_manager_base.cc View 1 2 3 1 chunk +7 lines, -6 lines 0 comments Download
M media/audio/audio_manager_unittest.cc View 1 2 3 11 12 13 2 chunks +2 lines, -4 lines 0 comments Download
M media/audio/audio_output_controller_unittest.cc View 1 2 3 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M media/audio/cras/audio_manager_cras.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -4 lines 0 comments Download
M media/audio/cras/audio_manager_cras.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +22 lines, -33 lines 0 comments Download
M media/audio/mac/audio_manager_mac.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/audio/pulse/audio_manager_pulse.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/audio/win/audio_manager_win.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M media/base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
A media/base/fake_media_resources.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +15 lines, -0 lines 0 comments Download
A media/base/fake_media_resources.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +18 lines, -0 lines 0 comments Download
A media/base/media_resources.h View 1 2 3 4 5 6 7 8 9 11 12 13 14 15 16 1 chunk +47 lines, -0 lines 0 comments Download
A media/base/media_resources.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +27 lines, -0 lines 0 comments Download
M media/base/run_all_unittests.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -0 lines 0 comments Download
M media/cast/cast_testing.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M media/media.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 54 (11 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1339183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1339183002/1
5 years, 3 months ago (2015-09-14 22:59:45 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/132279) mac_chromium_compile_dbg_ng on ...
5 years, 3 months ago (2015-09-14 23:13:44 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1339183002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1339183002/40001
5 years, 3 months ago (2015-09-14 23:50:22 UTC) #6
ajm
aluebs, tommi: everything abodenha: *.grd and please see the UTF-8 question in audio_manager_base.cc. dalecurtis: top-level ...
5 years, 3 months ago (2015-09-15 00:04:25 UTC) #8
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/99650)
5 years, 3 months ago (2015-09-15 00:05:34 UTC) #10
Nico
https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_strings.grd#newcode1317 chrome/app/chromium_strings.grd:1317: </message> chromium_strings.grd / google_chrome_strings.grd are for strings that contain ...
5 years, 3 months ago (2015-09-15 00:12:25 UTC) #11
ajm
https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_strings.grd#newcode1317 chrome/app/chromium_strings.grd:1317: </message> On 2015/09/15 00:12:25, Nico wrote: > chromium_strings.grd / ...
5 years, 3 months ago (2015-09-15 00:52:14 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1339183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1339183002/100001
5 years, 3 months ago (2015-09-15 00:53:20 UTC) #14
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/113459) mac_chromium_rel_ng on ...
5 years, 3 months ago (2015-09-15 01:04:58 UTC) #16
ajm
https://codereview.chromium.org/1339183002/diff/40001/media/DEPS File media/DEPS (right): https://codereview.chromium.org/1339183002/diff/40001/media/DEPS#newcode3 media/DEPS:3: "+chrome/grit", On 2015/09/15 00:12:25, Nico wrote: > media probably ...
5 years, 3 months ago (2015-09-15 05:34:00 UTC) #17
tommi (sloooow) - chröme
On 2015/09/15 05:34:00, ajm wrote: > https://codereview.chromium.org/1339183002/diff/40001/media/DEPS > File media/DEPS (right): > > https://codereview.chromium.org/1339183002/diff/40001/media/DEPS#newcode3 > ...
5 years, 3 months ago (2015-09-15 13:44:30 UTC) #18
DaleCurtis
Yeah depending on chrome/ is a no-go. In this case I'd construct a generic callback ...
5 years, 3 months ago (2015-09-15 16:25:02 UTC) #19
ajm
On 2015/09/15 16:25:02, DaleCurtis wrote: > Yeah depending on chrome/ is a no-go. In this ...
5 years, 3 months ago (2015-09-16 01:34:53 UTC) #20
tommi (sloooow) - chröme
Will that create a race or a period of time where we can't get names? ...
5 years, 3 months ago (2015-09-16 16:43:38 UTC) #21
ajm
On 2015/09/16 16:43:38, tommi wrote: > Will that create a race or a period of ...
5 years, 3 months ago (2015-09-16 16:46:38 UTC) #22
tommi (sloooow) - chröme
Sgtm On Wed, Sep 16, 2015, 18:46 <ajm@chromium.org> wrote: > On 2015/09/16 16:43:38, tommi wrote: ...
5 years, 3 months ago (2015-09-16 16:55:43 UTC) #23
ajm
Approach similar to Dale's suggestion is up. I modeled it after: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_module.h and https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/net/net_resource_provider.h which ...
5 years, 3 months ago (2015-09-17 05:47:28 UTC) #24
tommi (sloooow) - chröme
lgtm
5 years, 3 months ago (2015-09-17 09:03:41 UTC) #25
aluebs-chromium
lgtm
5 years, 3 months ago (2015-09-17 16:36:04 UTC) #26
DaleCurtis
media/ lgtm % two fixes or reasonable answers. https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resources.cc File media/base/media_resources.cc (right): https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resources.cc#newcode13 media/base/media_resources.cc:13: g_localized_string_provider ...
5 years, 3 months ago (2015-09-17 16:37:41 UTC) #27
ajm
https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resources.cc File media/base/media_resources.cc (right): https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resources.cc#newcode13 media/base/media_resources.cc:13: g_localized_string_provider = func; On 2015/09/17 16:37:41, DaleCurtis wrote: > ...
5 years, 3 months ago (2015-09-17 16:52:02 UTC) #28
DaleCurtis
Be careful a static base::Callback global though, since that'll create a static initializer.
5 years, 3 months ago (2015-09-17 16:56:17 UTC) #29
jungshik at Google
LGTM : media's l10n dependency +ainslie : to seek his help on "Default (pick up ...
5 years, 3 months ago (2015-09-17 21:56:44 UTC) #31
ajm
https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd#newcode15462 chrome/app/generated_resources.grd:15462: + Default (pick up everything) On 2015/09/17 21:56:43, jshin ...
5 years, 3 months ago (2015-09-17 22:42:04 UTC) #32
ajm
Dale, could you have another look? Nico, would you mind reviewing chrome/ for OWNERs approval? ...
5 years, 3 months ago (2015-09-18 00:43:21 UTC) #33
Nico
chrome/ lgtm https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_browser_main.cc#newcode1532 chrome/browser/chrome_browser_main.cc:1532: base::Bind(&chrome_common_media::LocalizedStringProvider)); NetResourceProvider doesn't use Bind() – can ...
5 years, 3 months ago (2015-09-18 00:48:23 UTC) #34
DaleCurtis
I think the raw pointer ended up being clearer and the callback adds only overhead ...
5 years, 3 months ago (2015-09-18 01:00:54 UTC) #35
ajm
On 2015/09/18 01:00:54, DaleCurtis wrote: > I think the raw pointer ended up being clearer ...
5 years, 3 months ago (2015-09-18 01:03:32 UTC) #36
ajm
Switched back to a function pointer. ainslie, PTAL at the grd file. https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testing.gypi File media/cast/cast_testing.gypi ...
5 years, 3 months ago (2015-09-18 05:31:57 UTC) #37
Albert Bodenhamer
GRD changes lgtm On Thu, Sep 17, 2015 at 10:31 PM <ajm@chromium.org> wrote: > Switched ...
5 years, 3 months ago (2015-09-18 16:29:21 UTC) #38
Albert Bodenhamer
lgtm
5 years, 3 months ago (2015-09-18 16:30:19 UTC) #39
Nico
lgtm
5 years, 3 months ago (2015-09-18 16:57:03 UTC) #40
ajm
https://codereview.chromium.org/1339183002/diff/250002/media/base/media_resources.cc File media/base/media_resources.cc (right): https://codereview.chromium.org/1339183002/diff/250002/media/base/media_resources.cc#newcode15 media/base/media_resources.cc:15: DCHECK(!g_localized_string_provider); Dale, forgot to mention: I had to remove ...
5 years, 3 months ago (2015-09-18 17:48:58 UTC) #41
DaleCurtis
Hmm unfortunate, but since there's already precedent lgtm
5 years, 3 months ago (2015-09-18 17:56:15 UTC) #42
jungshik at Google
https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd#newcode15462 chrome/app/generated_resources.grd:15462: + Default (pick up everything) On 2015/09/17 22:42:04, ajm ...
5 years, 3 months ago (2015-09-18 22:33:48 UTC) #43
ajm
Thanks jshin. Please have another look at the descriptions. https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_resources.grd#newcode15462 chrome/app/generated_resources.grd:15462: ...
5 years, 3 months ago (2015-09-21 19:49:26 UTC) #44
jungshik at Google
On 2015/09/21 19:49:26, ajm wrote: > Thanks jshin. Please have another look at the descriptions. ...
5 years, 3 months ago (2015-09-21 21:09:29 UTC) #45
ajm
On 2015/09/21 21:09:29, jshin (jungshik at g) wrote: > Looks good to me. Thank you ...
5 years, 3 months ago (2015-09-21 22:23:59 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1339183002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1339183002/360001
5 years, 3 months ago (2015-09-21 22:25:31 UTC) #49
commit-bot: I haz the power
Committed patchset #20 (id:360001)
5 years, 3 months ago (2015-09-22 00:04:16 UTC) #50
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/943e1b33dac970c6a970d085b66c5c526d6b9870 Cr-Commit-Position: refs/heads/master@{#350070}
5 years, 3 months ago (2015-09-22 00:05:04 UTC) #51
dgrogan
This broke iOS_Device_(ninja). "Revert patchset" gives an error -- I'm going to revert manually. http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/builds/22511/steps/compile/logs/stdio ...
5 years, 3 months ago (2015-09-22 00:25:42 UTC) #53
ajm
5 years, 3 months ago (2015-09-22 00:35:43 UTC) #54
Message was sent while issue was closed.
On 2015/09/22 00:25:42, dgrogan wrote:
> This broke iOS_Device_(ninja). "Revert patchset" gives an error -- I'm going
to
> revert manually.
> 
>
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...
> 
> [39/296] CXX obj/media/audio/media.audio_input_ipc.arm64.o
> FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
> obj/media/base/simd/media.convert_yuv_to_rgb_x86.armv7.o.d
> [snip]
> fcolor-diagnostics  -c ../../media/base/simd/convert_yuv_to_rgb_x86.cc -o
> obj/media/base/simd/media.convert_yuv_to_rgb_x86.armv7.o
> In file included from ../../media/base/simd/convert_yuv_to_rgb_x86.cc:8:
>
/b/build/slave/iOS_Device__ninja_/build/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.8.0/include/mmintrin.h:39:5:error:
> use of undeclared identifier '__builtin_ia32_emms'; did you mean
> '__builtin_isless'?
>     __builtin_ia32_emms();

Hmm, strange that it's trying to build x86 simd on iOS. Thanks.

Powered by Google App Engine
This is Rietveld 408576698