Switching WebRtc private API from AudioManager to AudioSystem

chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc

Index: chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
diff --git a/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc b/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
index 650bab58be958f2310a357ef55b6283d678acbfe..f58aea726be64e65a4e28ec9ba5b4aa5e810e36f 100644
--- a/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
+++ b/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
@@ -20,17 +20,13 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/permissions/permissions_data.h"
-#include "media/audio/audio_device_description.h"
+#include "media/audio/audio_system.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
 namespace extensions {
 
 using content::BrowserThread;
-using content::RenderProcessHost;
-using media::AudioDeviceDescriptions;
-using media::AudioManager;
-
 namespace wap = api::webrtc_audio_private;
 
 using api::webrtc_audio_private::RequestInfo;
@@ -76,7 +72,6 @@ void WebrtcAudioPrivateEventService::OnDevicesChanged(
     case base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE:
       SignalEvent();
       break;
-
     default:
       // No action needed.
       break;
@@ -105,58 +100,9 @@ void WebrtcAudioPrivateEventService::SignalEvent() {
 
 WebrtcAudioPrivateFunction::WebrtcAudioPrivateFunction() {}
 
-WebrtcAudioPrivateFunction::~WebrtcAudioPrivateFunction() {
-}
-
-void WebrtcAudioPrivateFunction::GetOutputDeviceDescriptions() {
-  scoped_refptr<base::SingleThreadTaskRunner> audio_manager_runner =
-      AudioManager::Get()->GetTaskRunner();
-  if (!audio_manager_runner->BelongsToCurrentThread()) {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    audio_manager_runner->PostTask(
-        FROM_HERE,
-        base::Bind(&WebrtcAudioPrivateFunction::GetOutputDeviceDescriptions,
-                   this));
-    return;
-  }
-
-  std::unique_ptr<AudioDeviceDescriptions> device_descriptions =
-      base::MakeUnique<AudioDeviceDescriptions>();
-  AudioManager::Get()->GetAudioOutputDeviceDescriptions(
-      device_descriptions.get());
+WebrtcAudioPrivateFunction::~WebrtcAudioPrivateFunction() {}
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&WebrtcAudioPrivateFunction::OnOutputDeviceDescriptions, this,
-                 base::Passed(&device_descriptions)));
-}
-
-void WebrtcAudioPrivateFunction::OnOutputDeviceDescriptions(
-    std::unique_ptr<AudioDeviceDescriptions> device_descriptions) {
-  NOTREACHED();
-}
-
-void WebrtcAudioPrivateFunction::CalculateHMAC(const std::string& raw_id) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&WebrtcAudioPrivateFunction::CalculateHMAC, this, raw_id));
-    return;
-  }
-
-  std::string hmac = CalculateHMACImpl(raw_id);
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&WebrtcAudioPrivateFunction::OnHMACCalculated, this, hmac));
-}
-
-void WebrtcAudioPrivateFunction::OnHMACCalculated(const std::string& hmac) {
-  NOTREACHED();
-}
-
-std::string WebrtcAudioPrivateFunction::CalculateHMACImpl(
+std::string WebrtcAudioPrivateFunction::CalculateHMAC(
     const std::string& raw_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -183,136 +129,124 @@ std::string WebrtcAudioPrivateFunction::device_id_salt() const {
 
 bool WebrtcAudioPrivateGetSinksFunction::RunAsync() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
   InitDeviceIDSalt();
-  GetOutputDeviceDescriptions();
-
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &WebrtcAudioPrivateGetSinksFunction::GetOutputDeviceDescriptions,
+          this));
   return true;
 }
 
-void WebrtcAudioPrivateGetSinksFunction::OnOutputDeviceDescriptions(
-    std::unique_ptr<AudioDeviceDescriptions> raw_ids) {
+void WebrtcAudioPrivateGetSinksFunction::GetOutputDeviceDescriptions() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  media::AudioSystem::Get()->GetDeviceDescriptions(
+      base::Bind(
+          &WebrtcAudioPrivateGetSinksFunction::OnOutputDeviceDescriptions,
+          this),
+      false);
+}
 
