1070823 - [Bluetooth][Settings] bluetooth panel support BT v2 API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[[e-mail]]

We need a new settings panel in gaia to support Bluetooth v2 API. 

With new API detection, we can decouple the work of Gaia and Gecko.
Once we land this issue, gecko could switch to BTv2 API anytime.
Once gecko is fully switched, we could remove the old panel.

Bluetooth API
v1 https://developer.mozilla.org/en-US/docs/Web/API/Web_Bluetooth_API
v2 https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2

Comment 1

[[e-mail]]

API slide reference https://wiki.mozilla.org/images/2/21/WebBluetooth_API_v4.pdf

Comment 2

[[e-mail], [e-mail]]

Bug 1074075 is ready to land. I can start to work on the new code base.

Comment 3

[[e-mail], [e-mail]]

Attachment: WIP 26139

Arthur, I would need your feedback for the architecture of Bluetooth panel/module in v2.0 API. The major modification is as following. Thanks.

* Functionality ready item:
  - Enable/Disable
  - Visible
  - Device List (Remote/Paired)
  - Search (startDiscovery)

* Functionality not ready item:
  - Pair
  - Connection of paired device
  - Timeout visible, Rename with new layout
  - Icon type of device item

Comment 4

[[e-mail], [e-mail]]

Comment on attachment 8522859 [details]
WIP 26139

>https://github.com/mozilla-b2g/gaia/pull/26139

Comment 5

[[e-mail], [e-mail]]

Comment on attachment 8522859 [details]
WIP 26139

https://github.com/mozilla-b2g/gaia/pull/26139

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8522859 [details]
WIP 26139

Looks good! There are two major comments:

- We should consider the cases of adding/removing the default adapter and the states of with/without the default adapter.
- Suggest to wrap the found bluetooth devices with an Observable (we could create a separate module for that) because there should be many modules interested the attribute changes from them.

Comment 7

[[e-mail], [e-mail]]

Comment on attachment 8522859 [details]
WIP 26139

>https://github.com/mozilla-b2g/gaia/pull/26139/files

Comment 8

[[e-mail], [e-mail]]

Attachment: pull request 26139

Thanks for Arthur's feedback in side of architecture. I have updated the WIP per comment 6. Since the part of architecture is revised, I would need the second feedback here.:)

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8525188 [details] [review]
pull request 26139

The part of wrapping a bluetooth device with an Observable is good. However, there seems no much difference for the parts regarding the default adapter. Note that we should do:
- make AdapterManager reflects the change of the default adapter
- make BtContext have two modes for with/without the default adapter

Comment 10

[[e-mail], [e-mail]]

Comment on attachment 8525188 [details] [review]
pull request 26139

I forget to make an observable property 'defaultAdapter' for AdapterManager. I have updated the patch with your comment on GitHub/comment 9. Thanks for your reminder.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8525188 [details] [review]
pull request 26139

Awesome! f=me.

Comment 12

[[e-mail], [e-mail]]

Arthur, thanks for your feedback first. I will keep to implement unfinished functionality in comment 3. While these major functionality are ready, I will set the patch in reviewing process.

Before implement v2 API in Bluetooth app, we should make Bluetooth app to support AMD for module reuse.

Comment 13

[[e-mail], [e-mail]]

Comment on attachment 8525188 [details] [review]
pull request 26139

Arthur, I have added unit test for the main module 'bluetooth_context'. I think we can start to go into reviewing process. To compare with feedback patch before, the modification is as following.

* '_refreshPairedDevicesInfo' function.
* '_getBluetoothDevice' module with promise call.
* Add debug log for all modules. We can toggle on/off debugging log if we need.

I will keep to add unit test for other modules. Thanks.

Comment 14

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #13)
> Comment on attachment 8525188 [details] [review]
> pull request 26139
> 
> To compare with feedback
> patch before, the modification is as following.
> 
> * '_refreshPairedDevicesInfo' function.
> * '_getBluetoothDevice' module with promise call.
> * Add debug log for all modules. We can toggle on/off debugging log if we
> need.

* Set device name with dialog service.
* Add unit test for 'bluetooth_adapter_manager'.

Comment 15

[[e-mail], [e-mail]]

* Add unit tests for all new modules compeletly.

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8525188 [details] [review]
pull request 26139

Good job! One general comment would be returning a promise for async functions. This enables more flexibility for UI operations and makes testing easier as we can simply do the assertion in the 'then' function.

Comment 17

[[e-mail], [e-mail]]

Comment on attachment 8525188 [details] [review]
pull request 26139

Revised the patch with reviewer's suggestion. The major change is as following:
* Revise asynchronies functions to return promise.
  ** Resolve in API's resolve case only.
  ** Reject with reason.(same state, state transition, Bluetooth is disabled, no default adapter).
* Move `Debug` function to local.
* Revise unit test for covering these change.

Arthur, it will need your review in second round. Thanks.

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8525188 [details] [review]
pull request 26139

One last comment to be addressed, please check github, thanks!

Comment 19

[[e-mail], [e-mail]]

Comment on attachment 8525188 [details] [review]
pull request 26139

I have updated the patch with some comment on GitHub. And thanks for Arthur's help on Gij, Gip test failed. Looks like there is no non-intermittent test failed now. And the patch is working fine on Jamin's local build with Bluetooth API v2.

Request review again.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8525188 [details] [review]
pull request 26139

r=me, thank you!

Comment 21

[[e-mail], [e-mail]]

Thanks for Arthur's review with such patient. And very clear to guide how these useful component we already have in settings app. It makes the new code base lightly and reuse-full. Let's wait all test case pass on try-server. Then we can land the patch later.

Comment 22

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Comment 23

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Comment 24

[GitHub Autolander Bot]

There was an error creating the taskgraph, please try again. If the issue persists please contact someone in #taskcluster.

Comment 25

[[e-mail], [e-mail]]

Since the pull request are landed, we can close the issue now.

Gaia/master: b76329ca62b20a795d7af2d50a502bd19178973d