Add localized default audio device names.

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}

[ajm, 2015-09-14 22:56:35]

The CQ bit was checked by ajm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 22:59:45]

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

[commit-bot: I haz the power, 2015-09-14 23:13:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 23:13:44]

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_chromiu...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ajm, 2015-09-14 23:49:45]

The CQ bit was checked by ajm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 23:50:22]

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

[ajm, 2015-09-15 00:04:24]

ajm@chromium.org changed reviewers:
+ abodenha@chromium.org, aluebs@chromium.org, dalecurtis@chromium.org,
jshin@chromium.org, thakis@chromium.org, tommi@chromium.org

[ajm, 2015-09-15 00:04:25]

aluebs, tommi: everything

abodenha: *.grd and please see the UTF-8 question in audio_manager_base.cc.

dalecurtis: top-level media/ OWNER
media/BUILD.gn
media/DEPS
media/media.gyp

jshin: adding ui/base/l10n to media/DEPS
thakis: adding chrome/grit to media/DEPS (grit is a generated directory;
apologies if your approval isn't actually needed.)

https://codereview.chromium.org/1339183002/diff/40001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1339183002/diff/40001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:392: return
l10n_util::GetStringUTF8(IDS_DEFAULT_AUDIO_DEVICE_NAME);
The localization guidelines recommend using UTF-16:
"Note that we generally prefer to use UTF-16 encoded strings for user-visible
text."
https://www.chromium.org/developers/design-documents/ui-localization

but all of the audio device name interfaces in Chrome use std::string. Is it
fine to use UTF-8 for this, or should we switch to UTF-16 for audio device names
across the board?

https://codereview.chromium.org/1339183002/diff/40001/media/audio/audio_manag...
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/1339183002/diff/40001/media/audio/audio_manag...
media/audio/audio_manager_base.h:31: // TODO(ajm): Move these strings to
AudioManager.
I indeed intend to do this in a follow-up. Avoided here because it will touch a
large number of files.

https://codereview.chromium.org/1339183002/diff/40001/media/audio/audio_manag...
media/audio/audio_manager_base.h:34: static const char kDefaultDeviceId[];
The DeviceIDs aren't exposed to users so don't need localization.

[commit-bot: I haz the power, 2015-09-15 00:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 00:05:34]

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_presub...)

[Nico, 2015-09-15 00:12:25]

https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1317: </message>
chromium_strings.grd / google_chrome_strings.grd are for strings that contain
the text "chromium" / "chrome". This doesn't seem to be the case here; these
should instead be in chrome/app/generated_resources.grd (or one of the grdp
files included there, if any is a better match) instead?

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",
media probably shouldn't depend on chrome, given that stuff in chrome/ already
depends on media. The good folks in content/browser/resources/media/OWNERS
probably know how resource access in media/ should work.

[ajm, 2015-09-15 00:52:14]

https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1339183002/diff/40001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1317: </message>
On 2015/09/15 00:12:25, Nico wrote:
> chromium_strings.grd / google_chrome_strings.grd are for strings that contain
> the text "chromium" / "chrome". This doesn't seem to be the case here; these
> should instead be in chrome/app/generated_resources.grd (or one of the grdp
> files included there, if any is a better match) instead?

Ah, that makes more sense. Done.

[ajm, 2015-09-15 00:52:38]

The CQ bit was checked by ajm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-15 00:53:20]

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

[commit-bot: I haz the power, 2015-09-15 01:04:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 01:04:58]

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_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ajm, 2015-09-15 05:34:00]

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 shouldn't depend on chrome, given that stuff in chrome/ already
> depends on media. The good folks in content/browser/resources/media/OWNERS
> probably know how resource access in media/ should work.

Good point. The way I have things now, some tests are failing due to the shared
resource bundle being uninitialized. I doubt I should be trying to initialize it
from AudioManager.

Tommi, Dale: do you have any recommendations on how to proceed?

