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

chromeos/dbus/bluetooth_media_endpoint_service_provider.cc

 // Copyright 2014 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 "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"
 #include "dbus/exported_object.h"
 #include "dbus/message.h"
 
 namespace {
 
 // TODO(mcchou): Move these constants to dbus/service_constants.h.
 // 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";
 const char kRelease[] = "Release";
 
 }  // namespace
 
 namespace chromeos {
 
 // The BluetoothMediaEndopintServiceProvider implementation used in production.
@@ skipping 74 matching lines @@
   }
 
   // Called by dbus:: when the remote device connects to the Media Endpoint.
   void SetConfiguration(dbus::MethodCall* method_call,
                         dbus::ExportedObject::ResponseSender response_sender) {
     DCHECK(OnOriginThread());
     DCHECK(delegate_);
 
     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));
   }
 
   // Called by dbus:: when the remote device receives the configuration for
   // media transport.
   void SelectConfiguration(
       dbus::MethodCall* method_call,
       dbus::ExportedObject::ResponseSender response_sender) {
@@ skipping 42 matching lines @@
     response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Called by Bluetooth daemon to do the clean up after unregistering the Media
   // Endpoint.
   void Release(dbus::MethodCall* method_call,
                dbus::ExportedObject::ResponseSender response_sender) {
     DCHECK(OnOriginThread());
     DCHECK(delegate_);
 
-    delegate_->Release();
+    delegate_->Released();
 
     response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Called by Delegate to response to a method requiring transport
   // configuration.
   void OnConfiguration(dbus::MethodCall* method_call,
                        dbus::ExportedObject::ResponseSender response_sender,
                        const std::vector<uint8_t>& configuration) {
     DCHECK(OnOriginThread());
@@ skipping 32 matching lines @@
   // Weak pointer factory for generating 'this' printers that might live longer
   // than we do.
   // Note This should remain the last member so it'll be destroyed and
   // invalidate it's weak pointers before any other members are destroyed.
   base::WeakPtrFactory<BluetoothMediaEndpointServiceProviderImpl>
       weak_ptr_factory_;
 
   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(
     dbus::Bus* bus,
     const dbus::ObjectPath& object_path,
     Delegate* delegate) {
   // Returns a real implementation.
   if (!DBusThreadManager::Get()->IsUsingStub(DBusClientBundle::BLUETOOTH)) {
     return new BluetoothMediaEndpointServiceProviderImpl(
         bus, object_path, delegate);
   }
   // Returns a fake implementation.
   return new FakeBluetoothMediaEndpointServiceProvider(object_path, delegate);
 }
 
 }  // namespace chromeos