device/bluetooth:Implement Register() for BluetoothAudioSinkChromeOS.

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 840769e8f1d337d79ec3a1927d9d5ea5aacf5e81..59c3d3e08c57ce79db2fede704f04d64e7200d37 100644
--- a/device/bluetooth/bluetooth_audio_sink_chromeos.cc
+++ b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
@@ -7,6 +7,23 @@
 #include <sstream>
 
 #include "base/logging.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+
+namespace {
+
+// TODO(mcchou): Add the constant to dbus/service_constants.h.
+const char kBluetoothAudioSinkServicePath[] = "/org/chromium/AudioSink";
+const char kBluetoothAudioSinkUUID[] = "0000110b-0000-1000-8000-00805f9b34fb";
+
+dbus::ObjectPath GenerateEndpointPath() {
+  static unsigned int sequence_number = 0;
+  ++sequence_number;
+  std::stringstream path;
+  path << kBluetoothAudioSinkServicePath << "/endpoint" << sequence_number;
+  return dbus::ObjectPath(path.str());
+}
+
+}  // namespace
 
 namespace chromeos {
 
@@ -25,7 +42,7 @@ BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
   present_ = adapter_->IsPresent();
   powered_ = adapter_->IsPowered();
   if (present_ && powered_)
-    state_ = device::BluetoothAudioSink::STATE_DISCONNECTED;
+    StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
   adapter_->AddObserver(this);
 }
 