[tommi (sloooow) - chröme, 2015-09-15 13:44:30]

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
> media/DEPS:3: "+chrome/grit",
> On 2015/09/15 00:12:25, Nico wrote:
> > media probably shouldn't depend on chrome, given that stuff in chrome/
already
> > depends on media. The good folks in content/browser/resources/media/OWNERS
> > probably know how resource access in media/ should work.
> 
> Good point. The way I have things now, some tests are failing due to the
shared
> resource bundle being uninitialized. I doubt I should be trying to initialize
it
> from AudioManager.
> 
> Tommi, Dale: do you have any recommendations on how to proceed?

I think we need a .grd file in media/.  It feels like overkill for only a couple
of strings, but I think that's what we need to do to not break the dependencies.

[DaleCurtis, 2015-09-15 16:25:02]

Yeah depending on chrome/ is a no-go. In this case I'd construct a generic
callback that returns an std::string from an enum value (duplicate the IDS
values in media/). You can then pass this into AudioManager in chrome/ since
it's a global singleton. Make sure you add static_assert's in chrome/ to keep
these in sync.

[ajm, 2015-09-16 01:34:53]

On 2015/09/15 16:25:02, DaleCurtis wrote:
> Yeah depending on chrome/ is a no-go. In this case I'd construct a generic
> callback that returns an std::string from an enum value (duplicate the IDS
> values in media/). You can then pass this into AudioManager in chrome/ since
> it's a global singleton. Make sure you add static_assert's in chrome/ to keep
> these in sync.

Thanks for the advice; looks like we can do it this way.

[tommi (sloooow) - chröme, 2015-09-16 16:43:38]

Will that create a race or a period of time where we can't get names?

On Wed, Sep 16, 2015, 03:34  <ajm@chromium.org> wrote:

> On 2015/09/15 16:25:02, DaleCurtis wrote:
> > Yeah depending on chrome/ is a no-go. In this case I'd construct a
> generic
> > callback that returns an std::string from an enum value (duplicate the
> IDS
> > values in media/). You can then pass this into AudioManager in chrome/
> > since
> > it's a global singleton. Make sure you add static_assert's in chrome/ to
> > keep
> > these in sync.
>
> Thanks for the advice; looks like we can do it this way.
>
> https://codereview.chromium.org/1339183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2015-09-16 16:46:38]

On 2015/09/16 16:43:38, tommi wrote:
> Will that create a race or a period of time where we can't get names?

That's a potential concern. I think I've found a place early enough in Chrome's
startup to alleviate it. I'll ping when I have a new patchset up.

[tommi (sloooow) - chröme, 2015-09-16 16:55:43]

Sgtm

On Wed, Sep 16, 2015, 18:46  <ajm@chromium.org> wrote:

> On 2015/09/16 16:43:38, tommi wrote:
> > Will that create a race or a period of time where we can't get names?
>
> That's a potential concern. I think I've found a place early enough in
> Chrome's
> startup to alleviate it. I'll ping when I have a new patchset up.
>
> https://codereview.chromium.org/1339183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2015-09-17 05:47:28]

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/...

which is solving the same problem.

