If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Settings] Remove BT panel and embed it from BT app

RESOLVED FIXED in 2.1 S1 (1aug)

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: eragonj, Assigned: iliu@mozilla.com, ianliu.moz@gmail.com)

Tracking

unspecified
2.1 S1 (1aug)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=5])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
eragonj
: review+
arthurcc
: review+
askeing
: review+
zac
: review+
arthurcc
: review+
Details | Review | Splinter Review
We don't want to maintain two similar codes, so after settings bundle feature is implemented, we can embed BT app directly into Settings app.
Blocks: 1032069
No longer blocks: 1032069
Depends on: 1032069
Depends on: 1020063
Depends on: 1033951
No longer depends on: 1020063
(Assignee)

Updated

3 years ago
Depends on: 1037306
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1037306
The target of the bug:

* Remove settings/js/bluetooth.js code base
* Enable Bluetooth app to be embedded iframe in Settings::Bluetooth
* Move the implementation of Bluetooth menu status to root panel
Assignee: nobody → iliu
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Created attachment 8459496 [details] [review]
pull request for 21987
(Assignee)

Updated

3 years ago
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S1 (1aug)
Comment on attachment 8459496 [details] [review]
pull request for 21987

Arthur, I would like to have some feedback from your pre-review for the WIP. 

The major change:

* Remove bluetooth.js, connectivity.js and some useless code about settings::bluetooth panel
* Implement modules/bluetooth.js
* Implement panels/root/bluetooth_item.js
  ** Implement bluetooth menu description
  ** Implement bluetooth address for about more info.

I'm still working unit test. And will revise it if you have some suggestion. Thanks.
Attachment #8459496 - Flags: feedback?(arthur.chen)
Add unit test for module bluetooth.
Comment on attachment 8459496 [details] [review]
pull request for 21987

Ian, I just helped Arthur to give the first round feedback.

Please check my comments on Github and don't hesitate to set r? f? again to us when you update it !

Thanks a lot !
Attachment #8459496 - Flags: feedback?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

Thanks for EJ's feedback soon. I have revised the patch with your suggestion. And add unit test for modules/bluetooth.js, panels/root/bluetooth_item.js. Set the review flag again. Looks Arthur has many reviews in queue. So I set two reviewers for this patch. Feel free to clear the other one, if the patch is reviewed.:)
Attachment #8459496 - Flags: review?(ejchen)
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

My comments has been fixed by Ian already, so r+ and leave the others to Atthur.

THanks :)
Attachment #8459496 - Flags: review?(ejchen) → review+
Thanks for EJ's reviewing effort. I also update the patch with Arthur's suggestion. And add unit test to cover bluetooth_helper.js.
Comment on attachment 8459496 [details] [review]
pull request for 21987

Update: 

* Remove escapeHTML from panels/frame/panel.js
* Revise id "menuItem-bluetooth" to be a CSS class for selector
* Modify the "gaia-ui-test" for CSS selector
Attachment #8459496 - Attachment description: A WIP for the goal → pull request for 21987
Comment on attachment 8459496 [details] [review]
pull request for 21987

Thanks for the patch! Overall it looks good to me. Please check github for a few comments.
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

Arthur, thanks for your suggestion. Already revised the patch with your suggestion. Will need your review again.

Hi Askeing and Zac, could you please help to review some python changed in Gaia-ui-test. Thanks.
Attachment #8459496 - Flags: review?(zcampbell)
Attachment #8459496 - Flags: review?(fyen)
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

r=me with the l10n id issue fixed. Thanks! Before merging let's focus on getting the tests fixed.
Attachment #8459496 - Flags: review?(arthur.chen) → review+

Comment 14

3 years ago
Comment on attachment 8459496 [details] [review]
pull request for 21987

r+ with the addition of `self.marionette` as I noted in the pull request
Attachment #8459496 - Flags: review?(zcampbell) → review+
Comment on attachment 8459496 [details] [review]
pull request for 21987

test_settings_bluetooth.py passed.
Attachment #8459496 - Flags: review?(fyen) → review+
Hi Zac,

I have a question about mock navigator.mozBluetooth in Gaia-ui-test. After the patch landed, we will require navigator.mozBluetooth while the settings app is launching in the root panel. Could we mock it? We have done the mock in Gaia-integration-javascript. (https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-d747f917a55b5c99bbb2b9f90b23c2d8R25). Will need your information for the implementation. Thanks.
Flags: needinfo?(zcampbell)

Comment 17

3 years ago
Hey in gaia-ui-tests we don't mock navigator.mozBluetooth. We use the real one - this test only runs on device.
Flags: needinfo?(zcampbell)
But looks like many gaia-ui-test script failed in B2G Desktop(https://tbpl.mozilla.org/?rev=caff107f9701125d4c8f604c7888975c2647a020&tree=Gaia-Try). We have to find a way to mock the bluetooth module on desktop. Because there is no bluetooth on b2g desktop version.

Comment 19

3 years ago
Oh I see, I didn't understand that the tests were failing.

I don't know how to mock it. We always prefer to use real APIs. Can you mock it in desktopb2g instead of in the test framework?
I don't know how to mock it in desktopb2g. So I do some check before return 'bluetooth' module in settings app. https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-15

After the change, looks like Gaia-ui-test is passed normally. But it might be not good solution.
Comment on attachment 8459496 [details] [review]
pull request for 21987

Arthur, I do some change for the test.

* Gaia-ui-test:

Return mockMozBluetooth while navigator.mozBluetooth is null in all platform. Since we don't have a way to mock it on b2gdesktop, I think we could do it for leaving the blocking.

https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-15

* Gaia-JS-Integration-test:

** Fixed Gij Bluetooth test case failed.
** Add a new test case for embedded Bluetooth app in Settings::Bluetooth panel.

https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-33
https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-34
https://github.com/mozilla-b2g/gaia/pull/21987/files#diff-38

I think these changes needed some review from you. Thanks.
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

Per the offline discussion, please remove the injection of the bt mock from the tests of panels that do not use bt. I also left some comments regarding the timing of checking the availability of bt. We are almost there!
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

Arthur, I have revised the patch with your suggestion. Please help to review it again. Thanks for your reviewing effort with such patient:)
Attachment #8459496 - Flags: review?(arthur.chen)
Comment on attachment 8459496 [details] [review]
pull request for 21987

r=me with the nits addressed, thank you!!
Attachment #8459496 - Flags: review?(arthur.chen) → review+
Thanks for all of your support in this patch. I have removed the nits. Since all of the tests are passed, the patch is landed. We can close the issue now.

Gaia/master: 8ae2cd8e9310a6b55f18e091fe0a512ec307ce17
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
See Also: → bug 1056708
This fix causes bug 1053107, which makes Settings app crash every time after trying to get into Bluetooth page. I have already had progress on investigating the root cause (see bug 1053107). Any help from Gaia side would be very welcome.
This is a candidate PR[1] for reverting this patch and other related patches. As gecko does not support OOP apps embedding other apps yet, if there is no feasible solution from the gecko side (see bug 1059662) for now, we should consider to apply the PR.

[1]: https://github.com/mozilla-b2g/gaia/pull/23563
Blocks: 1061437
File bug 1061437 for tracking the revert work.
You need to log in before you can comment on or make changes to this bug.