1082440 - B2G NFC: Dispatch NFC events to foreground app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Right now we reply on ShrinkingUI to notify Gecko the information of current foreground app, then Gecko dispatches the NFC event to that app.

But this requires close integration between System app and Gecko. See Bug 933136.

Also the reason why the event is dispatched to 'foregound app only' is that we'd like to make the foreground app could receive the NFC events directly without popping up Activity Chooser (User could possibly doesn't know which app to choose)

And this bug is filed to discuss if there's a better way to get the information of foreground app more easily, also to discuss if there are multiple foreground apps (like in Tablet) what's the desired behavior to dispatch NFC event? Dispatch to only 1 app or dispatch to all visible apps?

We have some ideas already.

1. Add a new NFC API, and System app will call this API every time when the foreground app changes.

2. Like above, but this API is an interface in Browser API, like setVisible.

3. use document.hidden or document.hasFocus
However this has some drawbacks like when doing app transition, two apps could both document.hidden=false (i.e. visible) at the same time. And for dragging the notification bar and drag it back, the foreground app will lose focus.

Last I'd file two bugs that may relate to this bug.
1. If the foreground app cannot handle this NFC event (like ontagfound, the foreground app cannot handle this tag), we should rollback and dispatch the NFC event to System app.

2. To discuss should we allow an app in background could doing NFC? For example, the foreground app received NFC event before, now it goes to background.

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

I would prefer to dispatch NFC Events to only one, currently focused foreground app. I think it would be really confusing if multiple apps would get access to the same NFCTag at once and try to write some data on it. 

document.hasFocus sounds like a good idea, but we should do something to get back the focus after dragging the notification bar down and up.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In this bug we can deliever NFC events to the app whose document.hidden is false or visibilityState is visible.

If there are more then one app receives the event, like in App-transition. The one later goes to background should receive a ontaglost/peerlost in Bug 1082445.

However if before it goes to background, suppose two apps are both in the foreground at the same time, should we allow these two apps could receive NFC event?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8522876 [details] [diff] [review]
Patch

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