https://codereview.chromium.org/1339183002/diff/170001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/renderer/chrome...
chrome/renderer/chrome_render_process_observer.cc:256:
media::SetLocalizedStringProvider(
I'm not sure if this is needed, but I added it everywhere
NetModule::SetResourceProvider was called.

https://codereview.chromium.org/1339183002/diff/170001/media/audio/audio_outp...
File media/audio/audio_output_controller_unittest.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/media/audio/audio_outp...
media/audio/audio_output_controller_unittest.cc:93: base::string16
FakeLocalizedStringProvider(MessageId message_id) {
I could move this to a fake_media_resources.h if we think it would be more
broadly useful. This is the only test that seemed to need it set. (It was
hanging otherwise).

[tommi (sloooow) - chröme, 2015-09-17 09:03:41]

lgtm

[aluebs-chromium, 2015-09-17 16:36:04]

lgtm

[DaleCurtis, 2015-09-17 16:37:41]

media/ lgtm % two fixes or reasonable answers.

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
File media/base/media_resources.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
media/base/media_resources.cc:13: g_localized_string_provider = func;
DCHECK that one isn't already set?

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
File media/base/media_resources.h (right):

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
media/base/media_resources.h:34: using LocalizedStringProvider = base::string16
(*)(MessageId message_id);
Why not using a base::Callback? We don't typically pass function pointers around
like this.

[ajm, 2015-09-17 16:52:02]

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
File media/base/media_resources.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
media/base/media_resources.cc:13: g_localized_string_provider = func;
On 2015/09/17 16:37:41, DaleCurtis wrote:
> DCHECK that one isn't already set?

Will do.

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
File media/base/media_resources.h (right):

https://codereview.chromium.org/1339183002/diff/170001/media/base/media_resou...
media/base/media_resources.h:34: using LocalizedStringProvider = base::string16
(*)(MessageId message_id);
On 2015/09/17 16:37:41, DaleCurtis wrote:
> Why not using a base::Callback? We don't typically pass function pointers
around
> like this.

The only reason was to follow net_module's example. I'll switch to
base::Callback.

[DaleCurtis, 2015-09-17 16:56:17]

Be careful a static base::Callback global though, since that'll create a static
initializer.

[jungshik at Google, 2015-09-17 21:56:43]

jshin@chromium.org changed reviewers:
+ ainslie@chromium.org

[jungshik at Google, 2015-09-17 21:56:44]

LGTM : media's l10n dependency 

+ainslie : to seek his help on "Default (pick up everything)" and "Default (pick
up one person)".

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15462: +          Default (pick up
everything)
I wonder if we can come up with 'better' wording than 'pick up {one person,
everything}'. Translators often do not read this long description and may
translate "(...)" to something unrelated. 

+ainslie for that.

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
File chrome/common/media/media_resource_provider.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
chrome/common/media/media_resource_provider.cc:33: return
l10n_util::GetStringUTF16(MediaMessageIdToGrdId(message_id));
It's more a note to myself than a comment on this cl. For more complex messages,
to avoid adding more wrappers like this here (and net/), methods in l10n_util
had better be split into a method (getting a string from res bundle) and
formatting methods.

[ajm, 2015-09-17 22:42:04]

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15462: +          Default (pick up
everything)
On 2015/09/17 21:56:43, jshin (jungshik at g) wrote:
> I wonder if we can come up with 'better' wording than 'pick up {one person,
> everything}'. Translators often do not read this long description and may
> translate "(...)" to something unrelated. 
> 
> +ainslie for that.

Happy to entertain suggestions. See crbug.com/497001 for the discussion,
specifically #17 - #21.

Also let me know if a more concise description would help.

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
File chrome/common/media/media_resource_provider.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
chrome/common/media/media_resource_provider.cc:33: return
l10n_util::GetStringUTF16(MediaMessageIdToGrdId(message_id));
On 2015/09/17 21:56:43, jshin (jungshik at g) wrote:
> It's more a note to myself than a comment on this cl. For more complex
messages,
> to avoid adding more wrappers like this here (and net/), methods in l10n_util
> had better be split into a method (getting a string from res bundle) and
> formatting methods.

I'm probably misunderstanding, but I'm not sure of the relevance to this case.
This wrapper exists simply because we can't depend on chrome/ in media/.

[ajm, 2015-09-18 00:43:21]

Dale, could you have another look?

Nico, would you mind reviewing chrome/ for OWNERs approval?

