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

device/bluetooth/bluetooth_audio_sink_chromeos.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "device/bluetooth/bluetooth_audio_sink_chromeos.h"
 
 #include <sstream>
 #include <vector>
 
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #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
 
 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),
+      volume_(nullptr),

[Ben Chan, 2015/02/19 00:21:45]

no need to initialize scoped_ptr to nullptr

what's the purpose of this change?

[Miao, 2015/02/23 21:04:25]

Removed.

+      read_mtu_(nullptr),
+      write_mtu_(nullptr),
       adapter_(adapter),
       weak_ptr_factory_(this) {
   DCHECK(adapter_.get());
   DCHECK(adapter_->IsPresent());
 
   adapter_->AddObserver(this);
 
   chromeos::BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
   media->AddObserver(this);
 
   chromeos::BluetoothMediaTransportClient *transport =
       chromeos::DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
   DCHECK(transport);
   transport->AddObserver(this);
 
   StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
 }
 
 BluetoothAudioSinkChromeOS::~BluetoothAudioSinkChromeOS() {
   DCHECK(adapter_.get());
+
+  if (state_ != BluetoothAudioSink::STATE_INVALID &&
+      media_endpoint_.get() != nullptr) {

[armansito, 2015/02/19 00:37:46]

nit: No need to compare against nullptr, the pointer itself will evaluate to
true of false based on its value.

[Miao, 2015/02/23 21:04:25]

Done.

+    Unregister(base::Bind(&base::DoNothing));
+  }
+
   adapter_->RemoveObserver(this);
 
   chromeos::BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
   media->RemoveObserver(this);
 
   chromeos::BluetoothMediaTransportClient *transport =
       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.
+void BluetoothAudioSinkChromeOS::Unregister(const base::Closure& callback) {

[armansito, 2015/02/19 00:37:46]

Note that if BluetoothAudioSinkChromeOS outlives DBusThreadManager, the code
below will crash. You should add a TODO here saying that we should provide this
guarantee in the future.

[Miao, 2015/02/23 21:04:24]

Done.

+  // Gets Media object exported by D-Bus and unregisters the endpoint.
+  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()));
 }
 
 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() != nullptr ? *volume_ : 0;

[armansito, 2015/02/19 00:37:46]

nit: no need for '!= nullptr'

[Miao, 2015/02/23 21:04:25]

Done.

 }
 
 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
+  // be disconnected before SetConfigruation is called.
+  if (state_ == BluetoothAudioSink::STATE_INVALID)
+    return;
+  StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);

[Ben Chan, 2015/02/19 00:21:45]

this may be simpler:

  if (state_ != BluetoothAudioSink::STATE_INVALID)
    StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);

[Miao, 2015/02/23 21:04:25]

Done.

+}
+
+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;
 
   // Retrieves the property set of the transport object with |object_path|.
   chromeos::BluetoothMediaTransportClient::Properties* properties =
       DBusThreadManager::Get()
           ->GetBluetoothMediaTransportClient()
           ->GetProperties(object_path);
 
   // Dispatches a property changed event to the corresponding handler.
   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());
   } else {
     VLOG(1) << "Bluetooth audio sink: transport property " << property_name
             << " changed";
   }
 }
 
 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;
 
   // The initial state for a connection should be "idle".
   if (properties.state != BluetoothMediaTransportClient::kStateIdle) {
     VLOG(1) << "Bluetooth Audio Sink: unexpected state " << properties.state;
     return;
   }
 
   // Updates |volume_| if the volume level is provided in |properties|.
-  if (properties.volume.get() != nullptr)
+  if (properties.volume.get() != nullptr) {

[armansito, 2015/02/19 00:37:46]

nit: no need for '!= nullptr'

[Miao, 2015/02/23 21:04:24]

Done.

+    volume_.reset(new uint16_t());

[Ben Chan, 2015/02/19 00:21:45]

this looks kinda weird as VolumeChanged does nothing if |volume_| is nullptr

perhaps VolumeChanged should take a pointer as well

[armansito, 2015/02/19 00:37:46]

Why not make this assignment from VolumeChanged instead of here?

[Miao, 2015/02/23 21:04:24]

Removed the allocation of memory here. Let VolumeChanged deal with it.

     VolumeChanged(*properties.volume);
+  }
 
