Closed Bug 1238825 Opened 4 years ago Closed 4 years ago

[web-bluetooth] Add a preference for W3C WebBluetooth API

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Tracking Status
firefox46 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

Attachments

(1 file, 5 obsolete files)

During developing W3C WebBluetooth API, we should have a way to conditionally expose this API set in run-time for development and testing purpose.
This preference will be default off, application developers could continue to use our previous BLE api normally.
What I have in mind right now is adding a 'dom.bluetooth.gattclient.webbluetooth.enabled' preference, default value is false during our development.
In the WebIDL of all W3C interfaces, add [Func="WebBluetoothEnabled"] to conditionally expose W3C interfaces, which has GATT client functionality.
In the WebIDL of B2G only GATT client API, add [Func="WebBluetoothDisabled"] to conditionally expose them.

GATT server API and classic bluetooth API could always be accessed by applications with 'bluetooth' permission regardless of the value of that preference.
- add a preference for webbluetooth API
- expose webbluetooth API if the value of the preference is true
- expose b2g only API if webbluetooth API is not enabled.
- add Func test for all B2G gatt client API interfaces
Attachment #8710303 - Attachment is obsolete: true
Hi Ben,

The goal of this bug is for hiding webbluetooth API during development.
I added a "dom.bluetooth.webbluetooth.enabled" pref for this.
true: webbluetooth API is exposed, B2G only BLE client API is not exposed
false (default): webbluetooth API is not exposed, B2G only BLE client API is exposed.
classic API and BLE server API are always exposed.
Could you help to review my patch?

Thanks,
Jocelyn
Attachment #8710315 - Attachment is obsolete: true
Attachment #8710328 - Flags: review?(btian)
Comment on attachment 8710328 [details] [diff] [review]
Bug 1238825: Add "dom.bluetooth.webbluetooth.enabled" preference for WebBluetooth API development.

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

r=me with comment addressed.

::: b2g/app/b2g.js
@@ +1123,5 @@
>  // The app origin of bluetooth app, which is responsible for listening pairing
>  // requests.
>  pref("dom.bluetooth.app-origin", "app://bluetooth.gaiamobile.org");
>  
> +// Enable W3C webbluetooth GATT client API and disabled the B2G only one.

Enable W3C 'W'eb'B'luetooth API and disable B2G only GATT client API.

@@ +1124,5 @@
>  // requests.
>  pref("dom.bluetooth.app-origin", "app://bluetooth.gaiamobile.org");
>  
> +// Enable W3C webbluetooth GATT client API and disabled the B2G only one.
> +pref("dom.bluetooth.webbluetooth.enabled", true);

false by default.

::: dom/bluetooth/common/webapi/BluetoothManager.h
@@ +77,5 @@
>     */
>    void AppendAdapter(const BluetoothValue& aValue);
>  
> +  /**
> +   * Check if W3C WebBluetooth is disabled or not.

Check whether B2G only GATT client API is enabled (true) or W3C WebBluetooth API is enabled (false).

@@ +79,5 @@
>  
> +  /**
> +   * Check if W3C WebBluetooth is disabled or not.
> +   */
> +  static bool IsWebBluetoothDisabled(JSContext* cx, JSObject* aGlobal);

Rename to |B2GGattClientEnabled|.

::: dom/webidl/BluetoothAdapter.webidl
@@ +128,5 @@
>  
>    sequence<BluetoothDevice> getPairedDevices();
>  
> +  /**
> +   * |startLeScan| and |stopLeScan| methods are B2G only GATT client API.

I prefer to simplify as below:

  /**
   * [B2G only GATT client API]
   * |startLeScan| and |stopLeScan| methods are exposed only if
   * "dom.bluetooth.webbluetooth.enabled" preference is false.
   */

and for all the following.
Attachment #8710328 - Flags: review?(btian) → review+
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #1)
> What I have in mind right now is adding a
> 'dom.bluetooth.gattclient.webbluetooth.enabled' preference, default value is
> false during our development.
> In the WebIDL of all W3C interfaces, add [Func="WebBluetoothEnabled"] to
> conditionally expose W3C interfaces, which has GATT client functionality.
> In the WebIDL of B2G only GATT client API, add [Func="WebBluetoothDisabled"]
> to conditionally expose them.
> 
A note here, for W3C interfaces, we will use [Pref="dom.bluetooth.webbluetooth.enabled"] directly.
Thanks a lot for your time and valuable feedback, Ben!

- address review comments
Attachment #8710328 - Attachment is obsolete: true
Comment on attachment 8710828 [details] [diff] [review]
Bug 1238825: Add "dom.bluetooth.webbluetooth.enabled" preference for WebBluetooth API development. r=btian

Hi Boris,

We are starting to work on migrating our GATT(BLE) client API on B2G to W3C webbluetooth[1] one (only GATT client is covered in this spec).
During the development, we plan to pref off WebBluetooth API by default and expose our own API for applications to be able to act as a GATT client as usual.
(We will also rename all current bluetooth interfaces to MozBluetooth* and add new interfaces using the same name as specified in the WebBluetooth spec, which is tracked by Bug 1238479.)

My approach here is adding a pref for WebBluetooth API and conditionally exposing WebBluetooth API or B2G only GATT client API based on that.
Could you help to review my patch here?

[1] https://webbluetoothcg.github.io/web-bluetooth/

Thanks,
Jocelyn
Attachment #8710828 - Flags: review?(bzbarsky)
Comment on attachment 8710828 [details] [diff] [review]
Bug 1238825: Add "dom.bluetooth.webbluetooth.enabled" preference for WebBluetooth API development. r=btian

Sorry for the disturbance, I need to revise few places first, will ask for review after that.
Attachment #8710828 - Flags: review?(bzbarsky)
Fwiw, I read the patch; if you don't plan to have C++ callers of the function, just use Pref annotations.
(In reply to Boris Zbarsky [:bz] from comment #10)
> Fwiw, I read the patch; if you don't plan to have C++ callers of the
> function, just use Pref annotations.

Thanks for your prompt response.
I thought Pref annotation doesn't support conditionally exposing an interface if the pref value is false?
Or you mean we should add another pref for B2G GATT client API?
- fix a typo in b2g.js
- use Func instead of Pref in BluetoothGattCharacteristicEvent.webidl
Attachment #8710828 - Attachment is obsolete: true
> I thought Pref annotation doesn't support conditionally
> exposing an interface if the pref value is false?

Oh, I see, you wnat to reverse the sense of the pref.  Then yeah, you need the Func, good point.
Comment on attachment 8710831 [details] [diff] [review]
Bug 1238825: Add "dom.bluetooth.webbluetooth.enabled" preference for WebBluetooth API development. r=btian

Please see my detailed explanation in Comment 8, thanks!
Attachment #8710831 - Flags: review?(bzbarsky)
Comment on attachment 8710831 [details] [diff] [review]
Bug 1238825: Add "dom.bluetooth.webbluetooth.enabled" preference for WebBluetooth API development. r=btian

r=me
Attachment #8710831 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e93a14409efe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.