::: dom/nfc/nsNfc.js
@@ +274,5 @@
>        dump("this._window or this.__DOM_IMPL__ is a dead wrapper.");
>        return;
>      }
>  
> +    if (this._window.document.hidden) {

When notification bar is dragged down, although the foreground app is still visible, but will lose focus (document.hasFocus() is false),
after discussed with UX, current ShrinkingUI shouldn't be triggerred if the notification bar is dragged. (Bug 1100789 will fix this)

However if the ShrinkingUI is already displayed, but user still wants to drag the notification bar, it's okay to do this.

So I'll add another condition document.hasFocus to check if we'd dispatch this NFC event to the content, and will NOT listen to focus change event since current UX allows this.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2.

Add hasFocus check.

Ask Jonas for feedback.

Hi Jonas
For WebNFC, you said that NFC API should be more generic, and shouldn't have too much dependency on UX.

And in this bug we'd like to dispatch NFC events (when a tag or a NFC device is detected) to the *foreground* app. (Before it is ShrinkingUI which will tell Gecko the info of foreground app)

However this already involves some UX decision for 'defining a foreground app'.
In this patch I use 'document.hidden' and 'document.hasFocus()' to determine this.

I am not sure if doing this, do we still has some UX dependency in our API?
Also if this is NOT what you intented, do you have any idea to isolate these?
Or do we need to add a NFC API to let System app to tell Gecko the information of current foreground app ? (Like a Browser API setVisible and that API is more NFC-wise)

So I'd like to request your feedback on this

Thanks

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8524508 [details] [diff] [review]
Patch. v2.

Also request for feeback from smaug to see if using document.hidden and document.hasFocus are appropriate.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> However this already involves some UX decision for 'defining a foreground
> app'.

Yeah, this is a problem that we've had for quite a while.

> In this patch I use 'document.hidden' and 'document.hasFocus()' to determine
> this.

I *think* document.hidden=false means that the app is the foreground app in B2G. But I'm not 100% sure. I think there are edge cases like if the phone is locked, or if the screen is off.

Alive might know more about when we set document.hidden to true/false, and when document.hidden does not equal "foreground app".

Ideally we'd have some API exposed to the system app for "has NFC focus". This is essentially the visibility API that we've talked about for a long time and that Kan-Ru is working on. But I don't know when that will be ready.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8524508 [details] [diff] [review]
Patch. v2.

cancel r? to smaug as we'd like to get more discussion from Alive, Kanru and Jonas.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Also Jonas said it would be great if someone could add a setNFCFocus(true/false) API to
the browser API, ni? for Kanru on this.

Comment 11

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

(In reply to Jonas Sicking (:sicking) from comment #8)
> > However this already involves some UX decision for 'defining a foreground
> > app'.
> 
> Yeah, this is a problem that we've had for quite a while.
> 
> > In this patch I use 'document.hidden' and 'document.hasFocus()' to determine
> > this.
> 
> I *think* document.hidden=false means that the app is the foreground app in
> B2G. But I'm not 100% sure. I think there are edge cases like if the phone
> is locked, or if the screen is off.
> 
> Alive might know more about when we set document.hidden to true/false, and
> when document.hidden does not equal "foreground app".
> 
> Ideally we'd have some API exposed to the system app for "has NFC focus".
> This is essentially the visibility API that we've talked about for a long
> time and that Kan-Ru is working on. But I don't know when that will be ready.

So there are some edge cases when !document.hidden !=== it is the top most iframe
* When it is playing normal channel audio (bug 961967)
* When doing web activity call (caller is always foreground if callee is foreground) (bug 892371)
* When doing app transition (bug 1034001)

Not all of them are in progress so I will not say: document.hidden = false means it is the top most app.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> Also Jonas said it would be great if someone could add a
> setNFCFocus(true/false) API to
> the browser API, ni? for Kanru on this.

The visibility API is bug 1034001

Given that we have other proposals to handle the different visibility issues (e.g. audio channel management api, lazy gfx resource expunge, setInputMethodActive) I'm leaning to implement NFC as another set of methods/attributes as the audio channel management api.

Thoughts?

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks for Alive's and Kanru's comments.

So this bug will depend on new Browser API feature, so maybe two options 
1. We wait until the new Browser API feature landed. In this case NFC feature will be delayed.

2. We start with 'document.hidden' and 'document.hasFocus' as our experiment implementation, since these could work on usually cases. Once the Browser API feature is landed we change to use it, or if 'document.hidden' has too many problems, we back this patch out.

Ni? for Jonas, Sandip, Vincent for the decision.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8524508 [details] [diff] [review]
Patch. v2.

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

Removing this based on recent discussion. Yes, I think a dedicated API on the browser API, similar to what we're discussion for audio channels, would work. As long as it's only exposed to the system app (also like the audio channels API).

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

So we actually have 'setActive' in browser-api that only set visibility on the parent side. I think Gaia now could ensure there is only one App is foreground (from b2g's point of view). Could you consider this approach?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That could probably work yeah. Though I think long term we probably should have an explicit "nfc focus" bit which can be assigned separately from visible. Rather than relying on that only one app is visible at all times.

But if you think the visibility thing will work for now then I'm fine with that.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

wait for Bug 1105666 so not actively working on this.

Comment 18

[Dimi Lee [:dimi]]

I will work on this because this bug is highly related to bug 1105666 and bug 1117633

Comment 19

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v1

This patch doesn't include fallback event to system app if gaia app cannot handle it.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8546501 [details] [diff] [review]
Dispatch NFC events to foregroun patch v1

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

::: dom/nfc/gonk/Nfc.js
@@ +247,1 @@
>        }

remove this, system app use system message.

@@ +255,5 @@
> +      if (target) {
> +        this.notifyDOMEvent(target,
> +                            { event: NFC.TAG_EVENT_LOST,
> +                              sessionToken: sessionToken });
> +      }

This is totally wrong.
ontaglost should be fired to the app if the app has receive ontagfound before.
But only content side has this information.

@@ +266,5 @@
>                                sessionToken: sessionToken });
>        }
>      },
>  
>      onPeerEvent: function onPeerEvent(eventType, sessionToken) {

This function handles PEER_FOUND and PEER_LOST
And PEER_LOST could be for
1. for PEER_FOUND
2. for PEER_READY

So you have to be careful to handle two cases here.

Comment 21

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v2

After discussing with yoshi, this patch will fix that DOM event should not be notified when any foreground app register NFC event.

Comment 22

[Dimi Lee [:dimi]]

Comment on attachment 8547404 [details] [diff] [review]
Dispatch NFC events to foregroun patch v2

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

::: dom/nfc/gonk/Nfc.js
@@ +275,5 @@
> +        this.notifyDOMEvent(target, message);
> +        return;
> +      }
> +
> +      // Before bug 1082443 is fixed, we still need to notify systemn app here.

I will remove this comment in next patch

Comment 23

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Dimi, 
Can you set the target milestone for this bug?

Comment 24

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v3

Comment 25

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v4

- Move SYSTEM_APP_ID to idl and nfc_const
- Refine code in Nfc.js

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8548038 [details] [diff] [review]
Dispatch NFC events to foregroun patch v4

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

::: dom/nfc/gonk/Nfc.js
@@ +236,2 @@
>        message.event = NFC.TAG_EVENT_FOUND;
> +      if (target) {

if check is not needed.

Comment 27

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v5

Change in this patch
-rebase to latest code
-remove unnecessary "if" addressed in Comment 26

Comment 28

[Dimi Lee [:dimi]]

Running try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d30bde00637

Comment 29

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v6

- move SYSTEM_APP_ID define position prior to export symbols

Re-run try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=122ca76f8a61

Comment 30

[Dimi Lee [:dimi]]

Attachment: Dispatch NFC events to foregroun patch v7

Fix white space nits

Comment 31

[Dimi Lee [:dimi]]

try run looks good

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c6605293bf1d

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6605293bf1d

Comment 34

[Dimi Lee [:dimi]]

Comment on attachment 8553490 [details] [diff] [review]
Dispatch NFC events to foregroun patch v7

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
NFC feature

User impact if declined: 
Third party app will get nfc notification even it is in background

Testing completed: 
Manually with patch provided in bug 1117633

Risk to taking this patch (and alternatives if risky): 
No.

String or UUID changes made by this patch:
nsINfcContentHelper.idl nsINfcBrowserAPI interface
uuid(7ae46728-bc42-44bd-8093-bc7563abf52d) to
uuid(7ae94107-adc9-4467-ae44-72f9a91f3ee8)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/db5a15dd2d16