-  StateChanged(device::BluetoothAudioSink::STATE_IDLE);
+  StateChanged(BluetoothAudioSink::STATE_IDLE);
 }
 
 void BluetoothAudioSinkChromeOS::SelectConfiguration(
     const std::vector<uint8_t>& capabilities,
     const SelectConfigurationCallback& callback) {
   VLOG(1) << "Bluetooth audio sink: SelectConfiguration called";
   callback.Run(options_.capabilities);
 }
 
 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();
 
   // Creates a Media Endpoint with newly-generated path.
   endpoint_path_ = GenerateEndpointPath();
   media_endpoint_.reset(
       BluetoothMediaEndpointServiceProvider::Create(
           system_bus, endpoint_path_, this));
 
@@ skipping 21 matching lines @@
       base::Bind(&BluetoothAudioSinkChromeOS::OnRegisterFailed,
                  weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 BluetoothMediaEndpointServiceProvider*
     BluetoothAudioSinkChromeOS::GetEndpointServiceProvider() {
   return media_endpoint_.get();
 }
 
 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 BluetoothAudioSink::STATE_INVALID: {

[armansito, 2015/02/19 00:37:46]

All of these added scopes are unnecessary. I would remove them.

[Miao, 2015/02/23 21:04:24]

Done.

+      ResetMedia();
+      ResetEndpoint();
 
-  switch (state) {
-    case device::BluetoothAudioSink::STATE_INVALID: {
-      // TODO(mcchou): BUG=441581
-      ResetMedia();
-      ResetTransport();
-      ResetEndpoint();
+      // Skips unnecessary clean-up on transport object.
+      if (state_ != BluetoothAudioSink::STATE_DISCONNECTED)

[armansito, 2015/02/19 00:37:46]

Kind of unnecessary I think, I'd just make ResetTransport not do anything if it
doesn't have anything to clean up.

So, remove the break and the if statement and just fall through to the next
case.

[Miao, 2015/02/23 21:04:24]

Done.

+        ResetTransport();
       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() == nullptr)

[armansito, 2015/02/19 00:37:46]

nit: if (!volume_.get())

That said, your code looks a bit odd. You're initializing volume_ both here and
outside, in addition you have this weird check, I'd change this so that you just
initialize it here if volume_ is NULL:

if (!volume_.get())
  volume_.reset(new...);

[Miao, 2015/02/23 21:04:24]

Removed.

+    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));
+  FOR_EACH_OBSERVER(BluetoothAudioSink::Observer, observers_,

[Miao, 2015/02/23 21:04:24]

Check whether the new volume before notifying 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();
 }
 
 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 std::string& error_name,
+    const std::string& error_message) {
+  VLOG(1) << "Bluetooth audio sink: " << error_name << ": " <<  error_message;
 }
 
 void BluetoothAudioSinkChromeOS::ReadFromFD() {
   DCHECK_GE(fd_.value(), 0);
 
   // TODO(mcchou): BUG=441581
   // Read from file descriptor using watcher and create a buffer to contain the
   // data. Notify |Observers_| while there is audio data available.
 }
 
 void BluetoothAudioSinkChromeOS::ResetMedia() {
   media_path_ = dbus::ObjectPath("");
 }
 
 void BluetoothAudioSinkChromeOS::ResetTransport() {
   transport_path_ = dbus::ObjectPath("");
+  VolumeChanged(0);

[armansito, 2015/02/19 00:37:46]

So did volume just become 0 or are you cleaning up the volume in general?

[Miao, 2015/02/23 21:04:24]

Once the transport is invalid, user applications should get notified first, and
then we reset the |volume_|.

   volume_ = 0;

[Ben Chan, 2015/02/19 00:21:45]

volume_.reset()?

[Miao, 2015/02/23 21:04:24]

Done.

   read_mtu_ = 0;
   write_mtu_ = 0;
   fd_.PutValue(-1);
 }
 
 void BluetoothAudioSinkChromeOS::ResetEndpoint() {
-  endpoint_path_ = dbus::ObjectPath("");
+  endpoint_path_ = ObjectPath("");
   media_endpoint_ = nullptr;
 }
 
 }  // namespace chromeos