1103874 - NFC - Statusbar should listen to nfc.status setting to display NFC icon

Status: RESOLVED INVALID

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pull-request-1103874.txt

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

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

(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?

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

(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

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

(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!

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

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