Last Comment Bug 758504 - [b2g-bluetooth] Implement function to register services
: [b2g-bluetooth] Implement function to register services
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Eric Chou [:ericchou] [:echou]
:
:
Mentors:
Depends on:
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-05-24 21:19 PDT by Eric Chou [:ericchou] [:echou]
Modified: 2012-08-10 01:57 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: patch1: Implement function to add reserved services (3.98 KB, patch)
2012-08-07 01:36 PDT, Eric Chou [:ericchou] [:echou]
kyle: review+
Details | Diff | Splinter Review
v1: patch 2: Implement function to remove reserved services (3.10 KB, patch)
2012-08-07 01:43 PDT, Eric Chou [:ericchou] [:echou]
kyle: review+
Details | Diff | Splinter Review
Final: Patch 1: Implement function to add reserved services, r=qDot (4.36 KB, patch)
2012-08-08 20:31 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Final: Patch 2: Implement function to remove reserved services, r=qDot (4.18 KB, patch)
2012-08-08 20:32 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review

Description Eric Chou [:ericchou] [:echou] 2012-05-24 21:19:49 PDT
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] 2012-05-24 21:26:54 PDT
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] 2012-08-07 01:36:09 PDT
Created attachment 649568 [details] [diff] [review]
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] 2012-08-07 01:43:50 PDT
Created attachment 649570 [details] [diff] [review]
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 [:kmachulis] [:qdot] 2012-08-07 15:42:11 PDT
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 [:kmachulis] [:qdot] 2012-08-07 15:49:57 PDT
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] 2012-08-08 01:58:35 PDT
> @@ +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] 2012-08-08 20:31:23 PDT
Created attachment 650428 [details] [diff] [review]
Final: Patch 1: Implement function to add reserved services, r=qDot

Nits picked.
Comment 8 Eric Chou [:ericchou] [:echou] 2012-08-08 20:32:47 PDT
Created attachment 650429 [details] [diff] [review]
Final: Patch 2: Implement function to remove reserved services, r=qDot

Nits picked & avoid potential memory leakage.
Comment 9 Kyle Machulis [:kmachulis] [:qdot] 2012-08-08 21:29:05 PDT
(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. :|

Note You need to log in before you can comment on or make changes to this bug.