https://codereview.chromium.org/1339183002/diff/170001/media/audio/audio_outp...
File media/audio/audio_output_controller_unittest.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/media/audio/audio_outp...
media/audio/audio_output_controller_unittest.cc:93: base::string16
FakeLocalizedStringProvider(MessageId message_id) {
On 2015/09/17 05:47:28, ajm wrote:
> I could move this to a fake_media_resources.h if we think it would be more
> broadly useful. This is the only test that seemed to need it set. (It was
> hanging otherwise).

Ended up doing this because there was another test that needed it.

https://codereview.chromium.org/1339183002/diff/170001/media/audio/cras/audio...
File media/audio/cras/audio_manager_cras.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/media/audio/cras/audio...
media/audio/cras/audio_manager_cras.cc:98:
beamforming_on_device_name_(BeamformingOnDeviceName()),
Initializing these at construction was too early and ended up in empty strings.

Instead I now have static defaults for the ID strings (there was really no
reason they had to depend on the default ID) and query the names in
GetAudioInputDeviceNames. This is consistent with the default device handling.

https://codereview.chromium.org/1339183002/diff/230001/media/base/media_resou...
File media/base/media_resources.cc (right):

https://codereview.chromium.org/1339183002/diff/230001/media/base/media_resou...
media/base/media_resources.cc:15: DCHECK(g_localized_string_provider ==
nullptr);
Not sure if this is a common pattern with LazyInstance, but it has an overloaded
operator== that supports it.

https://codereview.chromium.org/1339183002/diff/230001/media/base/run_all_uni...
File media/base/run_all_unittests.cc (right):

https://codereview.chromium.org/1339183002/diff/230001/media/base/run_all_uni...
media/base/run_all_unittests.cc:51: media::SetUpFakeMediaResources();
We need this here to avoid calling SetLocalizedStringProvider more than once.

[Nico, 2015-09-18 00:48:23]

chrome/ lgtm

https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1532:
base::Bind(&chrome_common_media::LocalizedStringProvider));
NetResourceProvider doesn't use Bind() – can this be consistent with that?

[DaleCurtis, 2015-09-18 01:00:54]

I think the raw pointer ended up being clearer and the callback adds only
overhead unfortunately. Sorry for steering you astray. I was hoping the lazy
instance was avoidable some how, but it seems not.

Lets go with your original raw pointer; lgtm

https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1339183002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1532:
base::Bind(&chrome_common_media::LocalizedStringProvider));
On 2015/09/18 00:48:23, Nico wrote:
> NetResourceProvider doesn't use Bind() – can this be consistent with
that?

Heh, I recommended he use bind instead, but now we have a lazy instance for
managing the callback which I was hoping was avoidable.

[ajm, 2015-09-18 01:03:32]

On 2015/09/18 01:00:54, DaleCurtis wrote:
> I think the raw pointer ended up being clearer and the callback adds only
> overhead unfortunately. Sorry for steering you astray. I was hoping the lazy
> instance was avoidable some how, but it seems not.
> 
> Lets go with your original raw pointer; lgtm

No worries, will revert.

[ajm, 2015-09-18 05:31:57]

Switched back to a function pointer.

ainslie, PTAL at the grd file.

https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testin...
File media/cast/cast_testing.gypi (right):

https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testin...
media/cast/cast_testing.gypi:77: '<(DEPTH)/media/base/run_all_unittests.cc',
The GN version doesn't use this.

[Albert Bodenhamer, 2015-09-18 16:29:21]

GRD changes lgtm

On Thu, Sep 17, 2015 at 10:31 PM <ajm@chromium.org> wrote:

> Switched back to a function pointer.
>
> ainslie, PTAL at the grd file.
>
>
>
>
https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testin...
> File media/cast/cast_testing.gypi (right):
>
>
>
https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testin...
> media/cast/cast_testing.gypi:77
>
<https://codereview.chromium.org/1339183002/diff/280001/media/cast/cast_testin...>
> :
> '<(DEPTH)/media/base/run_all_unittests.cc',
> The GN version doesn't use this.
>
> https://codereview.chromium.org/1339183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Albert Bodenhamer, 2015-09-18 16:30:19]

lgtm

[Nico, 2015-09-18 16:57:03]

lgtm

[ajm, 2015-09-18 17:48:58]

https://codereview.chromium.org/1339183002/diff/250002/media/base/media_resou...
File media/base/media_resources.cc (right):

https://codereview.chromium.org/1339183002/diff/250002/media/base/media_resou...
media/base/media_resources.cc:15: DCHECK(!g_localized_string_provider);
Dale, forgot to mention: I had to remove the DCHECK because some browser tests
are initializing things more than once through
chrome_render_process_observer.cc. The net/ version doesn't have the check, so I
think we're OK.

[DaleCurtis, 2015-09-18 17:56:15]

Hmm unfortunate, but since there's already precedent lgtm

[jungshik at Google, 2015-09-18 22:33:48]

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15462: +          Default (pick up
everything)
On 2015/09/17 22:42:04, ajm wrote:
> On 2015/09/17 21:56:43, jshin (jungshik at g) wrote:
> > I wonder if we can come up with 'better' wording than 'pick up {one person,
> > everything}'. Translators often do not read this long description and may
> > translate "(...)" to something unrelated. 
> > 
> > +ainslie for that.
> 
> Happy to entertain suggestions. See crbug.com/497001 for the discussion,
> specifically #17 - #21.

