Closed
Bug 1090799
Opened 10 years ago
Closed 9 years ago
make nfc_handover v2 and reuse system bluetooth adapter
Categories
(Firefox OS Graveyard :: Gaia::Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gasolin, Assigned: gasolin)
References
Details
Attachments
(4 files)
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
iliu
:
review+
tauzen
:
review+
tauzen
:
feedback+
arno
:
feedback+
iliu
:
feedback+
|
Details | Review |
Since we are transition to bluetooth v2 API, we'd better make all API call through system/js/bluetooth so it could be replaced easier.
Blocks: NFC-Gaia
Assignee | ||
Comment 1•10 years ago
|
||
Hi Ian, the goal of this patch is to make nfc_handover reuse system bluetooth adapter. Since in v2 API the bluetooth.enabled is removed, and BT does not depends on settings('bluetooth.enabled') key, we'd better try to decouple them in system modules. To achieve that, I use CustomEvent 'bluetooth-adapter-added' to denote enable state, 'bluetooth-disabled' to denote disable state to replace mozBluetooth.enable stat, then wrap settings.set to system/bluetooth's enable/disable method, so the other system module could wrap settings.set call via system/bluetooth module in the future patches.
Attachment #8514048 -
Flags: review?(iliu)
Comment 2•10 years ago
|
||
Comment on attachment 8514048 [details] [review] pull request redirect to github Looks good for me with some comment addressed on GitHub. Before we land the patch, I think both of us have to do some manual test here.:)
Updated•10 years ago
|
Attachment #8514048 -
Flags: review?(iliu)
Comment 3•10 years ago
|
||
Also cc Arno for the Bluetooth refactor work.
Assignee | ||
Comment 4•10 years ago
|
||
rename to implement BTv2 version in this bug. The previous patch is obsoleted. according to the plan described in bug 1089511 Comment 8
Status: ASSIGNED → NEW
Summary: make nfc_handover reuse system bluetooth adapter → make nfc_handover v2 and reuse system bluetooth adapter
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
found nfc_handover_manager test use the wrong Bluetooth mock. mockBluetooth is used for mock bluetooth.js, but not mozbluetooth
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master The goal is make nfc handover manager reuse existing Bluetooth adapter via service.query. Currently test on BTv1 device and works with BT default on/off. (BTv2 has issue with nfc from gecko side) We met a problem that BTv1 trigger bluetooth enabled event before bluetooth adapter-added event, therefore there will be a time gap between the BT enabled state and the timing that actually can get the BT adapter. I try to wrap the difference via promise interface. If the current implementation looks fine, I'll keep adding the rest of unit tests before review.
Attachment #8582925 -
Flags: feedback?(iliu)
Attachment #8582925 -
Flags: feedback?(alive)
Comment 10•9 years ago
|
||
In part of BTv2, I leave some comment on GitHub. Since the return type of API pair(), getPairedDevices() is changed in BTv2, the patch won't be worked as before. Please check the feasibility with Gecko Bluetooth devs. * If these API is not compatible for DOM request, please use version detector for separating them. * If there are many version detector in 'nfc_handover_manager.js', we might to considerate have another script to support BTv2. In part of BTv1, there is a setTimeout in _retrieveAdapter(). I don't think that is a good way to reach adapter. I considerate to have promise interface in Service.query module. But it will need Alive's suggestion here.
Comment 11•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master As above comment.
Attachment #8582925 -
Flags: feedback?(iliu)
Assignee | ||
Comment 12•9 years ago
|
||
Thanks for feedback, I've wrap getPairedDevices() to promise interface to abstract API difference. So v2 supposedly work now (as commet 9, BTv2 has issue with nfc from gecko side, so related function is not testable) Currently there are 3 places (_retrieveAdapter() for v1, _doPairing(), _getPairedDevices() for v1/v2) need version detection. Consider to 800+ lines of code, I feel it still worth to keep in the same script.
Comment 13•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Need to use async service request to avoid race condition
Attachment #8582925 -
Flags: feedback?(alive)
Comment 14•9 years ago
|
||
Let's not forget to ask :tauzen to feedback in next review turn.
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I add the waitForAdapter method in Bluetooth.js V1, which use object.watch to check if the defaultAdapter is available. So we don't have to count on setTimeout and avoid racing issue. test fine on v1 device.
Attachment #8582925 -
Flags: feedback?(kmioduszewski)
Attachment #8582925 -
Flags: feedback?(iliu)
Attachment #8582925 -
Flags: feedback?(alive)
Comment 16•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master This looks good to me. Great that we're finally switching to arrow functions here. |this._debug| and |this._logVisibly| are actually not needed and debug from BaseModule could be used here (same changes suggested by Alive were made in NfcManager). This patch is quite big so if you don't want to deal with this here we could rise a follow up bug for that. Since Arno is the original author and was recently working on NfcHandoverManager I think it's valuable to ask for his opinion.
Attachment #8582925 -
Flags: feedback?(kmioduszewski)
Attachment #8582925 -
Flags: feedback?(arno)
Attachment #8582925 -
Flags: feedback+
Comment 17•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Fred, Per comment 13, we have to use |Service.register('getAdapter', this);| for async query. We cannot setTimeout in a promise method to decide when we resolve/reject it. In part of nfc_handover_manager, we can use |this.service.request('getAdapter', 'arg').then(function() {// TODO something.. });|. I think it could be a solution between NFC and Bluetooth modules. Be careful the interface of Service module as following: * register: function(service, server) { * registerState: function(state, provider) { (Should be given with a state, not a async service or something..) Alive, Please correct me, If I misunderstand. Thanks.
Attachment #8582925 -
Flags: feedback?(iliu)
Comment 18•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master See Ian's comment
Attachment #8582925 -
Flags: feedback?(alive)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 19•9 years ago
|
||
Since bug 1153721 is resolved. Have quick test on BTv2 devices. The transfer between v1/v2 works with this patch.
Comment 20•9 years ago
|
||
Hi Fred, Thank you for providing patches to support BT API v2 for NFC handover manager. I'd like to remind you that gaia patch of Bug 1146744 has been landed to m-c. The patch is based on BT API v1 and it uses 'BluetoothDevice.connected' to determine whether to re-connect device again. https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L643 You may have noticed the attribute 'BluetoothDevice.connected' isn't supported by BT API v2 any more. However, you can use 'BluetoothAdapter.getConnectedDevices()' to achieve similar functionality.
Assignee | ||
Comment 21•9 years ago
|
||
@jamin thanks for notice, I'll fix that in PR as well.
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master ian & alive, Current patch use service.request to get bluetooth adapter asynchronously, local test works on v1/v2 devices. The main differences are: * cache adapter when bluetooth enabled and disabled * abstract API difference between BTv1/v2 API * wrap the difference interface of adapter.getPairedDevices API * wrap the difference callback of adapter.pair API * use adapter.getConnectedDevices API instead of device.connected The patch also fix several issues in nfcHandoverManager: * fix test missusing mock_bluetooth.js instead of mock_navigator_moz_bluetooth * use BaseModule's debug, rename `_debug` to `debug` * use arrow function instead of bind The existing test cases are more like integration test and tightly coupled with the whole flow, so it needs more time to rewrite large parts to get per function test. Please kindly feedback again, thanks.
Attachment #8582925 -
Flags: feedback?(iliu)
Attachment #8582925 -
Flags: feedback?(alive)
Comment 23•9 years ago
|
||
I remember the conclusion is use Service.request('Bluetooth.getAdapter') what's wrong with it?
Assignee | ||
Comment 24•9 years ago
|
||
The patch did use Service.request('Bluetooth:adapter') to get promised adapter. In offline discussion, Ian prefer name the method as `adapter` than `getAdapter`.
Comment 25•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Good work, Fred! Regarding the v1/v2 BT API: might it make sense to have an abstraction layer that hides the differences and that could also be used in other modules (e.g., BluetoothTransfer)?
Attachment #8582925 -
Flags: feedback?(arno) → feedback+
Assignee | ||
Comment 26•9 years ago
|
||
Thanks for feedback! Though those v1/v2 API now only used in nfc_transfer_manager, I'll abstract them into bluetooth.js v1/v2 to make it testable for different API version.
Comment 27•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Feedback+ for the patch. And I leave some comment on GitHub. We can start to revise them for reviewing process. Thanks for the effort with |service.request| version. It makes the code base more decoupled than before. As Arno's suggestion in comment 25, we also have discussion and plan to move file transfer relative work to another module. And the module is able to handle file transfer via Bluetooth/Wifi(if needed) in long tern. We won't do so much Bluetooth file transfer pre-process(get adapter/pair/getPairedDevices/connect) in |nfc_handover_manager| module.
Attachment #8582925 -
Flags: feedback?(iliu) → feedback+
Comment 28•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I am going to deliver the review right to Ian here; Ian, if you have uncertain issues during review please don't hesitate to flag me or find me directly.
Attachment #8582925 -
Flags: feedback?(alive)
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Thanks for all your feedback. I've addressed the comment and make sure it work with BTv1/v2 devices. I've tested with/without BT enabled cases as well.
Attachment #8582925 -
Flags: review?(kmioduszewski)
Attachment #8582925 -
Flags: review?(iliu)
Assignee | ||
Updated•9 years ago
|
Component: Bluetooth → Gaia::Bluetooth File Transfer
Comment 30•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I leave two major comments on GitHub. 1). Please don't use Object.watch() since it might make performance issue. And it's designed for debugging only[1]. I think we have to find out the other way to do the same thing. 2). I don't know why you have to do getConnectedDevices() here. I don't see the process in the old code base. And the usage is in-corrected. The parameter is serviceUuid(profile ID). [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch
Attachment #8582925 -
Flags: review?(iliu)
Comment 31•9 years ago
|
||
(In reply to Ian Liu [:ianliu] from comment #30) > Comment on attachment 8582925 [details] [review] > [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master > > I leave two major comments on GitHub. > > 1). Please don't use Object.watch() since it might make performance issue. > And it's designed for debugging only[1]. I think we have to find out the > other way to do the same thing. Especially the patch is in System app, We cannot jump into the risk in potential. It's not worth to use Object.watch().
Comment 32•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Left small comments on github.
Attachment #8582925 -
Flags: review?(kmioduszewski)
Assignee | ||
Comment 33•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Thanks for review. The device.connected API is deprecated in BTv2 so we have to use `getConnectedDevices()` to check if device is connected. It's my fault to miss-use the API. All issue addressed and test work on v1/v2 device, please kindly review it again :)
Attachment #8582925 -
Flags: review?(kmioduszewski)
Attachment #8582925 -
Flags: review?(iliu)
Comment 34•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Fred, Thanks for reminder for API changed in Bluetooth v2. Do getConnectedDevices() is needed since we have to let NFC module to support in Bluetooth v1/v2. Looks you have revised some of my comment. Then, I leave comment for other nits. Please take a look for them on GitHub. The patch is almost there. And is there any unit test for bluetooth.js/bluetooth_v2.js? We have to cover these changed in this patch also. Thanks. Krzysztof, could you please help to review the unit test of nfc_handover_manager.js? I'm not familiar with that part. Thanks.
Attachment #8582925 -
Flags: review?(iliu)
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master @Ian really thanks for review, issue addressed and unit tests added for bluetooth.js/bluetooth_v2.js to cover these changed in this patch. Please kindly review it again.
Attachment #8582925 -
Flags: review?(iliu)
Comment 36•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I've focused on NfcHandoverManager tests as suggested by Ian. I've left some comments on github. Great job with the unit tests Fred, this was really needed.
Attachment #8582925 -
Flags: review?(kmioduszewski)
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master Thanks for checking unit test part and provide better naming for test cases. I've update test case issues, please kindly review it again.
Attachment #8582925 -
Flags: review?(kmioduszewski)
Comment 38•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I have done some manual tests successfully. The patch is working on NFC send/receive file via Bluetooth. My test version are as following. send/receive: v1/v1 v2/v2 v1/v2 v2/v1 And I see Fred has updated the patch with my finally comment. Thanks for your effort here. Good job, r+ with me.
Attachment #8582925 -
Flags: review?(iliu) → review+
Comment 39•9 years ago
|
||
Comment on attachment 8582925 [details] [review] [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master I can see that all the comments are addressed. I've also done manual testing with Nexus 5 (running your gaia branch) and regular Android Lollipop device. I can send/receive files as expected, failure prompt is also present in proper scenarios. Looks good, r+ from me. As I already, wrote this is really great work. Thanks Fred!
Attachment #8582925 -
Flags: review?(kmioduszewski) → review+
Comment 40•9 years ago
|
||
(In reply to Ian Liu [:ianliu] from comment #38) > My test version are as following. > > send/receive: > v1/v1 > v2/v2 > v1/v2 > v2/v1 Note for my test device. flame <--> flame. And also thanks for Krzysztof's reviewing effort.:)
Assignee | ||
Comment 41•9 years ago
|
||
treeherder all green, merged to master https://github.com/mozilla-b2g/gaia/commit/526e8c2ff20a222ec68f7e820a979cb24fe549d4 thanks!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•