Closed
Bug 1141442
Opened 9 years ago
Closed 9 years ago
[Bluetooth][System] replace isProfileConnected with service query
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
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 | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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).
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
(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)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(iliu)
Comment 9•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Summary: [Bluetooth][System] replace isProfileConnected with event listener → [Bluetooth][System] replace isProfileConnected with service query
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Blocks: Gaia-BT-v2-API
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
Yes BluetoothProfiles is created just for consistency. I'll just compare string instead if it's the preferred way.
Assignee | ||
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
Comment on attachment 8575862 [details] [review] [gaia] gasolin:issue-1141442-2 > mozilla-b2g:master r=me
Attachment #8575862 -
Flags: review?(alive) → review+
Assignee | ||
Comment 17•9 years ago
|
||
treeherder green, merged to master https://github.com/mozilla-b2g/gaia/commit/6dd297d25b3fdca212d405b959f8d9905f330ad4 thanks!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 18•9 years ago
|
||
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)
Assignee | ||
Comment 19•9 years ago
|
||
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.
Description
•