Closed Bug 1082445 Opened 7 years ago Closed 7 years ago

B2G NFC: notifiy peer/taglost when app goes to background.

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=3])

Attachments

(1 file, 3 obsolete files)

Supposes a foreground app, Foo, now gets an NFC event, i.e. onpeerfound or ontagfound, then Foo goes to background, for example, user presses HOME key.
Should we allow Foo calling NFC API?

If we allow this, Foo could possibly share some (secret) data to another NFC device (or tag) without user perception.

Also when Foo is in background, can it still receive onpeerlost/ontaglost callback when the NFC device/tag is away?
Summary: B2G NFC: Should we allow calling NFC API for background apps? → B2G NFC: Should we allow calling NFC API from background apps?
(In reply to Yoshi Huang[:allstars.chh] from comment #0)
> Also when Foo is in background, can it still receive onpeerlost/ontaglost
> callback when the NFC device/tag is away?
This part should be discussed in Bug 1082440.
The plan now is to listen to onvisibilitychange in NFC DOM part(nsNfc.js).

If the (foreground) app has been notified ontag/peerfound before, and then if NFC DOM part finds out the app has been switched to background (i.e. become hidden), we may have 2 options 

1. fire ontaglost/peerlost to the app, even the phisical tag/peer is still in proximity. So calling NFCTag/NFCPeer API should throw(done in Bug 1085292), later when the app is back to the foreground, the ontag/peerlost should NOT be notified again when the phisical tag/peer has been removed.

2. We don't fire ontag/peerlost, but we change some internal state of MozNFC objects(MozNFC, MozNFCTag, and MozNFCpeer), so calling the NFC APIs on the object received in ontag/peerfound before should throw. Later when the app is back to foreground, ontag/peerlost SHOULD be notified if the phisical tag/peer has been removed.

Hi Smaug
May I have your feedback on this topic here?
Should we notify ontaglost/onpeerlost if the app which received ontagfound/onpeerfound before has been switched to background?
Or should we change the internal state of those NFC objects so calling NFC API in the background should throw?

Thanks
Flags: needinfo?(bugs)
(In reply to Yoshi Huang[:allstars.chh] from comment #2)
> The plan now is to listen to onvisibilitychange in NFC DOM part(nsNfc.js).
Make sure to add event listener to the right place so that the application/web page can't call stopImmediatePropagation() before
your listener.

> Hi Smaug
> May I have your feedback on this topic here?
> Should we notify ontaglost/onpeerlost if the app which received
> ontagfound/onpeerfound before has been switched to background?
> Or should we change the internal state of those NFC objects so calling NFC
> API in the background should throw?

I think (1) sounds simpler and better option to me.
Flags: needinfo?(bugs)
Thanks, smaug.
I'll work on this by your suggestion.
Assignee: nobody → allstars.chh
(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Yoshi Huang[:allstars.chh] from comment #2)
> > The plan now is to listen to onvisibilitychange in NFC DOM part(nsNfc.js).
> Make sure to add event listener to the right place so that the
> application/web page can't call stopImmediatePropagation() before
> your listener.

Actually, the best option is to add event listener to the system event group, since that way web page/app can't
prevent the propagation. (stop*Propagation() in the default group affects to that group only)
Comment on attachment 8521228 [details] [diff] [review]
WIP. Patch

Review of attachment 8521228 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, Smaug
Thanks for your suggestion,
adding the event to System Event Group does WORK.

Can you help to check am I doing the correct thing here?

Thanks
Attachment #8521228 - Flags: feedback?(bugs)
Comment on attachment 8521228 [details] [diff] [review]
WIP. Patch

That looks ok. 
if (event.type != "visibilitychange") { shouldn't be needed
(and remove the dump() ;) )


I wonder if the listener should be removed from aWindow at some point.
Or does it matter if aWindow keeps your 'this' alive?
Attachment #8521228 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #8)
> I wonder if the listener should be removed from aWindow at some point.
> Or does it matter if aWindow keeps your 'this' alive?

Hi smaug
Thanks for your feedback.

I think next version I'll add the listener when a tag/peer is detected, and remove the listener when the tag/peer is removed.

Thanks
Attached patch Patch. v2. (obsolete) — Splinter Review
Attachment #8521228 - Attachment is obsolete: true
Comment on attachment 8522875 [details] [diff] [review]
Patch. v2.

Review of attachment 8522875 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Yoshi,
Should we check visibility in |eventListenerWasAdded| to prevent background apps try to register events like tagfound, peerfound ...etcs?

::: dom/nfc/nsNfc.js
@@ +284,5 @@
>        return;
>      }
>  
> +    this.eventService.addSystemEventListener(this._window, "visibilitychange",
> +      this.tagVisibilityHandler.bind(this), /* useCapture */false);

add nsIDOMEventListener to queryInterface?

@@ +337,5 @@
>      this.__DOM_IMPL__.dispatchEvent(event);
>    },
>  
> +  tagVisibilityHandler: function tagVisibilityHandler(event) {
> +    debug("tagVisibilityHandler document.hidden="+this._window.document.hidden);

Nits. space between +
same as peerVisibilityHandler
Attachment #8522875 - Flags: review?(dlee)
(In reply to Dimi Lee[:dimi][:dlee] from comment #11)
> Should we check visibility in |eventListenerWasAdded| to prevent background
> apps try to register events like tagfound, peerfound ...etcs?
> 
App could register events even in background, we just need to make sure those callbacks won't be called when the app is background. 

> ::: dom/nfc/nsNfc.js
> @@ +284,5 @@
> >        return;
> >      }
> >  
> > +    this.eventService.addSystemEventListener(this._window, "visibilitychange",
> > +      this.tagVisibilityHandler.bind(this), /* useCapture */false);
> 
> add nsIDOMEventListener to queryInterface?
> 
Right now the system event listener will be removed when notifyTag/PeerLost, I seperate tag and peer cases because we can detect a tag and a peer at the same time, however removing one of them shouldn't remove the event listener of the other.
for example, when we get a taglost we shouldn't remove the visibility listener for peer.
Summary: B2G NFC: Should we allow calling NFC API from background apps? → B2G NFC: notifiy peer/taglost when app goes to background.
(In reply to Yoshi Huang[:allstars.chh] from comment #12)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #11)
> > Should we check visibility in |eventListenerWasAdded| to prevent background
> > apps try to register events like tagfound, peerfound ...etcs?
> > 
> App could register events even in background, we just need to make sure
> those callbacks won't be called when the app is background. 
> 
And this should be done in Bug 1082440.
(In reply to Yoshi Huang[:allstars.chh] from comment #13)
> (In reply to Yoshi Huang[:allstars.chh] from comment #12)
> > (In reply to Dimi Lee[:dimi][:dlee] from comment #11)
> > > Should we check visibility in |eventListenerWasAdded| to prevent background
> > > apps try to register events like tagfound, peerfound ...etcs?
> > > 
> > App could register events even in background, we just need to make sure
> > those callbacks won't be called when the app is background. 
> > 
> And this should be done in Bug 1082440.

Got it , thanks
Attachment #8522875 - Flags: review+
Attached patch Patch. v3. (obsolete) — Splinter Review
nits.
Attachment #8522875 - Attachment is obsolete: true
Attachment #8524288 - Flags: review+
Comment on attachment 8524288 [details] [diff] [review]
Patch. v3.

Add r? to smaug for the eventListenerWasAdded

Hi, Smaug
In this patch I just check the event listener in eventListenerWasAdded, however I am not sure if checking 
setting the event handler in [1] is also neccesary.

Will it be possible that the setter of event handler is called but not for eventListenerWasAdded?

Thanks

[1] http://lxr.mozilla.org/mozilla-central/source/dom/nfc/nsNfc.js#246
Attachment #8524288 - Flags: review?(bugs)
Comment on attachment 8524288 [details] [diff] [review]
Patch. v3.

Review of attachment 8524288 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, sorry
my previous comment is asking another bug 1097527.
Attachment #8524288 - Flags: review?(bugs)
Comment on attachment 8524288 [details] [diff] [review]
Patch. v3.

Review of attachment 8524288 [details] [diff] [review]:
-----------------------------------------------------------------

Add r? to smaug for the System Event Listener.

This patch has addressed the 'removing system event listener' by Smaug's comment in Comment 8,
Also in the patch I use one listener for tag and the other for peer.
The reason is that it's possible to detect a tag and a NFCPeer simultaneously,
so for example if the tag is removed we hope only remove the event listener for tag, not for the peer.

Thanks
Attachment #8524288 - Flags: review?(bugs)
Comment on attachment 8524288 [details] [diff] [review]
Patch. v3.

>+    this.eventService.removeSystemEventListener(this._window, "visibilitychange",
>+      this.tagVisibilityHandler.bind(this), /* useCapture */false);
AFAIK, bind() creates a new function, so this call doesn't actually remove anything, since remove*EventListener requires the
same function which was used for add*EventListener.

>+    this.eventService.removeSystemEventListener(this._window, "visibilitychange",
>+      this.peerVisibilityHandler.bind(this), /* useCapture */false);
ditto
Attachment #8524288 - Flags: review?(bugs) → review-
Attached patch Patch v4.Splinter Review
updated according to smaug's suggestion,
r? to Dimi again.
Attachment #8524288 - Attachment is obsolete: true
Attachment #8525116 - Flags: review?(dlee)
Comment on attachment 8525116 [details] [diff] [review]
Patch v4.

Review of attachment 8525116 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/nsNfc.js
@@ +415,5 @@
> +
> +    if (this.nfcPeer) {
> +      debug("handleEvent notifyPeerLost");
> +      this.notifyPeerLost(this.nfcPeer.session);
> +    }

I know now we may only have visibility change event but I think check |event|
before doing these things will make it more clear.
Attachment #8525116 - Flags: review?(dlee) → review+
(In reply to Dimi Lee[:dimi][:dlee] from comment #21)
> I know now we may only have visibility change event but I think check |event|
> before doing these things will make it more clear.

Smaug suggested to remove it at Comment 8 so I just did.
Comment on attachment 8525116 [details] [diff] [review]
Patch v4.

Review of attachment 8525116 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, smaug
I changed to implement nsIDOMEventListener, and add some check before removing the system event listener.

Could you help to review this again?

Thanks
Attachment #8525116 - Flags: review?(bugs)
Comment on attachment 8525116 [details] [diff] [review]
Patch v4.

(not sure if you actually have to add Ci.nsIDOMEventListener, but doesn't harm.)
Attachment #8525116 - Flags: review?(bugs) → review+
Whiteboard: [p=3]
Target Milestone: --- → 2.2 S1 (5dec)
https://hg.mozilla.org/mozilla-central/rev/3deea6e95e6a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.