-  std::vector<wap::SinkInfo> results;
-  for (const media::AudioDeviceDescription& description : *raw_ids) {
+void WebrtcAudioPrivateGetSinksFunction::OnOutputDeviceDescriptions(
+    media::AudioDeviceDescriptions sink_devices) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  std::unique_ptr<SyncInfoVector> results = base::MakeUnique<SyncInfoVector>();

[Devlin, 2017/04/06 01:42:33]

optional: I sometimes find it cleaner to do:
auto results = base::MakeUnique<SyncInfoVector>();

The type can't be anything other than std::unique_ptr<T> using
base::MakeUnique<T>, so auto just saves a bit of reading. :)

Though maybe moot with below.

[o1ka, 2017/04/06 11:01:22]

Makes sense!
Done.

+  for (const media::AudioDeviceDescription& description : sink_devices) {
     wap::SinkInfo info;
-    info.sink_id = CalculateHMACImpl(description.unique_id);
+    info.sink_id = CalculateHMAC(description.unique_id);
     info.sink_label = description.device_name;
     // TODO(joi): Add other parameters.
-    results.push_back(std::move(info));
+    results->push_back(std::move(info));
   }
-
-  // It's safe to directly set the results here (from a thread other
-  // than the UI thread, on which an AsyncExtensionFunction otherwise
-  // normally runs) because there is one instance of this object per
-  // function call, no actor outside of this object is modifying the
-  // results_ member, and the different method invocations on this
-  // object run strictly in sequence; first RunAsync on the UI thread,
-  // then DoQuery on the audio IO thread, then DoneOnUIThread on the
-  // UI thread.
-  results_ = wap::GetSinks::Results::Create(results);

[Devlin, 2017/04/06 01:42:33]

I think this was indeed safe, if you wanted to avoid currying the vector in the
callback.

[o1ka, 2017/04/06 11:01:22]

Yes, it was safe. I just prefer the code which does not require 8-line comment
explaining its safety (wherever it's possible and reasonable) :) Also, such code
is more future-proof.
I don't have a very strong opinion in this case though.
Do you prefer me to revert it?

[Devlin, 2017/04/06 21:23:27]

I don't have a strong preference.  The only reason to prefer this would be
because it lets us allocate the vector<SinkInfo> on the stack instead of the
heap, which would be a bit faster.  But this shouldn't be very
performance-sensitive code (and already does a ton), so this should be fine. 
(And, as you say, not needing an 8 line comment is its own reward. :))

[o1ka, 2017/04/07 12:10:22]

Acknowledged.

-
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&WebrtcAudioPrivateGetSinksFunction::DoneOnUIThread, this));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&WebrtcAudioPrivateGetSinksFunction::DoneOnUIThread, this,
+                 base::Passed(&results)));
 }
 
-void WebrtcAudioPrivateGetSinksFunction::DoneOnUIThread() {
+void WebrtcAudioPrivateGetSinksFunction::DoneOnUIThread(
+    std::unique_ptr<SyncInfoVector> results) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  results_ = wap::GetSinks::Results::Create(*results);
   SendResponse(true);
 }
 
 WebrtcAudioPrivateGetAssociatedSinkFunction::
-WebrtcAudioPrivateGetAssociatedSinkFunction() {
-}
+    WebrtcAudioPrivateGetAssociatedSinkFunction() {}
 
 WebrtcAudioPrivateGetAssociatedSinkFunction::
-~WebrtcAudioPrivateGetAssociatedSinkFunction() {
-}
+    ~WebrtcAudioPrivateGetAssociatedSinkFunction() {}
 
 bool WebrtcAudioPrivateGetAssociatedSinkFunction::RunAsync() {
   params_ = wap::GetAssociatedSink::Params::Create(*args_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   EXTENSION_FUNCTION_VALIDATE(params_.get());
-
   InitDeviceIDSalt();
 
-  AudioManager::Get()->GetTaskRunner()->PostTask(
-      FROM_HERE,
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
       base::Bind(&WebrtcAudioPrivateGetAssociatedSinkFunction::
-                 GetDevicesOnDeviceThread, this));
+                     GetInputDeviceDescriptions,
+                 this));
 
   return true;
 }
 
