1074800 - [NFC] NfcHandler should listen for peerready only when NFC is enabled

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

Currently NfcHandler is instantiated and starts to listen for peerready events in AppWindowManager init method [1]. I would like to propose to use settings observer and instantiate and/or start listening only when NFC is enabled (and stop listening on disabling). Since NFC can be enabled only when NFC hardware is available we won't need to have this checks in NfcHandler [2].  

[1] - https://github.com/mozilla-b2g/gaia/blob/e68f68150b89887db48acd59b8a66c3cb7f9a5bc/apps/system/js/app_window_manager.js#L250
[2] - https://github.com/mozilla-b2g/gaia/blob/bc4eef97e30ac1d484f82b3879b33fffd2f732db/apps/system/js/nfc_handler.js#L10

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Hi Alive, could I get your opinion on this bug? Do you think it is worth to implement the changes which I proposed in the description?

Comment 2

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #1)
> Hi Alive, could I get your opinion on this bug? Do you think it is worth to
> implement the changes which I proposed in the description?

Personally I agree this is a good idea but I suspect how to easily implement it now.
What's your design to realize it?

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

I would like to use the _settingsObserverHandler already used by AppWindowManager. The proposed solution is here: https://github.com/tauzen/gaia/commit/f3e1a468c4387c892b8a5c663d0f4e1feb0fcb80. What do you think?

Comment 4

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #3)
> I would like to use the _settingsObserverHandler already used by
> AppWindowManager. The proposed solution is here:
> https://github.com/tauzen/gaia/commit/
> f3e1a468c4387c892b8a5c663d0f4e1feb0fcb80. What do you think?

Works for me :)

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pull-request-1074800.txt

I hope that the full patch will be also ok ;)

Actually I've decided not to remove the checks for mozNfc in NfcHandler, this is because:
- it follows the same pattern as in other apps using onpeerready (removing checks might be confusing for future developers comparing NfcHandler with other sharing apps)
- in case of future changes, if we will decide to call |start()| or |stop()| in different situation, we prevent TypeError throwing if someone forgets to check for mozNfc.

Comment 6

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

Comment on attachment 8498762 [details] [review]
pull-request-1074800.txt

r=me

Comment 7

[Krzysztof Mioduszewski[:tauzen]]

Thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/83f495a1c12687970f7f2840c2729795c4b88177