app_shell: Do simple audio initialization.

app_shell: Do simple audio initialization.

Add a ShellAudioController class that activates audio
devices for input and output and requests max volume.

Also move chromeos::NetworkHandler initialization into
ShellBrowserMainParts so that ShellNetworkController can get
tests at some point.

BUG=405968
Committed: https://crrev.com/3698adff97208879298ad45a0bb07ea13cc5f49c
Cr-Commit-Position: refs/heads/master@{#292643}

[Daniel Erat, 2014-08-27 23:05:29]

derat@chromium.org changed reviewers:
+ jamescook@chromium.org, jennyz@chromium.org

[Daniel Erat, 2014-08-27 23:05:29]

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_browser_main_parts.cc:126:
EnsureBrowserContextKeyedServiceFactoriesBuilt();
i was initially adding some code to the anon namespace in this change, and i
moved the namespace within extensions. that broke this function (since it has
the same name as the version in the extensions namespace). i ended up moving the
code here. let me know if you'd like me to move it back -- i usually prefer to
nest anonymous namespaces inside of the .cc file's main namespace, though, and
it seemed weird to reuse the name like this.

[James Cook, 2014-08-27 23:18:34]

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.cc:107: it !=
devices.rend() && (!best_input || !best_output); ++it) {
Would it be simpler to do two searches, one for best input and one for best
output? That might simplify the logic here.

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.cc:126: }
How hard would it be to add a test for this class? Do we have enough test
support to at least construct an instance and tear it down?

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_audio_controller_chromeos.h (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.h:9: #include
"base/memory/ref_counted.h"
Do you need this include?

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.h:65: // device, and
activates them if they aren't already active.
I like how you always have descriptive function comments.

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_browser_main_parts.cc:68: // Perform
initialization of D-Bus objects here so as to not step on tests'
super nit: What does "step on tests' toes" mean?  Maybe a less colloquial
comment?

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_browser_main_parts.cc:126:
EnsureBrowserContextKeyedServiceFactoriesBuilt();
On 2014/08/27 23:05:29, Daniel Erat wrote:
> i was initially adding some code to the anon namespace in this change, and i
> moved the namespace within extensions. that broke this function (since it has
> the same name as the version in the extensions namespace). i ended up moving
the
> code here. let me know if you'd like me to move it back -- i usually prefer to
> nest anonymous namespaces inside of the .cc file's main namespace, though, and
> it seemed weird to reuse the name like this.

This seems better.

[Daniel Erat, 2014-08-28 00:12:21]

will reupload and ping after tests are done

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.cc:107: it !=
devices.rend() && (!best_input || !best_output); ++it) {
On 2014/08/27 23:18:33, James Cook wrote:
> Would it be simpler to do two searches, one for best input and one for best
> output? That might simplify the logic here.

hmm, that seems like it'd still be more code overall since c++ iterators are so
verbose. we can't use the c++11-style "for (auto &device : devices)" yet, can
we? (i don't know if there's any way to use them with reverse iterators in any
case.)

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.cc:126: }
On 2014/08/27 23:18:33, James Cook wrote:
> How hard would it be to add a test for this class? Do we have enough test
> support to at least construct an instance and tear it down?