-void WebrtcAudioPrivateGetAssociatedSinkFunction::GetDevicesOnDeviceThread() {
-  DCHECK(AudioManager::Get()->GetTaskRunner()->BelongsToCurrentThread());
-  AudioManager::Get()->GetAudioInputDeviceDescriptions(&source_devices_);
-
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
+void WebrtcAudioPrivateGetAssociatedSinkFunction::GetInputDeviceDescriptions() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  media::AudioSystem::Get()->GetDeviceDescriptions(
       base::Bind(&WebrtcAudioPrivateGetAssociatedSinkFunction::
-                 GetRawSourceIDOnIOThread,
-                 this));
+                     OnInputDeviceDescriptions,
+                 this),
+      true);
 }
 
-void
-WebrtcAudioPrivateGetAssociatedSinkFunction::GetRawSourceIDOnIOThread() {
+void WebrtcAudioPrivateGetAssociatedSinkFunction::OnInputDeviceDescriptions(
+    media::AudioDeviceDescriptions source_devices) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   url::Origin security_origin(GURL(params_->security_origin));
   std::string source_id_in_origin(params_->source_id_in_origin);
 
   // Find the raw source ID for source_id_in_origin.
   std::string raw_source_id;
-  for (AudioDeviceDescriptions::const_iterator it = source_devices_.begin();
-       it != source_devices_.end(); ++it) {
-    const std::string& id = it->unique_id;
+  for (const auto device : source_devices) {

[Devlin, 2017/04/06 01:42:33]

const auto&

[o1ka, 2017/04/06 11:01:22]

Done.

     if (content::DoesMediaDeviceIDMatchHMAC(device_id_salt(), security_origin,
-                                            source_id_in_origin, id)) {
-      raw_source_id = id;
+                                            source_id_in_origin,
+                                            device.unique_id)) {
+      raw_source_id = device.unique_id;
       DVLOG(2) << "Found raw ID " << raw_source_id
                << " for source ID in origin " << source_id_in_origin;
       break;
     }
   }
-
-  AudioManager::Get()->GetTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&WebrtcAudioPrivateGetAssociatedSinkFunction::
-                 GetAssociatedSinkOnDeviceThread,
-                 this,
-                 raw_source_id));
-}
-
-void
-WebrtcAudioPrivateGetAssociatedSinkFunction::GetAssociatedSinkOnDeviceThread(
-    const std::string& raw_source_id) {
-  DCHECK(AudioManager::Get()->GetTaskRunner()->BelongsToCurrentThread());
-
-  // We return an empty string if there is no associated output device.
-  std::string raw_sink_id;
-  if (!raw_source_id.empty()) {
-    raw_sink_id =
-        AudioManager::Get()->GetAssociatedOutputDeviceID(raw_source_id);
+  if (raw_source_id.empty()) {
+    CalculateHMACAndReplyOnUIThread(std::string());
+    return;
   }
+  media::AudioSystem::Get()->GetAssociatedOutputDeviceID(
+      raw_source_id, base::Bind(&WebrtcAudioPrivateGetAssociatedSinkFunction::
+                                    CalculateHMACAndReplyOnUIThread,
+                                this));
+}
 
-  CalculateHMAC(raw_sink_id);
+void WebrtcAudioPrivateGetAssociatedSinkFunction::
+    CalculateHMACAndReplyOnUIThread(const std::string& raw_sink_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&WebrtcAudioPrivateGetAssociatedSinkFunction::OnHMACCalculated,
+                 this, CalculateHMAC(raw_sink_id)));
 }
 
 void WebrtcAudioPrivateGetAssociatedSinkFunction::OnHMACCalculated(
     const std::string& associated_sink_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
   if (associated_sink_id == media::AudioDeviceDescription::kDefaultDeviceId) {
     DVLOG(2) << "Got default ID, replacing with empty ID.";
     results_ = wap::GetAssociatedSink::Results::Create("");
   } else {
     results_ = wap::GetAssociatedSink::Results::Create(associated_sink_id);
   }
-
   SendResponse(true);
 }