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.

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

Comment on attachment 8459496 [details] 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.

Add unit test for module bluetooth.

Comment on attachment 8459496 [details] 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 !

Comment on attachment 8459496 [details] 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.:)

Comment on attachment 8459496 [details] pull request for 21987 My comments has been fixed by Ian already, so r+ and leave the others to Atthur. THanks :)

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] 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

Comment on attachment 8459496 [details] pull request for 21987 Thanks for the patch! Overall it looks good to me. Please check github for a few comments.

Comment on attachment 8459496 [details] 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.

Comment on attachment 8459496 [details] pull request for 21987 r=me with the l10n id issue fixed. Thanks! Before merging let's focus on getting the tests fixed.

Comment on attachment 8459496 [details] pull request for 21987 r+ with the addition of `self.marionette` as I noted in the pull request

Comment on attachment 8459496 [details] pull request for 21987 test_settings_bluetooth.py passed.

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.

Hey in gaia-ui-tests we don't mock navigator.mozBluetooth. We use the real one - this test only runs on device.

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.

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] 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.

Comment on attachment 8459496 [details] 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!

Comment on attachment 8459496 [details] 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:)

Comment on attachment 8459496 [details] pull request for 21987 r=me with the nits addressed, thank you!!

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

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

File bug 1061437 for tracking the revert work.