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

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
NFC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tauzen, Assigned: tauzen)

Tracking

unspecified
2.1 S6 (10oct)
x86
Mac OS X

Firefox Tracking Flags

(b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
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?
Assignee: nobody → kmioduszewski
Flags: needinfo?(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?
Flags: needinfo?(alive)
(Assignee)

Comment 3

3 years ago
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?
Flags: needinfo?(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 :)
Flags: needinfo?(alive)
(Assignee)

Comment 5

3 years ago
Created attachment 8498762 [details] [review]
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.
Attachment #8498762 - Flags: review?(alive)
Comment on attachment 8498762 [details] [review]
pull-request-1074800.txt

r=me
Attachment #8498762 - Flags: review?(alive) → review+
(Assignee)

Comment 7

3 years ago
Thanks!
Keywords: checkin-needed
Summary: [NFC] NfcHandler shoud listen for peerready only when NFC is enabled → [NFC] NfcHandler should listen for peerready only when NFC is enabled
Master: https://github.com/mozilla-b2g/gaia/commit/83f495a1c12687970f7f2840c2729795c4b88177
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
You need to log in before you can comment on or make changes to this bug.