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

chromeos/dbus/bluetooth_media_endpoint_service_provider.cc

Index: chromeos/dbus/bluetooth_media_endpoint_service_provider.cc
diff --git a/chromeos/dbus/bluetooth_media_endpoint_service_provider.cc b/chromeos/dbus/bluetooth_media_endpoint_service_provider.cc
index 6a2de74643d084b8057a91a686d397f5b599cb2d..265e315cc9bb8a80582429d9c7f996edd9555f77 100644
--- a/chromeos/dbus/bluetooth_media_endpoint_service_provider.cc
+++ b/chromeos/dbus/bluetooth_media_endpoint_service_provider.cc
@@ -4,13 +4,12 @@
 
 #include "chromeos/dbus/bluetooth_media_endpoint_service_provider.h"
 
-#include <string>
-
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/platform_thread.h"
+#include "chromeos/dbus/bluetooth_media_transport_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h"
 #include "dbus/bus.h"
@@ -23,7 +22,7 @@ namespace {
 // Bluetooth Media Endpoint service identifier.
 const char kBluetoothMediaEndpointInterface[] = "org.bluez.MediaEndpoint1";
 
-// Bluetooth Media Endpoint methods.
+// Method names in Bluetooth Media Endpoint interface.
 const char kSetConfiguration[] = "SetConfiguration";
 const char kSelectConfiguration[] = "SelectConfiguration";
 const char kClearConfiguration[] = "ClearConfiguration";
@@ -118,14 +117,50 @@ class CHROMEOS_EXPORT BluetoothMediaEndpointServiceProviderImpl
 
     dbus::MessageReader reader(method_call);
     dbus::ObjectPath transport_path;
-    dbus::MessageReader properties(method_call);
+    dbus::MessageReader property_reader(method_call);
     if (!reader.PopObjectPath(&transport_path) ||
-        !reader.PopArray(&properties)) {
+        !reader.PopArray(&property_reader)) {
       LOG(WARNING) << "SetConfiguration called with incorrect parameters: "
                    << method_call->ToString();
       return;
     }
 
+    // Parses |properties| and passes the property set as a
+    // Delegate::TransportProperties structure to |delegate_|.

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

Some of the properties below are mandatory and some are optional. For the
mandatory ones, it might make sense to log an error if any of them is missing
and perhaps not call the delegate method at all?

Either way you need unit tests for all of these cases.

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

Done.

+    Delegate::TransportProperties properties;
+    while (property_reader.HasMoreData()) {
+      dbus::MessageReader dict_entry_reader(nullptr);
+      std::string key;
+      if (!property_reader.PopDictEntry(&dict_entry_reader) ||
+          !dict_entry_reader.PopString(&key)) {
+        LOG(WARNING) << "SetConfiguration called with incorrect parameters: "
+                     << method_call->ToString();
+      } else {

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

nit: you can just continue with "else-if" here instead of "else { if ...}"

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

Done.

+        if (key == BluetoothMediaTransportClient::kDeviceProperty) {
+          dict_entry_reader.PopVariantOfObjectPath(&properties.device);
+        } else if (key == BluetoothMediaTransportClient::kUUIDProperty) {
+          dict_entry_reader.PopVariantOfString(&properties.uuid);
+        } else if (key == BluetoothMediaTransportClient::kCodecProperty) {
+          dict_entry_reader.PopVariantOfByte(&properties.codec);
+        } else if (key ==
+                   BluetoothMediaTransportClient::kConfigurationProperty) {
+          dbus::MessageReader variant_reader(nullptr);
+          const uint8_t* bytes = nullptr;
+          size_t length = 0;
+          dict_entry_reader.PopVariant(&variant_reader);
+          variant_reader.PopArrayOfBytes(&bytes, &length);
+          properties.configuration =
+              std::vector<uint8_t>(bytes, bytes + length);
+        } else if (key == BluetoothMediaTransportClient::kStateProperty) {
+          dict_entry_reader.PopVariantOfString(&properties.state);
+        } else if (key == BluetoothMediaTransportClient::kDelayProperty) {
+          dict_entry_reader.PopVariantOfUint16(properties.delay.get());

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

These two won't work! You're passing a NULL pointer to PopVariantOfUint16, which
will dereference the NULL pointer to assign the result and thus crash. You have
to initialize the memory here:

  properties.delay.reset(new uint16_t());
  dict_entry_reader.PopVariantOfUint16(properties.delay.get());

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

Done.

+        } else if (key == BluetoothMediaTransportClient::kVolumeProperty) {
+          dict_entry_reader.PopVariantOfUint16(properties.volume.get());
+        }
+      }
+    }
+
     delegate_->SetConfiguration(transport_path, properties);
 
     response_sender.Run(dbus::Response::FromMethodCall(method_call));
@@ -188,7 +223,7 @@ class CHROMEOS_EXPORT BluetoothMediaEndpointServiceProviderImpl
     DCHECK(OnOriginThread());
     DCHECK(delegate_);
 
-    delegate_->Release();
+    delegate_->Released();
 
     response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
@@ -241,11 +276,20 @@ class CHROMEOS_EXPORT BluetoothMediaEndpointServiceProviderImpl
   DISALLOW_COPY_AND_ASSIGN(BluetoothMediaEndpointServiceProviderImpl);
 };
 
+BluetoothMediaEndpointServiceProvider::Delegate::TransportProperties::
+    TransportProperties() : delay(nullptr), volume(nullptr) {

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

You should assign a default value for codec and state here.

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

Done. These default value can be used to check whether the transport property
set is initiated correctly or not.

+}
+
+BluetoothMediaEndpointServiceProvider::Delegate::TransportProperties::
+    ~TransportProperties() {
+}
+
 BluetoothMediaEndpointServiceProvider::BluetoothMediaEndpointServiceProvider() {
 }
 
-BluetoothMediaEndpointServiceProvider::~BluetoothMediaEndpointServiceProvider()
-{}
+BluetoothMediaEndpointServiceProvider::
+    ~BluetoothMediaEndpointServiceProvider() {
+}
 
 BluetoothMediaEndpointServiceProvider*
     BluetoothMediaEndpointServiceProvider::Create(