probably not very; i'll do this.

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_audio_controller_chromeos.h (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_audio_controller_chromeos.h:9: #include
"base/memory/ref_counted.h"
On 2014/08/27 23:18:33, James Cook wrote:
> Do you need this include?

whoops, not anymore. thanks, deleted.

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/515573003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_browser_main_parts.cc:68: // Perform
initialization of D-Bus objects here so as to not step on tests'
On 2014/08/27 23:18:33, James Cook wrote:
> super nit: What does "step on tests' toes" mean?  Maybe a less colloquial
> comment?

Done.

[Daniel Erat, 2014-08-28 04:00:29]

added tests. okay, it was a bit more work than i expected due to
CrasAudioHandler not having a stub implementation.

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:46: return "HEADPHONE";
these strings seem like they should all be constants in the system_api repo.

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
File chromeos/dbus/cras_audio_client_stub_impl.h (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client_stub_impl.h:41: virtual void
ChangeAudioNodes(const AudioNodeList& new_nodes);
these are called in a bunch of places; let me know if you want me to add
ForTesting suffixes to their names. (since they're only defined in the stub, i
don't think there's much danger of them being misused, though.)

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:111: // TODO(derat):
Need to check |plugged_time|?
jenny, let me know if there's a danger of e.g. unplugged headphone jacks showing
up here.

[jennyz, 2014-08-28 17:42:50]

lgtm with a nit.

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:46: return "HEADPHONE";
On 2014/08/28 04:00:29, Daniel Erat wrote:
> these strings seem like they should all be constants in the system_api repo.

Acknowledged.

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
File chromeos/dbus/cras_audio_client_stub_impl.h (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client_stub_impl.h:41: virtual void
ChangeAudioNodes(const AudioNodeList& new_nodes);
On 2014/08/28 04:00:29, Daniel Erat wrote:
> these are called in a bunch of places; let me know if you want me to add
> ForTesting suffixes to their names. (since they're only defined in the stub, i
> don't think there's much danger of them being misused, though.)

Please add ForTesting suffixes.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:49: // honor changes
that are made at runtime.
Do we have a plan to persist the volume/gain and mute preference set by the app
in the future?

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:111: // TODO(derat):
Need to check |plugged_time|?
On 2014/08/28 04:00:29, Daniel Erat wrote:
> jenny, let me know if there's a danger of e.g. unplugged headphone jacks
showing
> up here.

If the headphone jack is unplugged, it should not be returned in the
GetAudioDevices, and plug time is already counted in AudioDeviceCompare, no need
to check again.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.cc:69: // helper classes so
said classes can be tested.
nit: said->that?

[James Cook, 2014-08-28 18:48:37]

LGTM assuming the DBusThreadManager::Initialize() thing isn't a real issue.

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:26: // Fall through.
Not your bug, but what the heck are these "fall through" comments?

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:45: case chromeos::AUDIO_TYPE_HEADPHONE:
none of these need "chromeos::" anymore

Also, you could move the chromeos namespace above the anonymous namespace and
get rid of some "chromeos::" there as well.

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
File chromeos/audio/audio_device.h (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.h:32: AudioDevice();
Thanks for fixing stuff like this.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:56: void
ShellAudioController::PrefHandler::SetMuteValue(
nit: blank line above

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:110: it !=
devices.rend() && (!best_input || !best_output); ++it) {
optional: this loop might be clearer if the best_input / best_output condition
was moved to the end as a specific break:

for (rbegin; rend; ++it) {
  if (is_input)
    best_input = id
  if (is_output)
    best_output = id
  if (best_input && best_output)
    // we're done
    break
}

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc:35:
chromeos::DBusThreadManager::Shutdown();
Just to double-check since I'm not familiar with this code: We need an explicit
DBusThreadManager::Shutdown() but not a DBusThreadManager::Initialize() or
InitializeForTesting()?

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc:77:
chromeos::AudioNode internal_speaker =
optional: using chromeos::AudioNode and/or AudioNodeList might improve
readability

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc:86: // mic,
so we should start out favoring headphones and the external mic.
nice per-block documentation

[Daniel Erat, 2014-08-28 23:27:57]

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:26: // Fall through.
On 2014/08/28 18:48:37, James Cook wrote:
> Not your bug, but what the heck are these "fall through" comments?

beats me; deleted them

https://codereview.chromium.org/515573003/diff/20001/chromeos/audio/audio_dev...
chromeos/audio/audio_device.cc:45: case chromeos::AUDIO_TYPE_HEADPHONE:
On 2014/08/28 18:48:36, James Cook wrote:
> none of these need "chromeos::" anymore
> 
> Also, you could move the chromeos namespace above the anonymous namespace and
> get rid of some "chromeos::" there as well.

Done.

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
File chromeos/dbus/cras_audio_client_stub_impl.h (right):

https://codereview.chromium.org/515573003/diff/20001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client_stub_impl.h:41: virtual void
ChangeAudioNodes(const AudioNodeList& new_nodes);
On 2014/08/28 17:42:49, jennyz wrote:
> On 2014/08/28 04:00:29, Daniel Erat wrote:
> > these are called in a bunch of places; let me know if you want me to add
> > ForTesting suffixes to their names. (since they're only defined in the stub,
i
> > don't think there's much danger of them being misused, though.)
> 
> Please add ForTesting suffixes.

done (also renamed to SetAudioNodesForTesting and
SetAudioNodesAndNotifyObserversForTesting)

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:49: // honor changes
that are made at runtime.
On 2014/08/28 17:42:49, jennyz wrote:
> Do we have a plan to persist the volume/gain and mute preference set by the
app
> in the future? 

you mean across app_shell runs? we'll probably want to do that at some point,
but i figured i'd just punt on it for now.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:56: void
ShellAudioController::PrefHandler::SetMuteValue(
On 2014/08/28 18:48:37, James Cook wrote:
> nit: blank line above

whoops, done

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:110: it !=
devices.rend() && (!best_input || !best_output); ++it) {
On 2014/08/28 18:48:37, James Cook wrote:
> optional: this loop might be clearer if the best_input / best_output condition
> was moved to the end as a specific break:
> 
> for (rbegin; rend; ++it) {
>   if (is_input)
>     best_input = id
>   if (is_output)
>     best_output = id
>   if (best_input && best_output)
>     // we're done
>     break
> }

i considered that, but it felt weird to me since it means we'll look at the
first device unconditionally. duplicating the check outside of the loop and
putting it at the beginning of the loop also seemed gross.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc:35:
chromeos::DBusThreadManager::Shutdown();
On 2014/08/28 18:48:37, James Cook wrote:
> Just to double-check since I'm not familiar with this code: We need an
explicit
> DBusThreadManager::Shutdown() but not a DBusThreadManager::Initialize() or
> InitializeForTesting()?

(discussed in person:) looks like it. i'll double-check that this is intentional
with zel when he's back at his desk (i think that he's the one who updated the
code that i copy-and-pasted here), but i've added comments here and in
DBusThreadManager for now.

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc:77:
chromeos::AudioNode internal_speaker =
On 2014/08/28 18:48:37, James Cook wrote:
> optional: using chromeos::AudioNode and/or AudioNodeList might improve
> readability

done (note to self: james is a reviewer who's okay with using-directives)

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.cc:69: // helper classes so
said classes can be tested.
On 2014/08/28 17:42:49, jennyz wrote:
> nit: said->that?

"said" means "just-mentioned" in this case -- see the adjective definition at
[define:said]: "used in legal language or humorously to refer to someone or
something already mentioned or named". i've reworded this comment to make it
clearer, though.

[Daniel Erat, 2014-08-28 23:31:24]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-08-28 23:32:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/515573003/40001

[commit-bot: I haz the power, 2014-08-29 04:39:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-29 04:43:28]

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

[commit-bot: I haz the power, 2014-08-29 04:43:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2014-08-29 14:46:56]

wrap line

[Daniel Erat, 2014-08-29 14:48:52]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-08-29 14:50:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/515573003/60001

[commit-bot: I haz the power, 2014-08-29 15:54:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[commit-bot: I haz the power, 2014-08-29 16:11:03]

Committed patchset #4 (id:60001) as 637dc4ebfcecabc3d4cd4b7f86124c145c5ad9d5

[commit-bot: I haz the power, 2014-09-10 03:08:58]

Patchset 4 (id:??) landed as
https://crrev.com/3698adff97208879298ad45a0bb07ea13cc5f49c
Cr-Commit-Position: refs/heads/master@{#292643}