1181478 - Expose BluetoothGattServer object in BluetoothAdapter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

Expose BluetoothGattServer:
 - Implement the skeleton of BluetoothGattServer
 - Make BluetoothAdapter.gattServer accessible

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_object.patch

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8631525 [details] [diff] [review]
bug1181478_bluetoothgattserver_object.patch

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

Please see my comment below.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +408,5 @@
>        mLeScanHandleArray.Clear();
>      }
> +
> +    if (mState != BluetoothAdapterState::Enabled) {
> +      mGattServer = nullptr;

Move this line into |if (mState == BluetoothAdapterState::Disabled)| since |mState| becomes either 'Enabled' or 'Disabled'.

@@ +1005,5 @@
>    }
>  
>    mState = aState;
>  
> +  if (mState != BluetoothAdapterState::Enabled) {

How to handle |Disable| failure that changes state Disabling -> Enabled? Can we set |mGattServer| as nullptr only when state is Disabled?

::: dom/webidl/BluetoothGattServer.webidl
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

Revise to IDL as [1]. Please correct other bluetooth related webidl files as well, either in another patch or a follow-up bug. 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/BluetoothManager2.webidl#1

Comment 3

[Bruce Sun [:brsun]]

> @@ +1005,5 @@
> >    }
> >  
> >    mState = aState;
> >  
> > +  if (mState != BluetoothAdapterState::Enabled) {
> 
> How to handle |Disable| failure that changes state Disabling -> Enabled? Can
> we set |mGattServer| as nullptr only when state is Disabled?
> 

I will set |mGattServer| to nullptr when state is Disabled as suggested.

> ::: dom/webidl/BluetoothGattServer.webidl
> @@ +1,2 @@
> > +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> > +/* vim: set ts=2 et sw=2 tw=80: */
> 
> Revise to IDL as [1]. Please correct other bluetooth related webidl files as
> well, either in another patch or a follow-up bug. 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/BluetoothManager2.
> webidl#1

I will issue another bug for other webidl files.

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_object.v2.patch

Address comment 2.

Comment 5

[Bruce Sun [:brsun]]

> I will issue another bug for other webidl files.

Bug 1184868 is the follow-up bug.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8635165 [details] [diff] [review]
bug1181478_bluetoothgattserver_object.v2.patch

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

r=me with comment addressed. Please request DOM peer for webidl review.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +364,5 @@
>  
> +BluetoothGattServer*
> +BluetoothAdapter::GetGattServer()
> +{
> +  if (mState != BluetoothAdapterState::Enabled) {

Leave comment to explain 1) different state condition to access and deinit of |gattServer| and 2) the reason of such difference.
- access: only when adapter is Enabled
- deinit: only when adapter is Disabled

::: dom/webidl/BluetoothGattServer.webidl
@@ +5,5 @@
> + */
> +
> +[CheckPermissions="bluetooth"]
> +interface BluetoothGattServer
> +{

Leave comment to note implementation will come later (or in bug xxx).

Comment 7

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_object.v3.patch

Hi Blake,

We are going to expose BluetoothGattServer[1] as an attribute of BluetoothAdapter[2]. This bug is for exposing |BluetoothAdapter.gattServer| attribute only, and all the rest tasks are split into dependent bugs of bug 933358.

Would you please help to review our patches regarding to the WebIDL part? Any comments will be appreciated.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#gattServer

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8635914 [details] [diff] [review]
bug1181478_bluetoothgattserver_object.v3.patch

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

I have a couple of questions that I'd like to see answered before I stamp this.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +365,5 @@
> +BluetoothGattServer*
> +BluetoothAdapter::GetGattServer()
> +{
> +  // Only expose GATT server if the adapter is enabled
> +  if (mState != BluetoothAdapterState::Enabled) {

What is this supposed to do, exactly? What's the expected behavior if JS holds a reference to the GATT server object and toggles the bluetooth adapter off and on? Will the old object continue to work? Will stop working?

@@ +1004,5 @@
>  
>    mState = aState;
>  
> +  if (mState == BluetoothAdapterState::Disabled) {
> +    mGattServer = nullptr;

This isn't guaranteed to destroy this object if JS also has a reference to it...

::: dom/bluetooth/bluetooth2/BluetoothGattServer.h
@@ +14,5 @@
> +#include "nsWrapperCache.h"
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +class BluetoothGattServer final : public nsISupports

This will eventually be an event target, right?

Comment 9

[Bruce Sun [:brsun]]

Thanks Blake, please kindly see comments inline.

> ::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
> @@ +365,5 @@
> > +BluetoothGattServer*
> > +BluetoothAdapter::GetGattServer()
> > +{
> > +  // Only expose GATT server if the adapter is enabled
> > +  if (mState != BluetoothAdapterState::Enabled) {
> 
> What is this supposed to do, exactly? What's the expected behavior if JS
> holds a reference to the GATT server object and toggles the bluetooth
> adapter off and on? Will the old object continue to work? Will stop working?
> 

If JS holds a reference to the GATT server object and the BluetoothAdapter is turned off, supposedly that old GATT server object should stop working till the end of life. If the BluetoothAdapter is turned on again, a new instance of GATT server will be created once BluetoothAdapter.gattServer has been accessed. And in the meantime, the old object should keep non-working.

> @@ +1004,5 @@
> >  
> >    mState = aState;
> >  
> > +  if (mState == BluetoothAdapterState::Disabled) {
> > +    mGattServer = nullptr;
> 
> This isn't guaranteed to destroy this object if JS also has a reference to
> it...
> 

Thanks for pointing this out. Yeah, supposedly some tear down function should be called at this point as well.

> ::: dom/bluetooth/bluetooth2/BluetoothGattServer.h
> @@ +14,5 @@
> > +#include "nsWrapperCache.h"
> > +
> > +BEGIN_BLUETOOTH_NAMESPACE
> > +
> > +class BluetoothGattServer final : public nsISupports
> 
> This will eventually be an event target, right?

Correct. It should be an event target eventually, and the efforts of corresponding changes are planed to be handled on other bugs.


I am going to add a tear down function in BluetoothGattServer and prepare another patch later on. Do you think the above information suffice your questions already?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8635914 [details] [diff] [review]
bug1181478_bluetoothgattserver_object.v3.patch

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

Yep. That answers my questions.

Comment 11

[Bruce Sun [:brsun]]

Hi Ben, I am going to add comments for |BluetoothAdapter.mGattServer| as following. Would you mind having a glance on it to see if those wordings should be rephrased or not?

> ::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
> @@ +364,5 @@
> >  
> > +BluetoothGattServer*
> > +BluetoothAdapter::GetGattServer()
> > +{
> > +  if (mState != BluetoothAdapterState::Enabled) {
> 
> Leave comment to explain 1) different state condition to access and deinit
> of |gattServer| and 2) the reason of such difference.
> - access: only when adapter is Enabled
> - deinit: only when adapter is Disabled
> 

  /**
   * GATT server object of this adapter.
   *
   * A new GATT server object will be created at the first time when
   * |GetGattServer| is called after the adapter has been enabled. If the
   * adapter has been disabled later on, the created GATT server will be
   * discard by the adapter, and this GATT object should stop working till the
   * end of its life. When |GetGattServer| is called after the adapter has been
   * enabled again, a new GATT server object will be created.
   */
  nsRefPtr<BluetoothGattServer> mGattServer;

Comment 12

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_object.commit.patch

Got feedback from Ben offline. The reason to expose BluetoothGattServer in the enabled state but to invalidate it in the disabled state is added in this patch.

Comment 13

[Bruce Sun [:brsun]]

Treeherder results seem good:
 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=967e1b99c39a

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/beed8e3e86d9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/beed8e3e86d9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G test_interfaces.html failures. Any particular reason that the Try push didn't include mochitests?
https://treeherder.mozilla.org/logviewer.html#?job_id=2391648&repo=b2g-inbound

https://hg.mozilla.org/mozilla-central/rev/5856a328963d

Comment 17

[Bruce Sun [:brsun]]

Well...no. Sorry about that. I missed the information "Note that if you're adding new interfaces, then the test at dom/tests/mochitest/general/test_interfaces.html will most likely fail." on [1].

[1] WebIDL bindings - https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Comment 18

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_interface_mochitest.patch

Hi Blake,

This patch contains corresponding changes for mochitest regarding to BluetoothGattServer. Would you please help to review it?

Comment 19

[Bruce Sun [:brsun]]

Attachment: bug1181478_bluetoothgattserver_interface_mochitest.commit.patch

Comment 20

[Bruce Sun [:brsun]]

Build status, marionette tests, mochitests seem good on Treeherder:
 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3bf4721298b

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ba34dfa90df7

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba34dfa90df7