EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive) Reporter
	Description • 10 years ago

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.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 2 • 10 years ago

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

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 4 • 10 years ago

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.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 5 • 10 years ago

Add unit test for module bluetooth.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive) Reporter
	Comment 6 • 10 years ago

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 !

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 7 • 10 years ago

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.:)

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive) Reporter
	Comment 8 • 10 years ago

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 :)

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 9 • 10 years ago

Thanks for EJ's reviewing effort. I also update the patch with Arthur's suggestion. And add unit test to cover bluetooth_helper.js.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 10 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 11 • 10 years ago

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.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 12 • 10 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 13 • 10 years ago

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.

	Zac C (:zac)
	Comment 14 • 10 years ago

Comment on attachment 8459496 [details]
pull request for 21987

r+ with the addition of `self.marionette` as I noted in the pull request

	Askeing Yen[:askeing][:fyen]
	Comment 15 • 10 years ago

Comment on attachment 8459496 [details]
pull request for 21987

test_settings_bluetooth.py passed.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 16 • 10 years ago

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.

	Zac C (:zac)
	Comment 17 • 10 years ago

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

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 18 • 10 years ago

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.

	Zac C (:zac)
	Comment 19 • 10 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?

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 20 • 10 years ago

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.

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 21 • 10 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 22 • 10 years ago

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!

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 23 • 10 years ago

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:)

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 24 • 10 years ago

Comment on attachment 8459496 [details]
pull request for 21987

r=me with the nits addressed, thank you!!

	iliu@mozilla.com, ianliu.moz@gmail.com Assignee
	Comment 25 • 10 years ago

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

	Eric Chou [:ericchou] [:echou]
	Comment 26 • 10 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 27 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 28 • 10 years ago

File bug 1061437 for tracking the revert work.