Thank you for the reference. For me, 'listen to {everything, just you}' seems
better in terms of the chance of being translated correctly. 
What we can do is to use 'pick up ....' as is now, but change the description
(hoping that translators read the description. They really should, but not
always). 
 
> Also let me know if a more concise description would help.

To translators, 'beamforming audio processing en/disabled' (well, they can be
pretty tech-savvy, too, but ...) may not help much. How about this?

* Default audio input device (microphone) with a narrow angle coverage. Only the
user in front of the device (i.e. you) will be listened to. This is typically
presented to users in a drop-down list of device choices.
* Default audio input device (microphone) with a wide angle coverage. All the
sounds in the vicinity will be picked up. This is typically presented to users
in a drop-down list of device choices.

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
File chrome/common/media/media_resource_provider.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
chrome/common/media/media_resource_provider.cc:33: return
l10n_util::GetStringUTF16(MediaMessageIdToGrdId(message_id));
On 2015/09/17 22:42:04, ajm wrote:
> On 2015/09/17 21:56:43, jshin (jungshik at g) wrote:
> > It's more a note to myself than a comment on this cl. For more complex
> messages,
> > to avoid adding more wrappers like this here (and net/), methods in
l10n_util
> > had better be split into a method (getting a string from res bundle) and
> > formatting methods.
> 
> I'm probably misunderstanding, but I'm not sure of the relevance to this case.
> This wrapper exists simply because we can't depend on chrome/ in media/. 

Sorry that I was not clear. Nothing wrong with the current change. 

In the future, if media/ or net/ require more than just a string with no
formatting (i.e. they require strings to be formatted with one or more
parameters), we need to separate 

// I simplified a function prototype (it's a bit longer than this).
// there are versions of this taking up to 4 args  
string GetStringF(int msgid, string arg1, string arg2); 

to two parts 

string GetString(int msgid); 
string GetStringFormatWithMsg(string msg, string arg1, string arg2); 

The former requires an access to the resource bundle and needs a wrapper, but
the latter does not. 

So with your wrapper (StringProvider), you get |msg| and pass it to the latter
to get a formatted result. For complex messages (plural, time, date, etc), what
corresponds to the latter is already available in  base/i18n/message_formatter.h
and you can use the above pattern (if/when there arises a need) without doing
any more change.

[ajm, 2015-09-21 19:49:26]

Thanks jshin. Please have another look at the descriptions.

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15462: +          Default (pick up
everything)
On 2015/09/18 22:33:48, jshin (jungshik at g) wrote:
> Thank you for the reference. For me, 'listen to {everything, just you}' seems
> better in terms of the chance of being translated correctly. 
> What we can do is to use 'pick up ....' as is now, but change the description
> (hoping that translators read the description. They really should, but not
> always). 
>  
> > Also let me know if a more concise description would help.
> 
> To translators, 'beamforming audio processing en/disabled' (well, they can be
> pretty tech-savvy, too, but ...) may not help much. How about this?
> 
> * Default audio input device (microphone) with a narrow angle coverage. Only
the
> user in front of the device (i.e. you) will be listened to. This is typically
> presented to users in a drop-down list of device choices.
> * Default audio input device (microphone) with a wide angle coverage. All the
> sounds in the vicinity will be picked up. This is typically presented to users
> in a drop-down list of device choices. 

Sounds good, except that I also used "listened to" rather than "picked up" in
the second description, which I think was your intention.

To be clear, I also now changed "Default (pick up just one person)" to "Default
(pick up just you)". I think the former is closer to what will really happen,
but the latter is probably more clear for users. What do you think?

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
File chrome/common/media/media_resource_provider.cc (right):

