Closed Bug 1082440 Opened 5 years ago Closed 5 years ago

B2G NFC: Dispatch NFC events to foreground app

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2+, tracking-b2g:backlog, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
feature-b2g 2.2+
tracking-b2g backlog
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: allstars.chh, Assigned: dimi)

References

Details

Attachments

(1 file, 8 obsolete files)

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.
Duplicate of this bug: 1065307
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.
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?
Assignee: nobody → allstars.chh
Attached patch Patch (obsolete) — Splinter Review
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.
Attached patch Patch. v2. (obsolete) — Splinter Review
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
Attachment #8522876 - Attachment is obsolete: true
Attachment #8524508 - Flags: feedback?(jonas)
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.
Attachment #8524508 - Flags: feedback?(bugs)
> 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.
Flags: needinfo?(alive)
feature-b2g: --- → 2.2?
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.
Attachment #8524508 - Flags: feedback?(bugs)
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.
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(alive)
(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?
Flags: needinfo?(kchen)
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.
Flags: needinfo?(vchang)
Flags: needinfo?(skamat)
Flags: needinfo?(jonas)
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).
Attachment #8524508 - Flags: feedback?(jonas)
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?
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.
Flags: needinfo?(jonas)
Flags: needinfo?(vchang)
Attachment #8524508 - Attachment is obsolete: true
Flags: needinfo?(skamat)
wait for Bug 1105666 so not actively working on this.
Assignee: allstars.chh → nobody
blocking-b2g: --- → backlog
feature-b2g: 2.2? → 2.2+
Assignee: nobody → dlee
I will work on this because this bug is highly related to bug 1105666 and bug 1117633
Depends on: 1117633
This patch doesn't include fallback event to system app if gaia app cannot handle it.
Attachment #8546501 - Flags: review?(allstars.chh)
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.
Attachment #8546501 - Flags: review?(allstars.chh) → review-
Blocks: 1120315
After discussing with yoshi, this patch will fix that DOM event should not be notified when any foreground app register NFC event.
Attachment #8546501 - Attachment is obsolete: true
Attachment #8547404 - Flags: review?(allstars.chh)
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
Hi Dimi, 
Can you set the target milestone for this bug?
- Move SYSTEM_APP_ID to idl and nfc_const
- Refine code in Nfc.js
Attachment #8547404 - Attachment is obsolete: true
Attachment #8548035 - Attachment is obsolete: true
Attachment #8547404 - Flags: review?(allstars.chh)
Attachment #8548038 - Flags: review?(allstars.chh)
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.
Attachment #8548038 - Flags: review?(allstars.chh) → review+
Target Milestone: --- → 2.2 S5 (6feb)
Change in this patch
-rebase to latest code
-remove unnecessary "if" addressed in Comment 26
Attachment #8548038 - Attachment is obsolete: true
- move SYSTEM_APP_ID define position prior to export symbols

Re-run try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=122ca76f8a61
Attachment #8553478 - Attachment is obsolete: true
Attachment #8553487 - Flags: review+
Fix white space nits
Attachment #8553487 - Attachment is obsolete: true
Attachment #8553490 - Flags: review+
try run looks good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c6605293bf1d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
No longer blocks: 1120315
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)
Attachment #8553490 - Flags: approval-mozilla-b2g37?
Attachment #8553490 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
No longer blocks: b2g-nfc
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.