@@ -57,19 +74,29 @@ uint16_t BluetoothAudioSinkChromeOS::GetVolume() const {
 void BluetoothAudioSinkChromeOS::AdapterPresentChanged(
     device::BluetoothAdapter* adapter,
     bool present) {
-  // TODO(mcchou): BUG=441581
-  // If |persent| is true, change state to |STATE_DISCONNECTED| and call
-  // StateChanged(). Otherwise, change state to |STATE_INVALID| and call
-  // StateChanged.
+  if (present == present_) return;

[armansito, 2015/01/27 04:38:06]

nit: Put return in its own line.

[armansito, 2015/01/27 04:38:06]

This check isn't really necessary, since the adapter won't call this if the
value hasn't changed.

[Miao, 2015/01/28 02:05:01]

Removed.

+  VLOG(1) << "Bluetooth audio sink: Bluetooth adapter present changed: "
+          << present;
+  present_ = present;

[armansito, 2015/01/27 04:38:06]

I don't think that |present_| and |powered_| are necessary member variables
since you can obtain these from the adapter by calling adapter_->IsPresent() and
adapter_->IsPowered(). So I would remove them from the class.

[Miao, 2015/01/28 02:05:01]

Removed.

+  if (present_ && adapter->IsPowered()) {
+    StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+  } else {

[armansito, 2015/01/27 04:38:06]

This means that the state will become INVALID if |present_| is true and
IsPowered is false, which is probably not what we want. I would simply mark the
state as invalid if |present_| is false. Otherwise, mark the state as
DISCONNECTED if no media transport is currently assigned.

[Miao, 2015/01/28 02:05:01]

Will remove AdapterPoweredChanged and let MediaTransportRemoved change the state
to STATE_DISCONNECTED. The MediaTransport will be destroyed if powered is down.

+    StateChanged(device::BluetoothAudioSink::STATE_INVALID);
+  }
 }
 
 void BluetoothAudioSinkChromeOS::AdapterPoweredChanged(
     device::BluetoothAdapter* adapter,
     bool powered) {
-  // TODO(mcchou): BUG=441581
-  // If |powered| is true, change state to |STATE_DISCONNECTED| and call
-  // StateChanged(). Otherwise, change state to |STATE_INVALID| and call
-  // StateChanged.
+  if (powered == powered_) return;

[armansito, 2015/01/27 04:38:05]

Same as above.

[Miao, 2015/01/28 02:05:01]

Removed.

+  VLOG(1) << "Bluetooth audio sink: Bluetooth adapter powered changed: "
+          << powered;
+  powered_ = powered;
+  if (powered_ && adapter->IsPresent()) {
+    StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+  } else {
+    StateChanged(device::BluetoothAudioSink::STATE_INVALID);

[armansito, 2015/01/27 04:38:05]

Same logic as above. Besides this code is essentially identical, so I would put
the state determination into a single method.

[Miao, 2015/01/28 02:05:01]

Removed.

+  }
 }
 
 void BluetoothAudioSinkChromeOS::MediaRemoved(
@@ -106,6 +133,8 @@ void BluetoothAudioSinkChromeOS::SelectConfiguration(
     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);
 }
 
 void BluetoothAudioSinkChromeOS::ClearConfiguration(
@@ -127,19 +156,86 @@ void BluetoothAudioSinkChromeOS::Register(
   // 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.
+  DCHECK(adapter_);
+  DCHECK_EQ(state_, device::BluetoothAudioSink::STATE_DISCONNECTED);
+
+  // Gets system bus.
+  dbus::Bus* system_bus = DBusThreadManager::Get()->GetSystemBus();
+
+  // Creates a Media Endpoint with newly-generated path.
+  dbus::ObjectPath endpoint_path_ = GenerateEndpointPath();

[armansito, 2015/01/27 04:38:05]

This code shouldn't compile, since you're redeclaring |endpoint_path_| (which
appears to be a member variable).

[Miao, 2015/01/28 02:05:01]

The compilation will pass, since it will be a local variable hiding the
class-wide one, but I shouldn't redeclare endpoint_point here anyway.

+  media_endpoint_.reset(
+      BluetoothMediaEndpointServiceProvider::Create(
+          system_bus, endpoint_path_, this));
+
+  DCHECK(media_endpoint_.get());
+
+  // Creates endpoint properties with |options|.
+  options_ = options;
+  chromeos::BluetoothMediaClient::EndpointProperties endpoint_properties;
+  endpoint_properties.uuid = std::string(kBluetoothAudioSinkUUID);
+  endpoint_properties.codec = options_.codec;
+  endpoint_properties.capabilities = options_.capabilities;
+
+  // Gets Media object exported by D-Bus and registers the endpoint.
+  chromeos::BluetoothMediaClient* media =
+      DBusThreadManager::Get()->GetBluetoothMediaClient();
+  media->AddObserver(this);
+  media_path_ = adapter_->object_path();
+  media->RegisterEndpoint(
+      media_path_,
+      endpoint_path_,
+      endpoint_properties,
+      base::Bind(&BluetoothAudioSinkChromeOS::OnRegisterSucceeded,
+                 weak_ptr_factory_.GetWeakPtr(), callback),
+      base::Bind(&BluetoothAudioSinkChromeOS::OnRegisterFailed,
+                 weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void BluetoothAudioSinkChromeOS::Unregister(
     const base::Closure& callback,
     const device::BluetoothAudioSink::ErrorCallback& error_callback) {
   // TODO(mcchou): BUG=441581
-  // Clean |observers_| and |transport_path_| and reset |state_| and |volume_|.
+  // Call UnregisterEndpoint on the media object with |media_path_| and clean
+  // |observers_| and transport_path_ and reset state_ and volume.
 }
 
 void BluetoothAudioSinkChromeOS::StateChanged(
     device::BluetoothAudioSink::State state) {
-  DCHECK_NE(state, state_);
+  if (state == state_) return;

[armansito, 2015/01/27 04:38:06]

nit: return; in its own line.

[Miao, 2015/01/28 02:05:01]

Done.

   VLOG(1) << "Bluetooth audio sink state changed: " << state;
+  switch (state) {
+    case device::BluetoothAudioSink::STATE_INVALID: {
+      // TODO(mcchou): BUG=441581
+      // Clean media, media transport and media endpoint.
+      break;
+    }
+    case device::BluetoothAudioSink::STATE_DISCONNECTED: {
+      // TODO(mcchou): BUG=441581
+      // Clean media transport.
+      break;
+    }
+    case device::BluetoothAudioSink::STATE_IDLE: {
+      // TODO(mcchou): BUG=441581
+      // Triggered by MediaTransportPropertyChanged. Stop watching on file
+      // descriptor if there is one.
+      break;
+    }
+    case device::BluetoothAudioSink::STATE_PENDING: {
+      // TODO(mcchou): BUG=441581
+      // Call BluetoothMediaTransportClient::Acquire() to get fd and mtus.
+      break;
+    }
+    case device::BluetoothAudioSink::STATE_ACTIVE: {
+      // TODO(mcchou): BUG=441581
+      // Read from fd and call DataAvailable.
+      ReadFromFD();
+      break;
+    }
+    default: {

[armansito, 2015/01/27 04:38:05]

No need for empty scope here.

[Miao, 2015/01/28 02:05:01]

Done.

+    }
+  }
+
   state_ = state;
   FOR_EACH_OBSERVER(device::BluetoothAudioSink::Observer, observers_,
                     BluetoothAudioSinkStateChanged(this, state_));
@@ -153,6 +249,26 @@ void BluetoothAudioSinkChromeOS::VolumeChanged(uint16_t volume) {
                     BluetoothAudioSinkVolumeChanged(this, volume_));
 }
 
+void BluetoothAudioSinkChromeOS::OnRegisterSucceeded(
+    const base::Closure& callback) {
+  VLOG(1) << "Bluetooth audio sink registerd";
+
+  StateChanged(device::BluetoothAudioSink::STATE_DISCONNECTED);
+  callback.Run();
+}
+
+void BluetoothAudioSinkChromeOS::OnRegisterFailed(
+    const BluetoothAudioSink::ErrorCallback& error_back,
+    const std::string& error_name,
+    const std::string& error_message) {
+  DCHECK(media_endpoint_.get());
+  VLOG(1) << "Bluetooth audio sink: " << error_name << ": " <<  error_message;
+
+  endpoint_path_ = dbus::ObjectPath("");
+  media_endpoint_ = nullptr;
+  error_back.Run(device::BluetoothAudioSink::ERROR_NOT_REGISTERED);
+}
+
 void BluetoothAudioSinkChromeOS::ReadFromFD() {
   DCHECK_GE(fd_.value(), 0);