Bug 1089486 introduces nfc.status setting which reflects the status of NFC HW. nfc.status will be used to disable/enable NFC toggle in settings. Statusbar should observe nfc.status and show NFC icon when it's equal to 'enabled'. This way NFC icon in statusbar will reflect the actually state of NFC HW and will be consistent with NFC toggle in settings (and possible other NFC indicators in future).

Hello Alive, this patch modifies both NfcManager and Statusbar. Could you review it? Thanks!

Comment on attachment 8538016 [details] pull-request-1103874.txt WOW, Sorry, not knowing you are working on this :/ I am refactoring statusbar right now in bug 1098168, and the logic you are fixing here will be moved back to nfcManager :( https://github.com/mozilla-b2g/gaia/pull/26617 On the other hand, I think it is redundant to use settings db to communicate in system app. It is slow and async. We could update the icon in nfcManager directly once the HW state is changed.

No problem. I wasn't aware of the statusbar refactoring effort. All what you wrote makes perfect sense. So there is actually some clean up needed from bug 1089486 which introduced nfc.status property. I think it would make sense to do it also in your bug, but if you don't agree I can do this in this bug, once you land your changes. - [1] comment and dispatching of nfc-state-change event removal (I've noticed you removed the code which listens for this event in statusbar.js) - [2] removal of the comment and I think also removal of the skipped test, since I don't think it will be possible to run it successfully. Icon is shown when NFC HW change is successful (this also enables NFC toggle in settings). Previously the icon was shown when the request to change the HW was sent, without paying attention to its result and the test was working fine. What do you think? [1] - https://github.com/mozilla-b2g/gaia/blob/8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/js/nfc_manager.js#L315 [2] - https://github.com/mozilla-b2g/gaia/blob/8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/test/marionette/statusbar_icon_visibility_test.js#L46

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #3) > No problem. I wasn't aware of the statusbar refactoring effort. All what you > wrote makes perfect sense. > > So there is actually some clean up needed from bug 1089486 which introduced > nfc.status property. I think it would make sense to do it also in your bug, > but if you don't agree I can do this in this bug, once you land your > changes. > - [1] comment and dispatching of nfc-state-change event removal (I've > noticed you removed the code which listens for this event in statusbar.js) > - [2] removal of the comment and I think also removal of the skipped test, > since I don't think it will be possible to run it successfully. Icon is > shown when NFC HW change is successful (this also enables NFC toggle in > settings). Previously the icon was shown when the request to change the HW > was sent, without paying attention to its result and the test was working > fine. > > What do you think? > > [1] - > https://github.com/mozilla-b2g/gaia/blob/ > 8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/js/nfc_manager.js#L315 > [2] - > https://github.com/mozilla-b2g/gaia/blob/ > 8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/test/marionette/ > statusbar_icon_visibility_test.js#L46 I don't know the detail of recent nfc change, but lemme ask a question before we continue: Are we planning to use this settings to notify? What is the value of this settings? Are we having intermittent value like 'enabling'/'disabling'? Who will write(set) this setting? Or will we have an API like navigator.mozNfc.enabled = true; afterwards? just like navigator.mozMobileConnection. The answer will decide what we should do. Thanks.

nfc.status has the following values: 'enabling', 'enabled', 'disabling', 'disabled'. It is controlled completely by nfcManager, which sets its value to 'disabled' on init and changes it accordingly in nfcManget._changeHardwareState. This week navigator.mozNfc.enabled API was introduced.

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #5) > nfc.status has the following values: 'enabling', 'enabled', 'disabling', > 'disabled'. It is controlled completely by nfcManager, which sets its value > to 'disabled' on init and changes it accordingly in > nfcManget._changeHardwareState. > > This week navigator.mozNfc.enabled API was introduced. Okay, so currently I still tend to use event before my patches lands. The reason is already stated above: settings change is async and slow, we should update the icon as soon as the HW state is changed. Do you agree?

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #3) > No problem. I wasn't aware of the statusbar refactoring effort. All what you > wrote makes perfect sense. > > So there is actually some clean up needed from bug 1089486 which introduced > nfc.status property. I think it would make sense to do it also in your bug, > but if you don't agree I can do this in this bug, once you land your > changes. > - [1] comment and dispatching of nfc-state-change event removal (I've > noticed you removed the code which listens for this event in statusbar.js) Fine, but if later we have other modules in system app needs to know nfc state changed I prefer to introduce it again. > - [2] removal of the comment and I think also removal of the skipped test, > since I don't think it will be possible to run it successfully. Icon is > shown when NFC HW change is successful (this also enables NFC toggle in > settings). Previously the icon was shown when the request to change the HW > was sent, without paying attention to its result and the test was working > fine. How do we know the result of hardware state? dom request? If so let's update the icon in the dom request callback. > > What do you think? > > [1] - > https://github.com/mozilla-b2g/gaia/blob/ > 8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/js/nfc_manager.js#L315 > [2] - > https://github.com/mozilla-b2g/gaia/blob/ > 8b70b6038b76034142499e70a42e3cd09034e4a4/apps/system/test/marionette/ > statusbar_icon_visibility_test.js#L46

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7) > Fine, but if later we have other modules in system app needs to know nfc > state changed I prefer to introduce it again. Sure, I agree. > How do we know the result of hardware state? dom request? If so let's update > the icon in the dom request callback. NFC API is now promise based. And right now the icon is updated when the promise is successfully resolved. Ok, so I'll wait for your bug to land, mark this one as dependant. If there we'll be some cleanup needed after your bug will land, I'll do this here, if not I'll mark this as invalid.

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #8) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7) > > Fine, but if later we have other modules in system app needs to know nfc > > state changed I prefer to introduce it again. > Sure, I agree. > > > How do we know the result of hardware state? dom request? If so let's update > > the icon in the dom request callback. > NFC API is now promise based. And right now the icon is updated when the > promise is successfully resolved. > > Ok, so I'll wait for your bug to land, mark this one as dependant. If there > we'll be some cleanup needed after your bug will land, I'll do this here, if > not I'll mark this as invalid. Thank you Tauzen!

ni? me to remember to take a look if some clean-up is needed, will do this once SE situation is clear

Clean up mentioned in comment 8 was done as part of Bug 1141490. Marking this as invalid.