https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
chrome/common/media/media_resource_provider.cc:33: return
l10n_util::GetStringUTF16(MediaMessageIdToGrdId(message_id));
On 2015/09/18 22:33:48, jshin (jungshik at g) wrote:
> Sorry that I was not clear. Nothing wrong with the current change. 
> 
> In the future, if media/ or net/ require more than just a string with no
> formatting (i.e. they require strings to be formatted with one or more
> parameters), we need to separate 
> 
> // I simplified a function prototype (it's a bit longer than this).
> // there are versions of this taking up to 4 args  
> string GetStringF(int msgid, string arg1, string arg2); 
> 
> to two parts 
> 
> string GetString(int msgid); 
> string GetStringFormatWithMsg(string msg, string arg1, string arg2); 
> 
> The former requires an access to the resource bundle and needs a wrapper, but
> the latter does not. 
> 
> So with your wrapper (StringProvider), you get |msg| and pass it to the latter
> to get a formatted result. For complex messages (plural, time, date, etc),
what
> corresponds to the latter is already available in 
base/i18n/message_formatter.h
> and you can use the above pattern (if/when there arises a need) without doing
> any more change. 

Ah, I get it. Thanks.

[jungshik at Google, 2015-09-21 21:09:29]

On 2015/09/21 19:49:26, ajm wrote:
> Thanks jshin. Please have another look at the descriptions.
> 
>
https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1339183002/diff/170001/chrome/app/generated_r...
> chrome/app/generated_resources.grd:15462: +          Default (pick up
> everything)
> On 2015/09/18 22:33:48, jshin (jungshik at g) wrote:
> > Thank you for the reference. For me, 'listen to {everything, just you}'
seems
> > better in terms of the chance of being translated correctly. 
> > What we can do is to use 'pick up ....' as is now, but change the
description
> > (hoping that translators read the description. They really should, but not
> > always). 
> >  
> > > Also let me know if a more concise description would help.
> > 
> > To translators, 'beamforming audio processing en/disabled' (well, they can
be
> > pretty tech-savvy, too, but ...) may not help much. How about this?
> > 
> > * Default audio input device (microphone) with a narrow angle coverage. Only
> the
> > user in front of the device (i.e. you) will be listened to. This is
typically
> > presented to users in a drop-down list of device choices.
> > * Default audio input device (microphone) with a wide angle coverage. All
the
> > sounds in the vicinity will be picked up. This is typically presented to
users
> > in a drop-down list of device choices. 
> 
> Sounds good, except that I also used "listened to" rather than "picked up" in
> the second description, which I think was your intention.
> 
> To be clear, I also now changed "Default (pick up just one person)" to
"Default
> (pick up just you)". I think the former is closer to what will really happen,
> but the latter is probably more clear for users. What do you think?

Looks good to me. Thank you for accommodating my suggestion. 
 
>
https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
> File chrome/common/media/media_resource_provider.cc (right):
> 
>
https://codereview.chromium.org/1339183002/diff/170001/chrome/common/media/me...
> chrome/common/media/media_resource_provider.cc:33: return
> l10n_util::GetStringUTF16(MediaMessageIdToGrdId(message_id));

[ajm, 2015-09-21 22:23:59]

On 2015/09/21 21:09:29, jshin (jungshik at g) wrote:
> Looks good to me. Thank you for accommodating my suggestion.

You're welcome!

[ajm, 2015-09-21 22:24:11]

The CQ bit was checked by ajm@chromium.org

[ajm, 2015-09-21 22:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
aluebs@chromium.org, jshin@chromium.org, thakis@chromium.org,
dalecurtis@chromium.org, abodenha@chromium.org
Link to the patchset: https://codereview.chromium.org/1339183002/#ps360001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-09-21 22:25:31]

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

[commit-bot: I haz the power, 2015-09-22 00:04:16]

Committed patchset #20 (id:360001)

[commit-bot: I haz the power, 2015-09-22 00:05:04]

Patchset 20 (id:??) landed as
https://crrev.com/943e1b33dac970c6a970d085b66c5c526d6b9870
Cr-Commit-Position: refs/heads/master@{#350070}

[dgrogan, 2015-09-22 00:25:41]

dgrogan@chromium.org changed reviewers:
+ dgrogan@chromium.org

[dgrogan, 2015-09-22 00:25:42]

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();

[ajm, 2015-09-22 00:35:43]

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.