Closed Bug 1140951 Opened 8 years ago Closed 8 years ago

[bluetooth2] Implement start/stopNotifications for GATT client API

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox40 fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S10 (17apr)
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: [webbt-api])

Attachments

(1 file, 2 obsolete files)

[Final] Bug 1140951 - Implement start/stopNotifications for GATT client API. r=shuang, r=mrbkap
25.62 KB, patch
Details | Diff | Splinter Review 
This bug is for implementing notification related APIs, which involved
  - BluetoothGatt.oncharacteristicchanged [1]
  - BluetoothGattCharacteristic.start/stopNotification() [2]

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#oncharacteristicchanged
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#startNotifications.28.29
typo, should be start/stopNotifications.
Summary: [bluetooth2] Implement start/stopNotification for GATT client API → [bluetooth2] Implement start/stopNotifications for GATT client API
typo, should be start/stopNotifications.
Depends on: 1114515
Per offline discussion with @shuang and @brsun, the first edition of this API will not include write CCCD behavior due to complexity.
And we also need to wait for write descriptor implementation to be done before including write CCCD behavior in this API.

Without write CCCD behavior in the first edition, users will need to call |descriptor.WriteValue| along with |start/stopNotifications|. (similar to Android API)

I will open a follow-up bug to revise the API implementation later.
Hi Ben,

This patch is implementing the first edition of |start/stopNotifications| API which doesn't include write CCCD behavior at this moment.
Could you help to review my patch?

Thanks,
Jocelyn
Attachment #8582146 - Flags: review?(btian)
Comment on attachment 8582146 [details] [diff] [review]
Bug 1140951(v1) - Implement start/stopNotifications for GATT client API

Redirect review to Shawn as I will be OOO from tomorrow.
Attachment #8582146 - Flags: review?(btian) → review?(shuang)
Comment on attachment 8582146 [details] [diff] [review]
Bug 1140951(v1) - Implement start/stopNotifications for GATT client API

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

