Expose BluetoothGattServer object in BluetoothAdapter

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

({dev-doc-needed})

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Expose BluetoothGattServer:
 - Implement the skeleton of BluetoothGattServer
 - Make BluetoothAdapter.gattServer accessible
Blocks: 933358
Blocks: 1181479
Blocks: 1181480
Assignee: nobody → brsun
Attachment #8631525 - Flags: review?(btian)
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
Attachment #8631525 - Flags: review?(btian)
> @@ +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.
Address comment 2.
Attachment #8631525 - Attachment is obsolete: true
Attachment #8635165 - Flags: review?(btian)
> I will issue another bug for other webidl files.

Bug 1184868 is the follow-up bug.
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).
Attachment #8635165 - Flags: review?(btian) → review+
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
Attachment #8635165 - Attachment is obsolete: true
Attachment #8635914 - Flags: review?(mrbkap)
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?
Attachment #8635914 - Flags: review?(mrbkap)
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?
Flags: needinfo?(mrbkap)
Comment on attachment 8635914 [details] [diff] [review]
bug1181478_bluetoothgattserver_object.v3.patch

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

Yep. That answers my questions.
Attachment #8635914 - Flags: review+
Flags: needinfo?(mrbkap)
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;
Flags: needinfo?(btian)
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.
Attachment #8635914 - Attachment is obsolete: true
Flags: needinfo?(btian)
Attachment #8639123 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/beed8e3e86d9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: FxOS-S4 (07Aug) → ---
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
Hi Blake,

This patch contains corresponding changes for mochitest regarding to BluetoothGattServer. Would you please help to review it?
Attachment #8639702 - Flags: review?(mrbkap)
Attachment #8639702 - Flags: review?(mrbkap) → review+
Build status, marionette tests, mochitests seem good on Treeherder:
 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3bf4721298b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba34dfa90df7
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in before you can comment on or make changes to this bug.