Closed Bug 1089511 Opened 5 years ago Closed 5 years ago

refactor system/js/bluetooth to future compatible format

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S9 (21Nov)

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [p=5])

Attachments

(2 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
iliu
: review+
alive
: feedback+
Details | Review
PR2
46 bytes, text/x-github-pull-request
alive
: review+
Details | Review
We need solid base first to enable BT API v2 support. need unit test as well
Attached file pull request redirect to github (obsolete) —
WIP
The tree herder shows ScreenManager/SoundManager can't find newable bluetooth module since the bluetooth module init in bootstrap is too late

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=5b4a125c432b

It looks like we need refactor ScreenManager/SoundManager before refactor system/bluetooth module, but I'd like focus on bluetooth related task first.

So the approach would be
 
1. refacot system/bluetooth module in origin format, add test case in this patch
2. refactor system/bluetoothTransfer to newable format in another patch

make sense?
Flags: needinfo?(iliu)
Flags: needinfo?(alive)
(In reply to Fred Lin [:gasolin] from comment #2)
> The tree herder shows ScreenManager/SoundManager can't find newable
> bluetooth module since the bluetooth module init in bootstrap is too late
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=5b4a125c432b
> 
> It looks like we need refactor ScreenManager/SoundManager before refactor
> system/bluetooth module, but I'd like focus on bluetooth related task first.
> 
> So the approach would be
>  
> 1. refacot system/bluetooth module in origin format, add test case in this
> patch
> 2. refactor system/bluetoothTransfer to newable format in another patch
> 
> make sense?

I don't really understand the problem.
The error you posted is "Unit test" - it is because of you didn't fix sound_manager_test and screen_manager_test. It's not a real error.

If you are talking about "on real device" screen manager/ sound manager will access bluetooth "before" window.bluetooth is instantiated, please point out where does the access come from. In worst case we could load bluetooth before screen manager/sound manager.
Flags: needinfo?(alive)
I think unit test cannot ensure functionality for Bluetooth. So that the refactor would make some regression to Bluetooth v1. We have to know the cost before we do it. I'm okay for the change to prepare a good code base. It would be easier to do something for Bluetooth v2. 

My original plan is aim to construct and load new script for Bluetooth v2 only. It will need a version detection before load Bluetooth script.

EX:
Gecko::Bluetooth v1 ==> load bluetooth.js, bluetooth_transfer.js
Gecko::Bluetooth v2 ==> load bluetooth_v2.js, bluetooth_transfer_v2.js

So that we won't need to spend cost for v1. And it won't have any regression in v1.:)
Flags: needinfo?(iliu)
(In reply to Ian Liu [:ianliu] from comment #4)
> I think unit test cannot ensure functionality for Bluetooth. So that the
> refactor would make some regression to Bluetooth v1. We have to know the
> cost before we do it. I'm okay for the change to prepare a good code base.
> It would be easier to do something for Bluetooth v2. 
> 
> My original plan is aim to construct and load new script for Bluetooth v2
> only. It will need a version detection before load Bluetooth script.
> 
> EX:
> Gecko::Bluetooth v1 ==> load bluetooth.js, bluetooth_transfer.js
> Gecko::Bluetooth v2 ==> load bluetooth_v2.js, bluetooth_transfer_v2.js
> 
> So that we won't need to spend cost for v1. And it won't have any regression
> in v1.:)

That plan sounds good.
make sense, I'd fire bug 1090761 to only boot to proper bluetooth version
Status: ASSIGNED → NEW
Note for long term the system bluetooth.getAdapter API might need change to promise style to ensure BT adapter in APIv2 is get. 

And we need redirect nfc_handover_manager to reuse system/bluetooth module for reuse
after some experiment and talk with Ian, Arthur, and Alive, the current plan is:

1. wrap system's heavy BT related modules to Bluetooth_core, use feature detection to distinguish v1/v2 API modules  (landed)

2. further decouple mozBluetooth with system by
  * action: implement window.Bluetooth.enable/.disable to wrap settings.set('bluetooth.enable')
  * action: implement window.Bluetooth.isEnabled to abstract the difference of v1/v2 API change
  * action: grep system to replace anywhere use bluetooth.enabled/settings.set('bluetooth.enable') API to above methods.

3. leave heavy BT related modules with v1 API as it was. For v2 API, we'll write brand new modules instead.
  * action: open related bugs

this bug will solve '2'.
Status: NEW → ASSIGNED
Summary: refactor system/js/bluetooth to newable format → refactor system/js/bluetooth to future compatible format
Blocks: 1093084
implement Bluetooth.enable/disable/isEnabled to decouplpe system bluetooth usage
Attachment #8512466 - Attachment is obsolete: true
Attachment #8516495 - Flags: review?(iliu)
Attachment #8516495 - Flags: review?(alive)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

Don't touch "Bluetooth" in other module directly. Dispatch event please.
Attachment #8516495 - Flags: review?(alive)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

Per comment 10, dispatch Bluetooth enable/disable request event is better to decouple module dependancy. Beside of decoupling module, the patch is okay for me.
Attachment #8516495 - Flags: review?(iliu)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

I've decoupled enable/disable to CustomEvent. If it looks fine I'll start implement BTv2 module with this pattern.

I'd send review after bug 1062819 landed and change modules that use Bluetooth.isEnabled to System.query('Bluetooth.isEnabled').
Attachment #8516495 - Flags: feedback?(alive)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

f+ with event naming
Attachment #8516495 - Flags: feedback?(alive) → feedback+
event naming updated, thanks!
Depends on: 1062819
Comment on attachment 8516495 [details] [review]
pull request redirect to github

@alive,

The new patch is based on bug 1062819 's change and

* update Bluetooth.isEnabled to use System.call, 
* wrap bluetooth enabled/suspended settings call in airplane_mode_service_helper.js

Tree Herder passed. 
Works fine with BTv1/v2 test device.

@Ian, 'Works fine' means:

- uitest/HW/bluetooth can get address in v1
- BT transfer works in v1 (not ported to v2 yet)
- BTv2 not crash when BT is enabled

Note BTv2 test is limited now since current BTv2 API is not testable for general enable/disable operation. We can fix followup issues in Bluetooth_v2 implementation while the BT test base is more stable.
Attachment #8516495 - Flags: review?(iliu)
Attachment #8516495 - Flags: review?(alive)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

r+ with nits
Attachment #8516495 - Flags: review?(alive) → review+
Comment on attachment 8516495 [details] [review]
pull request redirect to github

Fred, I do some manual test. But the patch is not working for me. 

First, I enable bluetooth. Then I turn on airplane mode. Looks like Bluetooth is still enabled after aiplane mode is enabled. BTW, I cannot disable Bluetooth via quick settings after applied the patch.
Attachment #8516495 - Flags: review?(iliu)
Thanks @Ian to point out. 

After more tests, I found after bug 1062819 the window.dispatchEvent from quickSettings.js can't be received in bluetooth.js. But the 

@alive, sorry I'm not familiar with the new structure yet. do I need some secret sauce to make it work?
Flags: needinfo?(alive)
(In reply to Fred Lin [:gasolin] from comment #18)
> Thanks @Ian to point out. 
> 
> After more tests, I found after bug 1062819 the window.dispatchEvent from
> quickSettings.js can't be received in bluetooth.js. But the 
> 
> @alive, sorry I'm not familiar with the new structure yet. do I need some
> secret sauce to make it work?

Comment 17 works for me in master. What do you mean by 'cannot be recieved?'
Flags: needinfo?(alive)
(In reply to Fred Lin [:gasolin] from comment #18)
> Thanks @Ian to point out. 
> 
> After more tests, I found after bug 1062819 the window.dispatchEvent from
> quickSettings.js can't be received in bluetooth.js. But the 
> 
> @alive, sorry I'm not familiar with the new structure yet. do I need some
> secret sauce to make it work?

https://github.com/mozilla-b2g/gaia/pull/25791/files#diff-41bf5f6e8c7acc31c2aed795e30c4b2eR141
https://github.com/mozilla-b2g/gaia/pull/25791/files#diff-106a370345bcef9455b0705ed6d48df9R93
Comment on attachment 8516495 [details] [review]
pull request redirect to github

sorry @alive it's a false alarm. After double check I've used the wrong register code in bluetooth.js. Now quick settings works well.

@Ian please kindly review it again, thanks!
Attachment #8516495 - Flags: review+ → review?(iliu)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

Fred, I see the status bar is not working fine in app mode after turned on/off Bluetooth. But the status bar is working fine in home screen mode. Could you please help to re-check it again? Thanks.
Attachment #8516495 - Flags: review?(iliu)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

As per offline discussion, this issue is focus on compatibility work. So it should works well on v1 API and left some followup work for v2 API: 

1. general bt support include quick settings
2. bt transfer
3. nfc+bt transfer

so please kindly review it again
Attachment #8516495 - Flags: review?(iliu)
Comment on attachment 8516495 [details] [review]
pull request redirect to github

After pulled this patch with your update, it is working for me. r+ Thanks for your effort.
Attachment #8516495 - Flags: review?(iliu) → review+
Thanks for carefully review and validation. 

tree herder green, merged https://github.com/mozilla-b2g/gaia/commit/60d2ad46dcd3c608b1ebbc885f17216c507a204c

Let's move ahead to brave world (bug 1093084 :p).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S9 (21Nov)
reverted https://github.com/mozilla-b2g/gaia/commit/4c32c19e22be3cfc44ce1a39ee5ec03c651a8a08
due to bug 1102885
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The root cause is I miss intercept power_save states. Will add related test cases for review
Attached file PR2
add PR2 and wait test result
Comment on attachment 8528999 [details] [review]
PR2

the change add check for bluetooth settings stat saved in powerSaveResume.

https://github.com/gasolin/gaia/commit/0540f398f7b876261953340e02c78ab605b2e9a0
Attachment #8528999 - Flags: review?(alive)
Attachment #8528999 - Flags: review?(alive) → review+
nit fixed, commit squashed, tree herder green. merged to master https://github.com/mozilla-b2g/gaia/commit/a11278e7d4727f6f90a0e9ed800d5120810d5c3e

thanks!
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.