::: dom/bluetooth2/BluetoothService.h
@@ +351,5 @@
>                                 BluetoothReplyRunnable* aRunnable) = 0;
>  
>    /**
> +   * Enable notifications of a given GATT characteristic.
> +   * (platform specific implementation)

Can you elaborate the reason why it's platform specific implementation?
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Comment on attachment 8582146 [details] [diff] [review]
> Bug 1140951(v1) - Implement start/stopNotifications for GATT client API
> 
> Review of attachment 8582146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth2/BluetoothService.h
> @@ +351,5 @@
> >                                 BluetoothReplyRunnable* aRunnable) = 0;
> >  
> >    /**
> > +   * Enable notifications of a given GATT characteristic.
> > +   * (platform specific implementation)
> 
> Can you elaborate the reason why it's platform specific implementation?

It's the same as other functions in BluetoothService.h.
We will have different implementation in DBusService and ServiceBluedroid.
I've experienced a crash issue in b2g process while starting notification:

Program received signal SIGSEGV, Segmentation fault.
nsRefPtr<gfxFontFamily>::nsRefPtr (this=0xbeae97e0, aSmartPtr=...) at ../../dist/include/nsRefPtr.h:77
77	      mRawPtr->AddRef();
(gdb) bt
#0  nsRefPtr<gfxFontFamily>::nsRefPtr (this=0xbeae97e0, aSmartPtr=...) at ../../dist/include/nsRefPtr.h:77
#1  0xb56e7192 in mozilla::dom::bluetooth::BluetoothGattManager::RegisterNotificationNotification (this=<optimized out>, aConnId=25737, aIsRegister=1, 
    aStatus=mozilla::dom::bluetooth::GATT_STATUS_SUCCESS, aServiceId=..., aCharId=...) at ../../../../mozilla-central-working/dom/bluetooth2/bluedroid/BluetoothGattManager.cpp:1126
#2  0xb56e4c24 in mozilla::dom::bluetooth::BluetoothNotificationHALRunnable5<mozilla::dom::bluetooth::BluetoothGattClientCallback::GattClientNotificationHandlerWrapper, void, int, int, mozilla::dom::bluetooth::BluetoothGattStatus, mozilla::dom::bluetooth::BluetoothGattServiceId, mozilla::dom::bluetooth::BluetoothGattId, int, int, mozilla::dom::bluetooth::BluetoothGattStatus, mozilla::dom::bluetooth::BluetoothGattServiceId const&, mozilla::dom::bluetooth::BluetoothGattId const&>::Run (this=<optimized out>) at ../../../../mozilla-central-working/dom/bluetooth2/bluedroid/BluetoothHALHelpers.h:1437
#3  0xb4c73340 in nsThread::ProcessNextEvent (this=0xb6a025c0, aMayWait=<optimized out>, aResult=0xbeae986f) at ../../../../mozilla-central-working/xpcom/threads/nsThread.cpp:855
#4  0xb4c80f1c in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=true) at /home/bruce_sun/Source/mozilla-central-working/xpcom/glue/nsThreadUtils.cpp:265
#5  0xb4dc4536 in mozilla::ipc::MessagePump::Run (this=0xb2fdb250, aDelegate=0xb2fcd1a0) at ../../../../mozilla-central-working/ipc/glue/MessagePump.cpp:140
#6  0xb4db8754 in MessageLoop::RunInternal (this=this@entry=0xb2fcd1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:233
#7  0xb4db8808 in RunHandler (this=0xb2fcd1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:226
#8  MessageLoop::Run (this=0xb2fcd1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:200
#9  0xb56f8fee in nsBaseAppShell::Run (this=0xb2960280) at ../../../mozilla-central-working/widget/nsBaseAppShell.cpp:164
#10 0xb5a1ad06 in nsAppStartup::Run (this=0xb295ff40) at ../../../../../mozilla-central-working/toolkit/components/startup/nsAppStartup.cpp:281
#11 0xb5a3a2f2 in XREMain::XRE_mainRun (this=this@entry=0xbeae99d0) at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4202
#12 0xb5a3b3d6 in XREMain::XRE_main (this=this@entry=0xbeae99d0, argc=argc@entry=1, argv=argv@entry=0xb6a2b190, aAppData=aAppData@entry=0xb6f72858 <_ZL8sAppData>)
    at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4278
#13 0xb5a3b556 in XRE_main (argc=1, argv=0xb6a2b190, aAppData=0xb6f72858 <_ZL8sAppData>, aFlags=<optimized out>) at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4498
#14 0xb6f57a68 in do_main (argc=argc@entry=1, argv=argv@entry=0xb6a2b190) at ../../../../mozilla-central-working/b2g/app/nsBrowserApp.cpp:167
#15 0xb6f57b76 in b2g_main (argc=argc@entry=1, argv=argv@entry=0xbeaeac84) at ../../../../mozilla-central-working/b2g/app/nsBrowserApp.cpp:299
#16 0xb6f578f8 in RunProcesses (aReservedFds=..., argv=0xbeaeac84, argc=1) at ../../../../mozilla-central-working/b2g/app/B2GLoader.cpp:225
#17 main (argc=1, argv=0xbeaeac84) at ../../../../mozilla-central-working/b2g/app/B2GLoader.cpp:290
Since there are no hints that input parameters cannot carry an invalid value, suggest to check the input parameter of BluetoothGattManager::AnyFunction() and return errors gracefully instead of using MOZ_ASSERT() and crash directly.
(In reply to Bruce Sun [:brsun] from comment #9)
> Since there are no hints that input parameters cannot carry an invalid
> value, suggest to check the input parameter of
> BluetoothGattManager::AnyFunction() and return errors gracefully instead of
> using MOZ_ASSERT() and crash directly.

Sorry, this crash is due to wrong value callbacked from the underlying Bluetooth stack. Using MOZ_ASSERT() is suitable in this case. Please ignore my previous comment. Thanks.
(In reply to Bruce Sun [:brsun] from comment #10)
> (In reply to Bruce Sun [:brsun] from comment #9)
> > Since there are no hints that input parameters cannot carry an invalid
> > value, suggest to check the input parameter of
> > BluetoothGattManager::AnyFunction() and return errors gracefully instead of
> > using MOZ_ASSERT() and crash directly.
> 
> Sorry, this crash is due to wrong value callbacked from the underlying
> Bluetooth stack. Using MOZ_ASSERT() is suitable in this case. Please ignore
> my previous comment. Thanks.
MOZ_ASSERT only works for debug build. So we'd better handle this case, although this looks like a bug in bluedroid and Android somehow doesn't check the invalid value.
Blocks: 1149006
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> (In reply to Bruce Sun [:brsun] from comment #10)
> > (In reply to Bruce Sun [:brsun] from comment #9)
> > > Since there are no hints that input parameters cannot carry an invalid
> > > value, suggest to check the input parameter of
> > > BluetoothGattManager::AnyFunction() and return errors gracefully instead of
> > > using MOZ_ASSERT() and crash directly.
> > 
> > Sorry, this crash is due to wrong value callbacked from the underlying
> > Bluetooth stack. Using MOZ_ASSERT() is suitable in this case. Please ignore
> > my previous comment. Thanks.
> MOZ_ASSERT only works for debug build. So we'd better handle this case,
> although this looks like a bug in bluedroid and Android somehow doesn't
> check the invalid value.

Since bluedroid returning a wrong connId, from the information we have in the notification callback,
we couldn't map back to a existing client.
The only workaround I have in mind is to resolve/reject promise directly in the result handler.
I have checked bluedroid stack for the cases that it will return error in the notification callback.

startNotifications:
It will return error GATT_STATUS if characteristic is not found or client if is not found which is unlikely to happen in our cases.

Another case in http://androidxref.com/4.4.4_r1/xref/external/bluetooth/bluedroid/bta/gatt/bta_gattc_api.c#916, bluedroid checks if this app has already registered notifications to 7 characteristics.
It might happen, but it's pretty ugly if we introduce this bluedroid specific stuff into gecko.

stopNotifications: check if the existence of clientIf, characteristic, and registration of notification.
We are unlikely to hit these error cases based on our implementation.

In summary, I would like to return promise in the result handler, but I'm a bit concerned of the BTA_GATTC_NOTIF_REG_MAX case.
Shawn, any suggestions on this?
Flags: needinfo?(shuang)
(In reply to jocelyn [:jocelyn] from comment #12)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> > (In reply to Bruce Sun [:brsun] from comment #10)
> > > (In reply to Bruce Sun [:brsun] from comment #9)
> > > > Since there are no hints that input parameters cannot carry an invalid
> > > > value, suggest to check the input parameter of
> > > > BluetoothGattManager::AnyFunction() and return errors gracefully instead of
> > > > using MOZ_ASSERT() and crash directly.
> > > 
> > > Sorry, this crash is due to wrong value callbacked from the underlying
> > > Bluetooth stack. Using MOZ_ASSERT() is suitable in this case. Please ignore
> > > my previous comment. Thanks.
> > MOZ_ASSERT only works for debug build. So we'd better handle this case,
> > although this looks like a bug in bluedroid and Android somehow doesn't
> > check the invalid value.
> 
> Since bluedroid returning a wrong connId, from the information we have in
> the notification callback,
> we couldn't map back to a existing client.
> The only workaround I have in mind is to resolve/reject promise directly in
> the result handler.
> I have checked bluedroid stack for the cases that it will return error in
> the notification callback.
> 
> startNotifications:
> It will return error GATT_STATUS if characteristic is not found or client if
> is not found which is unlikely to happen in our cases.
> 
> Another case in
> http://androidxref.com/4.4.4_r1/xref/external/bluetooth/bluedroid/bta/gatt/
> bta_gattc_api.c#916, bluedroid checks if this app has already registered
> notifications to 7 characteristics.
> It might happen, but it's pretty ugly if we introduce this bluedroid
> specific stuff into gecko.
> 
> stopNotifications: check if the existence of clientIf, characteristic, and
> registration of notification.
> We are unlikely to hit these error cases based on our implementation.
> 
> In summary, I would like to return promise in the result handler, but I'm a
> bit concerned of the BTA_GATTC_NOTIF_REG_MAX case.
> Shawn, any suggestions on this?

Let's open a bug to AOSP, see if they are willing to fix in upstream, although I'm not sure the same thing happened in 5.1 stack.
 
We can do short term workaround for now, but in the long term we shall fix in upstream (hopefully).
For the BTA_GATTC_NOTIF_REG_MAX case, I'm thinking we can add one static variable to track total number of registration? 

See: https://code.google.com/p/android/issues/list.
https://source.android.com/source/life-of-a-bug.html
Flags: needinfo?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #13)
> (In reply to jocelyn [:jocelyn] from comment #12)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> > > (In reply to Bruce Sun [:brsun] from comment #10)
> > > > (In reply to Bruce Sun [:brsun] from comment #9)
> > > > > Since there are no hints that input parameters cannot carry an invalid
> > > > > value, suggest to check the input parameter of
> > > > > BluetoothGattManager::AnyFunction() and return errors gracefully instead of
> > > > > using MOZ_ASSERT() and crash directly.
> > > > 
> > > > Sorry, this crash is due to wrong value callbacked from the underlying
> > > > Bluetooth stack. Using MOZ_ASSERT() is suitable in this case. Please ignore
> > > > my previous comment. Thanks.
> > > MOZ_ASSERT only works for debug build. So we'd better handle this case,
> > > although this looks like a bug in bluedroid and Android somehow doesn't
> > > check the invalid value.
> > 
> > Since bluedroid returning a wrong connId, from the information we have in
> > the notification callback,
> > we couldn't map back to a existing client.
> > The only workaround I have in mind is to resolve/reject promise directly in
> > the result handler.
> > I have checked bluedroid stack for the cases that it will return error in
> > the notification callback.
> > 
> > startNotifications:
> > It will return error GATT_STATUS if characteristic is not found or client if
> > is not found which is unlikely to happen in our cases.
> > 
> > Another case in
> > http://androidxref.com/4.4.4_r1/xref/external/bluetooth/bluedroid/bta/gatt/
> > bta_gattc_api.c#916, bluedroid checks if this app has already registered
> > notifications to 7 characteristics.
> > It might happen, but it's pretty ugly if we introduce this bluedroid
> > specific stuff into gecko.
> > 
> > stopNotifications: check if the existence of clientIf, characteristic, and
> > registration of notification.
> > We are unlikely to hit these error cases based on our implementation.
> > 
> > In summary, I would like to return promise in the result handler, but I'm a
> > bit concerned of the BTA_GATTC_NOTIF_REG_MAX case.
> > Shawn, any suggestions on this?
> 
> Let's open a bug to AOSP, see if they are willing to fix in upstream,
> although I'm not sure the same thing happened in 5.1 stack.
>  
> We can do short term workaround for now, but in the long term we shall fix
> in upstream (hopefully).
> For the BTA_GATTC_NOTIF_REG_MAX case, I'm thinking we can add one static
> variable to track total number of registration? 
> 
> See: https://code.google.com/p/android/issues/list.
> https://source.android.com/source/life-of-a-bug.html

It's doable to track the registration count, I'm just concerned it's not good to introduce this into gecko.
If there's no other possible ways, I suppose what I can do is just add it and fix it when the upstream fixed(hopefully...).
(In reply to jocelyn [:jocelyn] from comment #14)
> It's doable to track the registration count, I'm just concerned it's not
> good to introduce this into gecko.
> If there's no other possible ways, I suppose what I can do is just add it
> and fix it when the upstream fixed(hopefully...).
I just want to double confirm again. Have you actually tried on real Android device and check the log this conn_Id is also wrong?
Flags: needinfo?(joliu)
Ok, I tested the latest android 5.1 with application calling android bluetooth api, the connId is wrong, so this is why address=null (can not map into correct value).

03-30 15:04:01.382 D/BtGatt.GattService( 4865): registerForNotification() - address=CF:8F:D6:7C:BA:C1 enable: true
03-30 15:04:01.382 D/BtGatt.GattService( 4865): onRegisterForNotifications() - address=null, status=0, registered=1, charUuid=00002a37-0000-1000-8000-00805f9b34fb
Canel ni, I tested with Android device, and confirm this is a stack bug.
Flags: needinfo?(joliu)
Blocks: 1149043
- Resolve/reject promise in result handlers instead of notification callback due to stack bug.

Hi Shawn,

I have revised the patch to reply promise in the result handler instead of notification callback, could you help to review my patch?

Thanks,
Jocelyn
Attachment #8582146 - Attachment is obsolete: true
Attachment #8582146 - Flags: review?(shuang)
Attachment #8585328 - Flags: review?(shuang)
(In reply to jocelyn [:jocelyn] from comment #18)
> Created attachment 8585328 [details] [diff] [review]
> Bug 1140951(v2) - Implement start/stopNotifications for GATT client API
> 
> - Resolve/reject promise in result handlers instead of notification callback
> due to stack bug.
> 
> Hi Shawn,
> 
> I have revised the patch to reply promise in the result handler instead of
> notification callback, could you help to review my patch?
> 
> Thanks,
> Jocelyn
I opened AOSP bug.
https://code.google.com/p/android/issues/detail?id=162386&thanks=162386&ts=1427701308
Comment on attachment 8585328 [details] [diff] [review]
Bug 1140951(v2) - Implement start/stopNotifications for GATT client API

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

Overall, it looks good to me, although adding this workaround is a hard decision. I only have a few comments, and you need to DOM peer to review webidl. :)

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +674,5 @@
> +      (int)aStatus);
> +    MOZ_ASSERT(mClient->mRegisterNotificationsRunnable);
> +
> +    DispatchReplyError(mClient->mRegisterNotificationsRunnable,
> +                       NS_LITERAL_STRING("RegisterNotifications failed"));

I think we should somehow document the error string in wiki or MDN.

@@ +692,5 @@
> +  MOZ_ASSERT(aRunnable);
> +
> +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());

Is there any chance that UnregisterClient was been called earlier and you cannot get a valid index?

@@ +707,5 @@
> +  }
> +
> +  client->mRegisterNotificationsRunnable = aRunnable;
> +
> +  sBluetoothGattClientInterface->RegisterNotification(

Can we add |MOZ_ASSERT(sBluetoothGattClientInterface)|?
Attachment #8585328 - Flags: review?(shuang) → review+
Thanks for reviewing, I am not a fan of workaround either. :(

(In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> Comment on attachment 8585328 [details] [diff] [review]
> Bug 1140951(v2) - Implement start/stopNotifications for GATT client API
> 
> Review of attachment 8585328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, it looks good to me, although adding this workaround is a hard
> decision. I only have a few comments, and you need to DOM peer to review
> webidl. :)
> 
> ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
> @@ +674,5 @@
> > +      (int)aStatus);
> > +    MOZ_ASSERT(mClient->mRegisterNotificationsRunnable);
> > +
> > +    DispatchReplyError(mClient->mRegisterNotificationsRunnable,
> > +                       NS_LITERAL_STRING("RegisterNotifications failed"));
> 
> I think we should somehow document the error string in wiki or MDN.
> 

Actually, this string is not exposed to applications.
In this case, we will only use STATUS_FAIL[1] and generate an nsError from it[2].
This string in this case will only serve as an gecko error log.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothUtils.cpp#249
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothReplyRunnable.cpp#85-88

> @@ +692,5 @@
> > +  MOZ_ASSERT(aRunnable);
> > +
> > +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> > +
> > +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> 
> Is there any chance that UnregisterClient was been called earlier and you
> cannot get a valid index?
> 

UnregisterClient will only be called in |DisconnectFromOwner| and |~BluetoothGatt|, I think it's not possible to make requests after those thing happened.

> @@ +707,5 @@
> > +  }
> > +
> > +  client->mRegisterNotificationsRunnable = aRunnable;
> > +
> > +  sBluetoothGattClientInterface->RegisterNotification(
> 
> Can we add |MOZ_ASSERT(sBluetoothGattClientInterface)|?

It has already been covered by |ENSURE_GATT_CLIENT_INTF_IS_READY_VOID|.
(In reply to jocelyn [:jocelyn] from comment #21)
> Thanks for reviewing, I am not a fan of workaround either. :(
> 
> (In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> > Comment on attachment 8585328 [details] [diff] [review]
> > Bug 1140951(v2) - Implement start/stopNotifications for GATT client API
> > 
> > Review of attachment 8585328 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Overall, it looks good to me, although adding this workaround is a hard
> > decision. I only have a few comments, and you need to DOM peer to review
> > webidl. :)
> > 
> > ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
> > @@ +674,5 @@
> > > +      (int)aStatus);
> > > +    MOZ_ASSERT(mClient->mRegisterNotificationsRunnable);
> > > +
> > > +    DispatchReplyError(mClient->mRegisterNotificationsRunnable,
> > > +                       NS_LITERAL_STRING("RegisterNotifications failed"));
> > 
> > I think we should somehow document the error string in wiki or MDN.
> > 
> 
> Actually, this string is not exposed to applications.
> In this case, we will only use STATUS_FAIL[1] and generate an nsError from
> it[2].
> This string in this case will only serve as an gecko error log.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothUtils.
> cpp#249
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/
> BluetoothReplyRunnable.cpp#85-88
> 
> > @@ +692,5 @@
> > > +  MOZ_ASSERT(aRunnable);
> > > +
> > > +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> > > +
> > > +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> > 
> > Is there any chance that UnregisterClient was been called earlier and you
> > cannot get a valid index?
> > 
> 
> UnregisterClient will only be called in |DisconnectFromOwner| and
> |~BluetoothGatt|, I think it's not possible to make requests after those
> thing happened.
> 
Ok.
> > @@ +707,5 @@
> > > +  }
> > > +
> > > +  client->mRegisterNotificationsRunnable = aRunnable;
> > > +
> > > +  sBluetoothGattClientInterface->RegisterNotification(
> > 
> > Can we add |MOZ_ASSERT(sBluetoothGattClientInterface)|?
> 
> It has already been covered by |ENSURE_GATT_CLIENT_INTF_IS_READY_VOID|.

OK.
Comment on attachment 8585328 [details] [diff] [review]
Bug 1140951(v2) - Implement start/stopNotifications for GATT client API

Hi Blake,

This bug is for adding start/stopNotifications() of GATT client API.
Could you help to review it from DOM peer point of view?

Patch summary
1) BluetoothGattCharacteristic.webidl/cpp/h: Add start/stopNotifications() method into GattCharacteristic interface and implement them.
2) bluedroid/BluetoothGattManger.cpp/h: Interact with bluetooth stack
3) Others: ipc files

Thanks,
Jocelyn
Attachment #8585328 - Flags: review?(mrbkap)
Attachment #8585328 - Flags: review?(mrbkap) → review+
No try server result since bluetooth2 won't be built.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/78302294ed38
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
You need to log in before you can comment on or make changes to this bug.