758504 - [b2g-bluetooth] Implement function to register services

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Eric Chou [:ericchou] [:echou]]

Others devices can retrieve what services our device is providing now from SDP Service and Attribute queries. So we need an interface for applications to register/unregister its RFCOMM service to local SDP server.

Comment 1

[Eric Chou [:ericchou] [:echou]]

Sorry, we may not provide unregister to application in order to avoid accidentally unregistering some system built-in services.

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: v1: patch1: Implement function to add reserved services

The word 'Reserved Services' is from BlueZ's implementation. There are 4 reserved services defined in BlueZ, which are PBAP_PSE, HEADSET_AG, HANDSFREE_AG and OBEX_OBJPUSH. 

(For more information, see add_reserved_service_records() in external/bluetooth/bluez/src/adapter.c)

Comment 3

[Eric Chou [:ericchou] [:echou]]

Attachment: v1: patch 2: Implement function to remove reserved services

We still need a function to remove added reserved services because those services may be added every time after turning on Bluetooth.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 649568 [details] [diff] [review]
v1: patch1: Implement function to add reserved services

Review of attachment 649568 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1217,5 @@
> +                            DBUS_TYPE_INVALID)) {
> +    if (!handles) {
> +      LOG("Null array in extract_handles");
> +    } else {
> +      for (int i = 0;i < len;++i) {

Nit: space after ;'s in fors.

@@ +1244,5 @@
> +                                      DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32,
> +                                      &services, length, DBUS_TYPE_INVALID);
> +
> +  if (!reply) {
> +    LOG("Null DBus message. Couldn't extract handles.");

Nit: return early here, kill else block.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 649570 [details] [diff] [review]
v1: patch 2: Implement function to remove reserved services

Review of attachment 649570 [details] [diff] [review]:
-----------------------------------------------------------------

r+  with nits picked and question addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1255,5 @@
>  
>  bool
> +BluetoothDBusService::RemoveReservedServicesInternal(const nsAString& aAdapterPath,
> +                                                     const nsTArray<PRUint32>& aServiceHandles)
> +{

Nit: Blocks, shouldn't be called on Main thread, add thread check.

@@ +1256,5 @@
>  bool
> +BluetoothDBusService::RemoveReservedServicesInternal(const nsAString& aAdapterPath,
> +                                                     const nsTArray<PRUint32>& aServiceHandles)
> +{
> +  const char* adapterPath = NS_ConvertUTF16toUTF8(aAdapterPath).get();

Nit: Intermediate isn't needed, we can just pass this in to dbus_func_args.

@@ +1258,5 @@
> +                                                     const nsTArray<PRUint32>& aServiceHandles)
> +{
> +  const char* adapterPath = NS_ConvertUTF16toUTF8(aAdapterPath).get();
> +
> +  DBusMessage *msg = NULL;

Nit: This isn't used?

@@ +1267,5 @@
> +
> +  const uint32_t* services = aServiceHandles.Elements();
> +
> +  reply = dbus_func_args(mConnection,
> +                         adapterPath,                    

Nit: Remove trailing space

@@ +1272,5 @@
> +                         DBUS_ADAPTER_IFACE, "RemoveReservedServiceRecords",
> +                         DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32,
> +                         &services, length, DBUS_TYPE_INVALID);
> +
> +  return reply ? true : false;

Are we expected to deallocate reply ourselves?

Comment 6

[Eric Chou [:ericchou] [:echou]]

> @@ +1272,5 @@
> > +                         DBUS_ADAPTER_IFACE, "RemoveReservedServiceRecords",
> > +                         DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32,
> > +                         &services, length, DBUS_TYPE_INVALID);
> > +
> > +  return reply ? true : false;
> 
> Are we expected to deallocate reply ourselves?

Interesting. Looks like a potential memory leak in Android. :P

Comment 7

[Eric Chou [:ericchou] [:echou]]

Attachment: Final: Patch 1: Implement function to add reserved services, r=qDot

Nits picked.

Comment 8

[Eric Chou [:ericchou] [:echou]]

Attachment: Final: Patch 2: Implement function to remove reserved services, r=qDot

Nits picked & avoid potential memory leakage.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #6)

> Interesting. Looks like a potential memory leak in Android. :P

Yeah. A /big/ portion of the kickbacks on bug 740744 were me having to fix all the stuff I'd cut and pasted from the event loop code. :|

Comment 10

[Eric Chou [:ericchou] [:echou]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5459232d867
http://hg.mozilla.org/integration/mozilla-inbound/rev/4f31d157cf6b

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5459232d867
https://hg.mozilla.org/mozilla-central/rev/4f31d157cf6b