device/bluetooth:Implement BluetoothMediaEndpointServiceProvider delegate and media-related overrid…

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 9623c7824e93e50033dbf6f8d4c943e7312b85e4..e7a5a3a1787603906736d0ef2ea8a2c4a2baf0e7 100644
--- a/device/bluetooth/bluetooth_audio_sink_chromeos.cc
+++ b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
@@ -5,9 +5,11 @@
 #include "device/bluetooth/bluetooth_audio_sink_chromeos.h"
 
 #include <sstream>
+#include <vector>
 
 #include "base/logging.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "dbus/message.h"
 #include "device/bluetooth/bluetooth_adapter_chromeos.h"
 
 namespace {
@@ -34,6 +36,7 @@ BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
       read_mtu_(0),
       write_mtu_(0),
       adapter_(adapter),
+      media_endpoint_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(adapter_.get());
   DCHECK(adapter_->IsPresent());
@@ -45,11 +48,22 @@ BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
 BluetoothAudioSinkChromeOS::~BluetoothAudioSinkChromeOS() {
   DCHECK(adapter_.get());
   adapter_->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::AddObserver(
     device::BluetoothAudioSink::Observer* observer) {
   DCHECK(observer);
@@ -86,40 +100,79 @@ void BluetoothAudioSinkChromeOS::AdapterPresentChanged(
 void BluetoothAudioSinkChromeOS::MediaRemoved(
     const dbus::ObjectPath& object_path) {
   // TODO(mcchou): BUG=441581
-  // Check if |object_path| equals to |media_path_|. If true, change the state
-  // of the audio sink, call StateChanged and reset the audio sink.
+  // Changes |state_| to STATE_INVALID or STATE_DISCONNECTED?
+  if (object_path == media_path_) {
+    StateChanged(device::BluetoothAudioSink::STATE_INVALID);
+  }
 }
 
 void BluetoothAudioSinkChromeOS::MediaTransportRemoved(
     const dbus::ObjectPath& object_path) {
-  // TODO(mcchou): BUG=441581
-  // Check if |object_path| equals to |transport_path_|. If true, change the
-  // state of the audio sink, call StateChanged and reset the audio sink.
   // 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);
+  }
 }
 
 void BluetoothAudioSinkChromeOS::MediaTransportPropertyChanged(
     const dbus::ObjectPath& object_path,
     const std::string& property_name) {
-  // TODO(mcchou): BUG=441581
-  // Call StateChanged and VolumeChanged accordingly if there is any change on
-  // state/volume.
+  // 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);
+    } else if (properties->state.value() ==
+               BluetoothMediaTransportClient::kStatePending) {
+      StateChanged(device::BluetoothAudioSink::STATE_PENDING);
+    } else if (properties->state.value() ==
+               BluetoothMediaTransportClient::kStateActive) {
+      StateChanged(device::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 dbus::MessageReader& properties) {
+    const TransportProperties& properties) {
   // TODO(mcchou): BUG=441581
-  // Update |transport_path_| and store properties if needed.
+  // Stores properties if needed.
+  VLOG(1) << "Bluetooth audio sink: SetConfiguration called";
+  transport_path_ = transport_path;
+  chromeos::DBusThreadManager::Get()
+      ->GetBluetoothMediaTransportClient()
+      ->AddObserver(this);
+
+  // The initial state for a connection should be "idle".
+  if (properties.state.compare(BluetoothMediaTransportClient::kStateIdle) ==

[armansito, 2015/02/10 00:17:01]

nit: You can just compare the strings with operator==

[Miao, 2015/02/10 22:15:21]

Done.

+      0) {
+    // Updates |volume_| if the volume level is provided in |properties|.
+    if (properties.volume.get() != nullptr)
+      VolumeChanged(*properties.volume);
+
+    StateChanged(device::BluetoothAudioSink::STATE_IDLE);
+  } else {
+    LOG(WARNING) << "Bluetooth Audio Sink: unexpected state "
+                 << properties.state;

[armansito, 2015/02/10 00:17:02]

All of these nested if statements are getting a bit unreadable. Turn this around
and return early.

[Miao, 2015/02/10 22:15:21]

Done.

+  }
 }
 
 void BluetoothAudioSinkChromeOS::SelectConfiguration(
     const std::vector<uint8_t>& capabilities,
     const SelectConfigurationCallback& callback) {
-  // TODO(mcchou): BUG=441581
-  // Use SelectConfigurationCallback to return the agreed capabilities.
   VLOG(1) << "Bluetooth audio sink: SelectConfiguration called";
   callback.Run(options_.capabilities);
 }
@@ -127,11 +180,13 @@ void BluetoothAudioSinkChromeOS::SelectConfiguration(
 void BluetoothAudioSinkChromeOS::ClearConfiguration(
     const dbus::ObjectPath& transport_path) {
   // TODO(mcchou): BUG=441581
-  // Reset the configuration to the default one and close IOBuffer.
+  // Closes IOBuffer.

[armansito, 2015/02/10 00:17:02]

Well, currently there's no IOBuffer anywhere so this comment is somewhat
invalid.

[Miao, 2015/02/10 22:15:21]

Removed.

+  VLOG(1) << "Bluetooth audio sink: ClearConfiguration called";
+  StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
 }
 
-void BluetoothAudioSinkChromeOS::Release() {
-  VLOG(1) << "Bluetooth audio sink: Release called";
+void BluetoothAudioSinkChromeOS::Released() {
+  VLOG(1) << "Bluetooth audio sink: Released called";
   StateChanged(device::BluetoothAudioSink::STATE_INVALID);
 }
 
@@ -139,10 +194,10 @@ void BluetoothAudioSinkChromeOS::Register(
     const device::BluetoothAudioSink::Options& options,
     const base::Closure& callback,
     const device::BluetoothAudioSink::ErrorCallback& error_callback) {
-  // TODO(mcchou): BUG=441581
-  // Get Media object, initiate an Media Endpoint with options, and return the
-  // audio sink via callback. Add the audio sink as observer of both Media and
-  // Media Transport.
+  // Gets Media object, initiates an Media Endpoint with options, and returns

[armansito, 2015/02/10 00:17:02]

nit: s/an Media/a Media/

[Miao, 2015/02/10 22:15:21]

Done.

+  // the audio sink via callback. Adds the audio sink as an observer of both
+  // Media.

[armansito, 2015/02/10 00:17:01]

Note: You should avoid comments like this if all they do is explain verbatim
what the following code does and the code itself is self explanatory, unless you
have some additional insight that you think is valuable, such as side-effects,
etc.

[Miao, 2015/02/10 22:15:21]

Done.

+
   DCHECK(adapter_.get());
   DCHECK_EQ(state_, device::BluetoothAudioSink::STATE_DISCONNECTED);
 
@@ -167,6 +222,8 @@ void BluetoothAudioSinkChromeOS::Register(
   // Gets Media object exported by D-Bus and registers the endpoint.
   chromeos::BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
+  DCHECK(media);
+
   BluetoothAdapterChromeOS* adapter_chromeos =
       static_cast<BluetoothAdapterChromeOS*>(adapter_.get());
   media->AddObserver(this);
@@ -181,14 +238,9 @@ void BluetoothAudioSinkChromeOS::Register(
                  weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
-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.
+BluetoothMediaEndpointServiceProvider*
+    BluetoothAudioSinkChromeOS::GetEndpointServiceProvider() {
+  return media_endpoint_.get();
 }
 
 void BluetoothAudioSinkChromeOS::StateChanged(
@@ -201,18 +253,21 @@ void BluetoothAudioSinkChromeOS::StateChanged(
   switch (state) {
     case device::BluetoothAudioSink::STATE_INVALID: {
       // TODO(mcchou): BUG=441581
-      // Clean media, media transport and media endpoint.
+      ResetMedia();
+      ResetTransport();
+      ResetEndpoint();
       break;
     }
     case device::BluetoothAudioSink::STATE_DISCONNECTED: {
       // TODO(mcchou): BUG=441581
-      // Clean media transport.
+      // Clean media transport and remove the audio sink from its observer list.
+      ResetTransport();
       break;
     }
     case device::BluetoothAudioSink::STATE_IDLE: {
       // TODO(mcchou): BUG=441581
-      // Triggered by MediaTransportPropertyChanged. Stop watching on file
-      // descriptor if there is one.
+      // Triggered by MediaTransportPropertyChanged and SetConfiguration.
+      // Stop watching on file descriptor if there is one.
       break;
     }
     case device::BluetoothAudioSink::STATE_PENDING: {
@@ -258,8 +313,7 @@ void BluetoothAudioSinkChromeOS::OnRegisterFailed(
   DCHECK(media_endpoint_.get());
   VLOG(1) << "Bluetooth audio sink: " << error_name << ": " <<  error_message;
 
-  endpoint_path_ = dbus::ObjectPath("");
-  media_endpoint_ = nullptr;
+  ResetEndpoint();
   error_callback.Run(device::BluetoothAudioSink::ERROR_NOT_REGISTERED);
 }
 
@@ -271,4 +325,25 @@ void BluetoothAudioSinkChromeOS::ReadFromFD() {
   // data. Notify |Observers_| while there is audio data available.
 }
 
+void BluetoothAudioSinkChromeOS::ResetMedia() {
+  chromeos::DBusThreadManager::Get()->GetBluetoothMediaClient()
+      ->RemoveObserver(this);
+  media_path_ = dbus::ObjectPath("");
+}
+
+void BluetoothAudioSinkChromeOS::ResetTransport() {
+  chromeos::DBusThreadManager::Get()->GetBluetoothMediaTransportClient()
+      ->RemoveObserver(this);
+  transport_path_ = dbus::ObjectPath("");
+  volume_ = 0;
+  read_mtu_ = 0;
+  write_mtu_ = 0;
+  fd_.PutValue(-1);
+}
+
+void BluetoothAudioSinkChromeOS::ResetEndpoint() {
+  endpoint_path_ = dbus::ObjectPath("");
+  media_endpoint_ = nullptr;
+}
+
 }  // namespace chromeos