Final cleanup pass over audio device API

extensions/browser/api/audio/audio_api.cc

Index: extensions/browser/api/audio/audio_api.cc
diff --git a/extensions/browser/api/audio/audio_api.cc b/extensions/browser/api/audio/audio_api.cc
index dabb853b1a277ce58ee9d4361b662e30a28d5d02..ef4b23a46a37a54cff084126c6007e6f5ec8b061 100644
--- a/extensions/browser/api/audio/audio_api.cc
+++ b/extensions/browser/api/audio/audio_api.cc
@@ -48,14 +48,20 @@ void AudioAPI::OnDeviceChanged() {
 }
 
 void AudioAPI::OnLevelChanged(const std::string& id, int level) {
-  if (EventRouter::Get(browser_context_)) {
-    std::unique_ptr<base::ListValue> args =
-        audio::OnLevelChanged::Create(id, level);
-    std::unique_ptr<Event> event(new Event(events::AUDIO_ON_LEVEL_CHANGED,
-                                           audio::OnLevelChanged::kEventName,
-                                           std::move(args)));
-    EventRouter::Get(browser_context_)->BroadcastEvent(std::move(event));
-  }
+  EventRouter* event_router = EventRouter::Get(browser_context_);
+  if (!event_router)
+    return;
+
+  audio::LevelChangedEvent raw_event;
+  raw_event.device_id = id;
+  raw_event.level = level;
+
+  std::unique_ptr<base::ListValue> event_args =
+      audio::OnLevelChanged::Create(raw_event);
+  std::unique_ptr<Event> event(new Event(events::AUDIO_ON_LEVEL_CHANGED,
+                                         audio::OnLevelChanged::kEventName,
+                                         std::move(event_args)));
+  event_router->BroadcastEvent(std::move(event));
 }
 
 void AudioAPI::OnMuteChanged(bool is_input, bool is_muted) {
@@ -77,14 +83,16 @@ void AudioAPI::OnMuteChanged(bool is_input, bool is_muted) {
 }
 
 void AudioAPI::OnDevicesChanged(const DeviceInfoList& devices) {
-  if (EventRouter::Get(browser_context_)) {
-    std::unique_ptr<base::ListValue> args =
-        audio::OnDevicesChanged::Create(devices);
-    std::unique_ptr<Event> event(new Event(events::AUDIO_ON_DEVICES_CHANGED,
-                                           audio::OnDevicesChanged::kEventName,
-                                           std::move(args)));
-    EventRouter::Get(browser_context_)->BroadcastEvent(std::move(event));
-  }
+  EventRouter* event_router = EventRouter::Get(browser_context_);
+  if (!event_router)
+    return;
+
+  std::unique_ptr<base::ListValue> args =
+      audio::OnDeviceListChanged::Create(devices);
+  std::unique_ptr<Event> event(new Event(events::AUDIO_ON_DEVICES_CHANGED,
+                                         audio::OnDeviceListChanged::kEventName,
+                                         std::move(args)));
+  event_router->BroadcastEvent(std::move(event));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -106,6 +114,26 @@ ExtensionFunction::ResponseAction AudioGetInfoFunction::Run() {
 
 ///////////////////////////////////////////////////////////////////////////////
 
+ExtensionFunction::ResponseAction AudioGetDevicesFunction::Run() {
+  std::unique_ptr<audio::GetDevices::Params> params(
+      audio::GetDevices::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  AudioService* service =
+      AudioAPI::GetFactoryInstance()->Get(browser_context())->GetService();
+  DCHECK(service);
+
+  std::vector<api::audio::AudioDeviceInfo> devices;
+  if (!service->GetDevices(params->filter, &devices)) {
+    return RespondNow(
+        Error("Error occurred when querying audio device information."));
+  }
+
+  return RespondNow(ArgumentList(audio::GetDevices::Results::Create(devices)));
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
 ExtensionFunction::ResponseAction AudioSetActiveDevicesFunction::Run() {
   std::unique_ptr<audio::SetActiveDevices::Params> params(
       audio::SetActiveDevices::Params::Create(*args_));
@@ -141,13 +169,21 @@ ExtensionFunction::ResponseAction AudioSetPropertiesFunction::Run() {
       AudioAPI::GetFactoryInstance()->Get(browser_context())->GetService();
   DCHECK(service);
 
+  int level_value =
+      params->properties.level.get() ? *params->properties.level : -1;

[Devlin, 2017/02/10 20:24:29]

largely unrelated to this cl, but where do we validate that level, volume, and
gain are between 0 and 100?  I don't see a min/max set.

[tbarzic, 2017/02/10 21:05:18]

volume is normalized to 0-100 here:
https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl...

gain is intentionally not normalized (due
https://bugs.chromium.org/p/chromium/issues/detail?id=273872)


(rewrote this to better handle the case level is set to a negative value.)

[Devlin, 2017/02/13 23:18:09]

Ideally, we'd have it set as a minimum/maximum in the bindings, so that the
developers are aware that calling it with anything but 0 <= x <= 100 is invalid.
 Any chance we can do that before moving the API to stable?  (Though doesn't
need to be in this cl)

[tbarzic, 2017/02/14 00:27:13]

I agree, but from what I can see [0, 100] is not the complete set of valid
values, at least not for input devices.

Let me see with jennyz if this could be enforced/changed.

+
   int volume_value = params->properties.volume.get() ?
       *params->properties.volume : -1;
 
   int gain_value = params->properties.gain.get() ?
       *params->properties.gain : -1;
 
-  if (!service->SetDeviceSoundLevel(params->id, volume_value, gain_value))
+  // |volume_value| and |gain_value| are deprecated in favor of |level_value|;
+  // they are kept around only to ensure backward-compatibility and should be
+  // ignored if |level_value| is set.
+  if (!service->SetDeviceSoundLevel(
+          params->id, level_value >= 0 ? level_value : volume_value,
+          level_value >= 0 ? level_value : gain_value))
     return RespondNow(Error("Could not set volume/gain properties"));
 
   if (params->properties.is_muted.get() &&