device/bluetooth: Implement Unregister() of BlueotoothAudioSinkChromeOS and disconnection-related c…

device/bluetooth/bluetooth_audio_sink_chromeos.cc

Index: device/bluetooth/bluetooth_audio_sink_chromeos.cc
diff --git a/device/bluetooth/bluetooth_audio_sink_chromeos.cc b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
index 7791563070b0ec7dd02a834d297d1fb0846a6469..070ac4e5024315b52cd32b15b660e82cad73b41f 100644
--- a/device/bluetooth/bluetooth_audio_sink_chromeos.cc
+++ b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
@@ -13,17 +13,20 @@
 #include "dbus/message.h"
 #include "device/bluetooth/bluetooth_adapter_chromeos.h"
 
+using dbus::ObjectPath;
+using device::BluetoothAudioSink;
+
 namespace {
 
 // TODO(mcchou): Add the constant to dbus/service_constants.h.
 const char kBluetoothAudioSinkServicePath[] = "/org/chromium/AudioSink";
 
-dbus::ObjectPath GenerateEndpointPath() {
+ObjectPath GenerateEndpointPath() {
   static unsigned int sequence_number = 0;
   ++sequence_number;
   std::stringstream path;
   path << kBluetoothAudioSinkServicePath << "/endpoint" << sequence_number;
-  return dbus::ObjectPath(path.str());
+  return ObjectPath(path.str());
 }
 
 }  // namespace
@@ -32,10 +35,9 @@ namespace chromeos {
 
 BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
     scoped_refptr<device::BluetoothAdapter> adapter)
-    : state_(device::BluetoothAudioSink::STATE_INVALID),
-      volume_(0),
-      read_mtu_(0),
-      write_mtu_(0),
+    : state_(BluetoothAudioSink::STATE_INVALID),
+      read_mtu_(nullptr),
+      write_mtu_(nullptr),
       adapter_(adapter),
       weak_ptr_factory_(this) {
   DCHECK(adapter_.get());
@@ -58,6 +60,13 @@ BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
 
 BluetoothAudioSinkChromeOS::~BluetoothAudioSinkChromeOS() {
   DCHECK(adapter_.get());
+
+  if (state_ != BluetoothAudioSink::STATE_INVALID && media_endpoint_.get()) {
+    Unregister(base::Bind(&base::DoNothing),
+               base::Bind(&BluetoothAudioSinkChromeOS::UnregisterErrorCallback,
+                          base::Unretained(this)));

[armansito, 2015/02/23 22:07:06]

You wouldn't need to pass an unretained pointer if you just made
UnregisterErrorCallback a file-local function (i.e. in an anonymous namespace).

[Miao, 2015/02/24 01:12:13]

Done.

+  }
+
   adapter_->RemoveObserver(this);
 
   chromeos::BluetoothMediaClient* media =
@@ -69,76 +78,91 @@ BluetoothAudioSinkChromeOS::~BluetoothAudioSinkChromeOS() {
       chromeos::DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
   DCHECK(transport);
   transport->RemoveObserver(this);
-
-  // TODO(mcchou): BUG=441581
-  // Unregister() should be called while media and media endpoint are still
-  // valid.
 }
 
 void BluetoothAudioSinkChromeOS::Unregister(
     const base::Closure& callback,
     const device::BluetoothAudioSink::ErrorCallback& error_callback) {
-  // TODO(mcchou): BUG=441581
-  // Call UnregisterEndpoint on the media object with |media_path_| and clean
-  // |observers_| and transport_path_ and reset state_ and volume.
-  // Check whether the media endpoint is registered before invoking
-  // UnregisterEndpoint.
+  // TODO(mcchou): Guarantee that DBusThreadManager outlives
+  // BluetoothAudioSinkChromeOS. Otherwise the code below will crash.
+  // Gets Media object exported by D-Bus and unregisters the endpoint.

[armansito, 2015/02/23 22:07:06]

nit: probably file a bug for this and refer to the bug number from here.

[Miao, 2015/02/24 01:12:13]

Done.

+  chromeos::BluetoothMediaClient* media =
+      DBusThreadManager::Get()->GetBluetoothMediaClient();
+  DCHECK(media);
+
+  media->UnregisterEndpoint(
+      media_path_,
+      endpoint_path_,
+      base::Bind(&BluetoothAudioSinkChromeOS::OnUnregisterSucceeded,
+                 weak_ptr_factory_.GetWeakPtr(), callback),
+      base::Bind(&BluetoothAudioSinkChromeOS::OnUnregisterFailed,
+                 weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void BluetoothAudioSinkChromeOS::AddObserver(
-    device::BluetoothAudioSink::Observer* observer) {
+    BluetoothAudioSink::Observer* observer) {
   DCHECK(observer);
   observers_.AddObserver(observer);
 }
 
 void BluetoothAudioSinkChromeOS::RemoveObserver(
-    device::BluetoothAudioSink::Observer* observer) {
+    BluetoothAudioSink::Observer* observer) {
   DCHECK(observer);
   observers_.RemoveObserver(observer);
 }
 
-device::BluetoothAudioSink::State BluetoothAudioSinkChromeOS::GetState() const {
+BluetoothAudioSink::State BluetoothAudioSinkChromeOS::GetState() const {
   return state_;
 }
 
 uint16_t BluetoothAudioSinkChromeOS::GetVolume() const {
-  return volume_;
+  return volume_.get() ? *volume_ : 0;

[armansito, 2015/02/23 22:07:06]

Document at the API definition that GetVolume() returns 0 if the volume is
unknown.

Now, that you're actually returning 0 if the volume is unknown, do you need to
store it as a scoped_ptr?

[Miao, 2015/02/24 01:12:13]

Changed |volume_| back to uint16_t. Possible values of a volume are 0-127, so I
use 128 to represent invalid volume.

 }
 
 void BluetoothAudioSinkChromeOS::AdapterPresentChanged(
-    device::BluetoothAdapter* adapter,
-    bool present) {
+    device::BluetoothAdapter* adapter, bool present) {
   VLOG(1) << "Bluetooth audio sink: Bluetooth adapter present changed: "
           << present;
 
   if (adapter->IsPresent()) {
-    StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+    StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
   } else {
-    StateChanged(device::BluetoothAudioSink::STATE_INVALID);
+    adapter_->RemoveObserver(this);
+    StateChanged(BluetoothAudioSink::STATE_INVALID);
   }
 }
 
-void BluetoothAudioSinkChromeOS::MediaRemoved(
-    const dbus::ObjectPath& object_path) {
-  // TODO(mcchou): BUG=441581
-  // Changes |state_| to STATE_INVALID or STATE_DISCONNECTED?
+void BluetoothAudioSinkChromeOS::AdapterPoweredChanged(
+    device::BluetoothAdapter* adapter, bool powered) {
+  VLOG(1) << "Bluetooth audio sink: Bluetooth adapter powered changed: "
+          << powered;
+
+  // Regardless of the new powered state, |state_| goes to STATE_DISCONNECTED.
+  // If false, the transport is closed, but the endpoint is still valid for use.
+  // If true, the previous transport has been deprecated, so the |state_| has to

[armansito, 2015/02/23 22:07:06]

"deprecated" is not the correct word here, maybe "torn down"?

[Miao, 2015/02/24 01:12:13]

Done.

+  // be disconnected before SetConfigruation is called.
+  if (state_ != BluetoothAudioSink::STATE_INVALID)
+    StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
+}
+
+void BluetoothAudioSinkChromeOS::MediaRemoved(const ObjectPath& object_path) {
   if (object_path == media_path_) {
-    StateChanged(device::BluetoothAudioSink::STATE_INVALID);
+    StateChanged(BluetoothAudioSink::STATE_INVALID);
   }
 }
 
 void BluetoothAudioSinkChromeOS::MediaTransportRemoved(
-    const dbus::ObjectPath& object_path) {
+    const ObjectPath& object_path) {
   // Whenever powered of |adapter_| turns false while present stays true, media
   // transport object should be removed accordingly, and the state should be
   // changed to STATE_DISCONNECTED.
   if (object_path == transport_path_) {
-    StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+    StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
   }
 }
 
 void BluetoothAudioSinkChromeOS::MediaTransportPropertyChanged(
-    const dbus::ObjectPath& object_path,
+    const ObjectPath& object_path,
     const std::string& property_name) {
   if (object_path != transport_path_)
     return;
@@ -153,13 +177,13 @@ void BluetoothAudioSinkChromeOS::MediaTransportPropertyChanged(
   if (property_name == properties->state.name()) {
     if (properties->state.value() ==
         BluetoothMediaTransportClient::kStateIdle) {
-      StateChanged(device::BluetoothAudioSink::STATE_IDLE);
+      StateChanged(BluetoothAudioSink::STATE_IDLE);
     } else if (properties->state.value() ==
                BluetoothMediaTransportClient::kStatePending) {
-      StateChanged(device::BluetoothAudioSink::STATE_PENDING);
+      StateChanged(BluetoothAudioSink::STATE_PENDING);
     } else if (properties->state.value() ==
                BluetoothMediaTransportClient::kStateActive) {
-      StateChanged(device::BluetoothAudioSink::STATE_ACTIVE);
+      StateChanged(BluetoothAudioSink::STATE_ACTIVE);
     }
   } else if (property_name == properties->volume.name()) {
     VolumeChanged(properties->volume.value());
@@ -170,7 +194,7 @@ void BluetoothAudioSinkChromeOS::MediaTransportPropertyChanged(
 }
 
 void BluetoothAudioSinkChromeOS::SetConfiguration(
-    const dbus::ObjectPath& transport_path,
+    const ObjectPath& transport_path,
     const TransportProperties& properties) {
   VLOG(1) << "Bluetooth audio sink: SetConfiguration called";
   transport_path_ = transport_path;
@@ -182,10 +206,11 @@ void BluetoothAudioSinkChromeOS::SetConfiguration(
   }
 
   // Updates |volume_| if the volume level is provided in |properties|.
-  if (properties.volume.get() != nullptr)
+  if (properties.volume.get()) {
     VolumeChanged(*properties.volume);
+  }
 
-  StateChanged(device::BluetoothAudioSink::STATE_IDLE);
+  StateChanged(BluetoothAudioSink::STATE_IDLE);
 }
 
 void BluetoothAudioSinkChromeOS::SelectConfiguration(
@@ -196,22 +221,24 @@ void BluetoothAudioSinkChromeOS::SelectConfiguration(
 }
 
 void BluetoothAudioSinkChromeOS::ClearConfiguration(
-    const dbus::ObjectPath& transport_path) {
+    const ObjectPath& transport_path) {
+  if (transport_path != transport_path_)
+    return;
   VLOG(1) << "Bluetooth audio sink: ClearConfiguration called";
-  StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+  StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
 }
 
 void BluetoothAudioSinkChromeOS::Released() {
   VLOG(1) << "Bluetooth audio sink: Released called";
-  StateChanged(device::BluetoothAudioSink::STATE_INVALID);
+  StateChanged(BluetoothAudioSink::STATE_INVALID);
 }
 
 void BluetoothAudioSinkChromeOS::Register(
-    const device::BluetoothAudioSink::Options& options,
+    const BluetoothAudioSink::Options& options,
     const base::Closure& callback,
-    const device::BluetoothAudioSink::ErrorCallback& error_callback) {
+    const BluetoothAudioSink::ErrorCallback& error_callback) {
   DCHECK(adapter_.get());
-  DCHECK_EQ(state_, device::BluetoothAudioSink::STATE_DISCONNECTED);
+  DCHECK_EQ(state_, BluetoothAudioSink::STATE_DISCONNECTED);
 
   // Gets system bus.
   dbus::Bus* system_bus = DBusThreadManager::Get()->GetSystemBus();
@@ -253,65 +280,56 @@ BluetoothMediaEndpointServiceProvider*
 }
 
 void BluetoothAudioSinkChromeOS::StateChanged(
-    device::BluetoothAudioSink::State state) {
+    BluetoothAudioSink::State state) {
   if (state == state_)
     return;
 
   VLOG(1) << "Bluetooth audio sink state changed: " << state;
-
   switch (state) {
-    case device::BluetoothAudioSink::STATE_INVALID: {
-      // TODO(mcchou): BUG=441581
+    case BluetoothAudioSink::STATE_INVALID:
       ResetMedia();
-      ResetTransport();
       ResetEndpoint();
-      break;
-    }
-    case device::BluetoothAudioSink::STATE_DISCONNECTED: {
-      // TODO(mcchou): BUG=441581
-      // Clean media transport and remove the audio sink from its observer list.
+    case BluetoothAudioSink::STATE_DISCONNECTED:
       ResetTransport();
       break;
-    }
-    case device::BluetoothAudioSink::STATE_IDLE: {
+    case BluetoothAudioSink::STATE_IDLE:
       // TODO(mcchou): BUG=441581
       // Triggered by MediaTransportPropertyChanged and SetConfiguration.
       // Stop watching on file descriptor if there is one.
       break;
-    }
-    case device::BluetoothAudioSink::STATE_PENDING: {
+    case BluetoothAudioSink::STATE_PENDING:
       // TODO(mcchou): BUG=441581
       // Call BluetoothMediaTransportClient::Acquire() to get fd and mtus.
       break;
-    }
-    case device::BluetoothAudioSink::STATE_ACTIVE: {
+    case BluetoothAudioSink::STATE_ACTIVE:
       // TODO(mcchou): BUG=441581
       // Read from fd and call DataAvailable.
       ReadFromFD();
       break;
-    }
     default:
       break;
   }
 
   state_ = state;
-  FOR_EACH_OBSERVER(device::BluetoothAudioSink::Observer, observers_,
+  FOR_EACH_OBSERVER(BluetoothAudioSink::Observer, observers_,
                     BluetoothAudioSinkStateChanged(this, state_));
 }
 
 void BluetoothAudioSinkChromeOS::VolumeChanged(uint16_t volume) {
-  DCHECK_NE(volume, volume_);
+  if (volume_.get() && volume == *volume_)

[armansito, 2015/02/23 22:07:06]

You could probably check "if (GetVolume() == volume)" here, since from a public
interface standpoint, you're returning 0 anyway.

[Miao, 2015/02/24 01:12:13]

Done.

+    return;
   VLOG(1) << "Bluetooth audio sink volume changed: " << volume;
-  volume_ = volume;
-  FOR_EACH_OBSERVER(device::BluetoothAudioSink::Observer, observers_,
-                    BluetoothAudioSinkVolumeChanged(this, volume_));
+  volume_.reset(new uint16_t(volume));

[armansito, 2015/02/23 22:07:06]

You should only reset here if volume_ is NULL, to avoid unnecessary realloc.

[Miao, 2015/02/24 01:12:13]

Done.

+  FOR_EACH_OBSERVER(BluetoothAudioSink::Observer, observers_,
+                    BluetoothAudioSinkVolumeChanged(this, *volume_));
 }
 
 void BluetoothAudioSinkChromeOS::OnRegisterSucceeded(
     const base::Closure& callback) {
+  DCHECK(media_endpoint_.get());
   VLOG(1) << "Bluetooth audio sink registerd";
 
-  StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+  StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
   callback.Run();
 }
 
@@ -319,11 +337,33 @@ void BluetoothAudioSinkChromeOS::OnRegisterFailed(
     const BluetoothAudioSink::ErrorCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {
-  DCHECK(media_endpoint_.get());
   VLOG(1) << "Bluetooth audio sink: " << error_name << ": " <<  error_message;
 
   ResetEndpoint();
-  error_callback.Run(device::BluetoothAudioSink::ERROR_NOT_REGISTERED);
+  error_callback.Run(BluetoothAudioSink::ERROR_NOT_REGISTERED);
+}
+
+void BluetoothAudioSinkChromeOS::OnUnregisterSucceeded(
+    const base::Closure& callback) {
+  VLOG(1) << "Bluetooth audio sink unregisterd";
+
+  // Once the state becomes STATE_INVALID, media, media transport and media
+  // endpoint will be reset.
+  StateChanged(BluetoothAudioSink::STATE_INVALID);
+  callback.Run();
+}
+
+void BluetoothAudioSinkChromeOS::OnUnregisterFailed(
+    const device::BluetoothAudioSink::ErrorCallback& error_callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  VLOG(1) << "Bluetooth audio sink: " << error_name << ": " <<  error_message;
+  error_callback.Run(BluetoothAudioSink::ERROR_NOT_UNREGISTERED);
+}
+
+void BluetoothAudioSinkChromeOS::UnregisterErrorCallback(
+    device::BluetoothAudioSink::ErrorCode error_code) {
+  VLOG(1) << "Bluetooth audio sink: Error code: " << error_code;
 }
 
 void BluetoothAudioSinkChromeOS::ReadFromFD() {
@@ -339,15 +379,18 @@ void BluetoothAudioSinkChromeOS::ResetMedia() {
 }
 
 void BluetoothAudioSinkChromeOS::ResetTransport() {
+  if (transport_path_.value() == "")
+    return;
   transport_path_ = dbus::ObjectPath("");
-  volume_ = 0;
+  VolumeChanged(0);
+  volume_.reset();
   read_mtu_ = 0;
   write_mtu_ = 0;
   fd_.PutValue(-1);
 }
 
 void BluetoothAudioSinkChromeOS::ResetEndpoint() {
-  endpoint_path_ = dbus::ObjectPath("");
+  endpoint_path_ = ObjectPath("");
   media_endpoint_ = nullptr;
 }