Closed Bug 1141442 Opened 7 years ago Closed 7 years ago

[Bluetooth][System] replace isProfileConnected with service query

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

Attachments

(2 files)

Currently in system we use Bluetooth.isProfileConnected to get BT profile connection state directly. But we can't guarantee the bluetooth module is ready when it's been import with lazyload method. So we would like to check isProfileConnected state with event listener instead.
Assignee: nobody → gasolin
Blocks: 1122327
Status: NEW → ASSIGNED
Some modules already listen to `bluetoothprofileconnectionchange` event, so I'll listen this event in other system modules to replace the usage of Bluetooth.isProfileConnected function. (after the patch only bluetooth icons still use isProfileConnected function).
Comment on attachment 8575158 [details] [review]
[gaia] gasolin:issue-1141442 > mozilla-b2g:master

remove direct call of Bluetooth.isProfileConnected around system modules. Mainly effect media_playback and sound_manager so set review to Dominic as well.

@domi, though treeherder is green, is it possible to arrange a QA test to make sure all existing cases works?
Attachment #8575158 - Flags: review?(dkuo)
Attachment #8575158 - Flags: review?(alive)
Comment on attachment 8575158 [details] [review]
[gaia] gasolin:issue-1141442 > mozilla-b2g:master

I don't think everyone should maintain the state..
Please use Service.registerState in Bluetooth and Service.query in all other modules (if state is undefined -> don't do anything until getting events.)
Attachment #8575158 - Flags: review?(alive)
I've discussed with @Ian before work on this PR. We can see the BT profiles might be expanded in the future.
Currently only Bluetooth A2DP and SCO profiles are need to be detected in other system modules, so both approaches work. But it is not that extendable to provide each BT profile states via Service.query. 

And currently most Bluetooth related functions are provide via events to other system modules. How do you think?
Flags: needinfo?(iliu)
Flags: needinfo?(alive)
(In reply to Fred Lin [:gasolin] from comment #5)
> I've discussed with @Ian before work on this PR. We can see the BT profiles
> might be expanded in the future.
> Currently only Bluetooth A2DP and SCO profiles are need to be detected in
> other system modules, so both approaches work. But it is not that extendable
> to provide each BT profile states via Service.query. 

I don't see how your current solution work with the extended profiles... could you elaborate?

To me the solution candidates is something like:
1.
Provide an object to tell all possible profiles
Provide a query method to test if one of the profiles are connected

2.
Provide query for each profiles - Bluetooth.isA2dpConnected, Bluetooth.isXXXConnected

3.
Mapping profiles to a more semantic naming if possible: a2dp to music, maybe.

> 
> And currently most Bluetooth related functions are provide via events to
> other system modules. How do you think?
Flags: needinfo?(alive)
Once Bluetooth profiles are updated, the change event is emitted and bring with detail.name/connected state.

Other module listen to the change event can filter target profile by detail.name ('sco' or 'a2dp' for current cases), So if we have more profiles, other module could listen to the same event and just filter with new profile name.
Comment on attachment 8575158 [details] [review]
[gaia] gasolin:issue-1141442 > mozilla-b2g:master

Offline discuss with @Ian, from other modules view, calling service.query is more simpler than handle event listener.
So I'll provide a patch with service.query instead.
Attachment #8575158 - Flags: review?(dkuo)
Flags: needinfo?(iliu)
Yes, looks like sound_manager and media_playback will access the status of bluetooth connected profile. As Alive's mention(comment 4), these modules will need some effort to maintain profiles status. In the view point of these modules, they are not always care about the profiles status changed. And it's duplicated of what Bluetooth module have maintained. I think that is not expected. So I agree with using Service.query.
Summary: [Bluetooth][System] replace isProfileConnected with event listener → [Bluetooth][System] replace isProfileConnected with service query
Comment on attachment 8575862 [details] [review]
[gaia] gasolin:issue-1141442-2 > mozilla-b2g:master

* add BluetoothProfiles to provide readonly bluetooth profile list for other modules
* make `isProfileConnected` a private function and expose OPP/SCO/A2DP profile connected state via Service query
Attachment #8575862 - Flags: review?(iliu)
Attachment #8575862 - Flags: review?(alive)
Comment on attachment 8575862 [details] [review]
[gaia] gasolin:issue-1141442-2 > mozilla-b2g:master

Good work for me. Looks the patch decreases dependancy than before. Please make sure all functionality is working fine before land the patch. Thanks:)
Attachment #8575862 - Flags: review?(iliu) → review+
Comment on attachment 8575862 [details] [review]
[gaia] gasolin:issue-1141442-2 > mozilla-b2g:master

Sorry, I don't think we need the BluetoothProfiles Object...
Do you create it just because bluetooth1/2 is lazily loaded?

Create an object which only changes 'OPP' to 'opp' seems insane to me.

I would suggest no more mapping - just do string.lowercase() when isXXXXProfileConnected() is queried.
Attachment #8575862 - Flags: review?(alive)
Yes BluetoothProfiles is created just for consistency. I'll just compare string instead if it's the preferred way.
Comment on attachment 8575862 [details] [review]
[gaia] gasolin:issue-1141442-2 > mozilla-b2g:master

removed BluetoothProfiles object and use profile name directly. Please kindly review it again.
Attachment #8575862 - Flags: review?(alive)
Comment on attachment 8575862 [details] [review]
[gaia] gasolin:issue-1141442-2 > mozilla-b2g:master

r=me
Attachment #8575862 - Flags: review?(alive) → review+
treeherder green, merged to master https://github.com/mozilla-b2g/gaia/commit/6dd297d25b3fdca212d405b959f8d9905f330ad4

thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1143563
The error is found in latest master.

TypeError: this.manager.isOPPProfileConnected is not a function
Stack trace:
BluetoothTransferIcon.prototype.shouldDisplay@app://system.gaiamobile.org/js/bluetooth_transfer_icon.js:11:12
BaseIcon.prototype.update@app://system.gaiamobile.org/js/base_icon.js:185:5
BaseIcon.prototype.render/<@app://system.gaiamobile.org/js/base_icon.js:122:1
Flags: needinfo?(gasolin)
Bug 1143563 fixed the regression yesterday, please rebase on latest master
Flags: needinfo?(gasolin)
You need to log in before you can comment on or make changes to this bug.