1003268 - B2G NFC: implement onpeerfound

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Michał Budzyński (:michalbe)]

Attachment: [NFC] Pairing Vcard_20131202.pdf

For now there it's impossible for Gaia apps to know when the devices are tapped together (only system app receives right system msgs). We will need this for Contacts app, according to Bug 997599, to display a notification when user tries to share Facebook data (attachment, page 8).

Comment 1

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

If we need to support this feature, we need to implement onpeerfound callback from W3C.

http://w3c.github.io/nfc/proposals/common/nfc.html

Comment 2

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

https://bugzilla.mozilla.org/attachment.cgi?id=8414588
From page#8, the step3 NFC pairing requires this work. 
There might be significant modifications on NFC manager per discussion with Ken.
Juwei, should we discuss on the possibility of skipping the "red flash"?

Comment 3

[Ken Chang[:kenkai]]

Garner, if we want to do this, do you have any idea how to do it?

Comment 4

[Michał Budzyński (:michalbe)]

:wesley_huang :
As I answered by email - there is also this toaster notification that said 'The contact cannot be shared due to Facebook constraint.' on the same page, next image. It will need this event too.

Comment 5

[Ken Chang[:kenkai]]

We can have 2 options if we can remove "red flash". 1. Always to display this notification when user selects a faceboot contact. 2. Always to display nothing...:-(.

Comment 6

[Michał Budzyński (:michalbe)]

The first option will not really work - the notification will pops up every time you'll open contact details, what makes very little sense. The only difference between sharing & opening details is that you need to tap devices when you want to share.

Comment 7

[Michał Budzyński (:michalbe)]

The fix for the second one is already done [1], so if it's acceptable solution I'm ready to send it for r? and land.

[1] https://github.com/michalbe/gaia/commit/c1413dba3590c06f24493fd2280573a54916909c

Comment 8

[Garner Lee]

onpeerready (W3C: onpeerfound), are the right functions to set callbacks to, although onpeerready in FirefoxOS has ShrinkUI in front for the user to block the P2P callback access to the app.

A Device is sending FB contact (using ShrinkUI), a question:

  if (fb.isFbContact(contact) && !fb.isFbLinked(contact)) {}

NFC Manager and the NFC DOM doesn't know what the app wants to share until the app calls sendNDEF().

I'm not quite understanding what and where the "red flash" problem is (or is that the FB contact sharing warning/Notification message?)

Is it sufficient for a privilaged app, or a marketplace non-privilaged app to meet legal agreements/requirements to filter a facebook "vcard" contact before sending? The app already can see the FB contact it wants to send (and can reformat it to anything it wants, like a non-vcard text file), correct?

How would the system (or NFC DOM) block a FB vcard contact then?

  mozNFC.sendNDEF(NDEF_Text_Data_With_Forbidden_FB_Info);

Comment 9

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

* It's our design to let system shrink the app but not the app. In android it's the app calls an activity to shrink itself. Because we shrink app in system but handle nfc in app, we cannot do such special error handling in app side. API change is needed.
* What kind of change? I recalled there's a discussion to send onpeerfound to top most app but I forgot the bug number. Maybe we could always send onpeerfound to the active app but if app doesn't handle it or doesn't have the permission, we start the shrinking and send onpeerready.

(In reply to Garner Lee from comment #8)
> onpeerready (W3C: onpeerfound), are the right functions to set callbacks to,
> although onpeerready in FirefoxOS has ShrinkUI in front for the user to
> block the P2P callback access to the app.
> 
> A Device is sending FB contact (using ShrinkUI), a question:
> 
>   if (fb.isFbContact(contact) && !fb.isFbLinked(contact)) {}
> 
> NFC Manager and the NFC DOM doesn't know what the app wants to share until
> the app calls sendNDEF().
> 
> I'm not quite understanding what and where the "red flash" problem is (or is
> that the FB contact sharing warning/Notification message?)
> 
> Is it sufficient for a privilaged app, or a marketplace non-privilaged app
> to meet legal agreements/requirements to filter a facebook "vcard" contact
> before sending? The app already can see the FB contact it wants to send (and
> can reformat it to anything it wants, like a non-vcard text file), correct?
> 
> How would the system (or NFC DOM) block a FB vcard contact then?
> 
>   mozNFC.sendNDEF(NDEF_Text_Data_With_Forbidden_FB_Info);

This does not block shrinking, does it?

Comment 10

[Juwei Huang[:juwei]]

New spec updated on bug 997599

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 1: MozNFC.webidl changes to support onpeerfound

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 2: NFC DOM and NfcContentHelper changes to support onpeerfound

Comment 13

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 3: Nfc.js changes to support onpeerfound

Hi Yoshi, could you review this patches? I'm using this demo app [1] to tests the the new event. 

There is still one problem here which needs to be addressed, probably in a different bug - Gecko does not know which app is currently in the foreground. In the demo app I solved this by using 'visibilitychange' events [2] to register/unregister onpeerfound handler when the app is visible. But since we would like to expose this to other developers we need have the info about currently visible app directly in Gecko.    

[1] - https://github.com/tauzen/onpeerfound-demo
[2] - https://github.com/tauzen/onpeerfound-demo/blob/7b1b3383c1f809c4144ea8902f3c17338a80716d/js/app.js#L36

Comment 14

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

Comment on attachment 8485689 [details] [diff] [review]
Part 1: MozNFC.webidl changes to support onpeerfound

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

::: dom/webidl/MozNFC.webidl
@@ +67,5 @@
>     [CheckPermissions="nfc-write"]
>     attribute EventHandler onpeerlost;
> +
> +   /**
> +    * This event will be fired when NFCPeer is detected. No confirmation from user

when a NFCPeer is detected.
Remove 'No confirmation from user'

Comment 15

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

Comment on attachment 8485695 [details] [diff] [review]
Part 3: Nfc.js changes to support onpeerfound

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

::: dom/nfc/gonk/Nfc.js
@@ +244,5 @@
>  
> +    onPeerFound: function onPeerFound(message) {
> +      if (message.records || message.techList.join() !== 'P2P') {
> +        return false;
> +      }

The check should be done Bug 1058490

@@ +250,5 @@
> +      let appId = Object.keys(this.peerTargets).find((appId) => {
> +        // TODO we need to check here if app is visible, right
> +        // now we use first app which registered onpeerfound, asuming that
> +        // the app uses |visibiltychange| events to register/unregister
> +        // onpeerfound handler

fix TODO first.

Comment 16

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) Bug 1003268 Part 2: NFC DOM and NfcContentHelper changes to support onpeerfound, with visbility change monitoring

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) Part 3: Nfc.js, nfc_consts.js changes to support onpeerfound, with visibility change monitoring

Comment 18

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

Comment on attachment 8490001 [details] [diff] [review]
(v1.1) Bug 1003268 Part 2: NFC DOM and NfcContentHelper changes to support onpeerfound, with visbility change monitoring

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

I still prefer registering an EventTarget in Bug 1069177 so setting a DOM callback shouldn't trigger IPC.

Comment 19

[Krzysztof Mioduszewski[:tauzen]]

Attachment: WIP onpeerfound support, no IPC on setting callback

Hi Yoshi, could I have your feedback on this version of the patch? It's rebased against the latest Gecko, and as you suggested does not trigger IPC on callback registration.

Comment 20

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

Comment on attachment 8499554 [details] [diff] [review]
WIP onpeerfound support, no IPC on setting callback

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

::: dom/nfc/nsNfc.js
@@ +151,5 @@
>      debug("mozNfc init called");
>      this._window = aWindow;
> +
> +    this._eventHelper = {
> +      isVisibile: () => { return (this._window) ? !this._window.document.hidden : false; },

From developers from Browser API and System app, they suggest we'd use document.hasFocus to check.

The problem of document.hidden is when doing app transistion it could be possible that more then one apps will have document.hidden at the same time.

However we should also check document.hasFocus could work well when dragging notification bar. But I haven't checked that yet.

Comment 21

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

Comment on attachment 8499554 [details] [diff] [review]
WIP onpeerfound support, no IPC on setting callback

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

::: dom/nfc/gonk/Nfc.js
@@ +311,5 @@
>        }
>  
>        switch (message.name) {
>          case "NFC:AddEventTarget":
> +          this.addEventTarget({target: message.target, eventHelper: message.objects.eventHelper });

eventHelper is not neccesary.
Parent process should only care which child processes should receive NFC events.

And I don't think pass a function could work here.

Comment 22

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Yoshi Huang[:allstars.chh] from comment #20)
> From developers from Browser API and System app, they suggest we'd use
> document.hasFocus to check.
> 
> The problem of document.hidden is when doing app transistion it could be
> possible that more then one apps will have document.hidden at the same time.

This is true, but the transitions are triggered by user and are rather quick so there is a low possibility that a user will try to share something during a transition from one app to another (since this does not make much sense). The problem with document.hidden is that the system app has it set to false all the time, so we would probably need to rework system browser NFC URL share a bit. 

> However we should also check document.hasFocus could work well when dragging
> notification bar. But I haven't checked that yet.

As you wrote, hasFocus() is false when the notification bar is dragged down. When the notification bar is dragged up hasFocus() is still false until the user clicks on the screen. So this is actually a worse user experience, since the user will expect to be able to share if he sees the app. I think document.hidden is better. BTW, in the system app document.hasFocus() is also true.

Comment 23

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Yoshi Huang[:allstars.chh] from comment #21)
> eventHelper is not neccesary.
> Parent process should only care which child processes should receive NFC
> events.
But it still needs to know if there was any child process which could handle the event. If there isn't any app which can consume the event, parent process should dispatch the system message to NfcManager. I agree that keeping the dispatching logic in DOM impl makes more sense. I have one more idea how this can be implemented, which I think follows your guidelines. I will ask for feedback once I have this working and tested.

> And I don't think pass a function could work here.
I'm not sure if understand this correctly, could you explain this a bit more?

Comment 24

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #22)
> This is true, but the transitions are triggered by user and are rather quick
> so there is a low possibility that a user will try to share something during
> a transition from one app to another (since this does not make much sense).
But we still need to consider the edge case here.
Suppose A and B are doing app transition, and at this time they both get onpeerfound.
But if A is background now, it would be a security concern that user thought B is going to share data to another device, but actually it could be A is doing the sharing.

> As you wrote, hasFocus() is false when the notification bar is dragged down.
Yeah we need to fix this.
Only document.hidden is not enough either.

Comment 25

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #23)
> > And I don't think pass a function could work here.
> I'm not sure if understand this correctly, could you explain this a bit more?
Sorry I don't have time to try your patch, I don't know if it works or not.
But even if it works, you should explain in more detail about how it works.
How does the function call happen across process boundary?

Comment 26

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

I'd like to make things go more smoothly, so I'd like to move the discussion of 'Dispatch to foregound' to Bug 1082440.
So in this bug we only do 'implementing onpeerfound'.

Comment 27

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pointer-to-onpeerfound-demo.txt

Pointer to onpeerfound demo used for testing and patch verification.

Comment 28

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Yoshi Huang[:allstars.chh] from comment #25)
> Sorry I don't have time to try your patch, I don't know if it works or not.
> But even if it works, you should explain in more detail about how it works.
> How does the function call happen across process boundary?
Each patch I'm testing/validating with the onpeerfound demo (pointer to it is here -> attachment 8504767 [details]). I keep the demo in sync with every version of the patch.

Sorry for not explaining this earlier. I'm using here Cross Process Object Wrapper (CPOW) [1], to access content process in a synchronous way. The idea was to access the necessary info in each content process (which registered itself as eventTarget) and decide if the event should be dispatched to it or not.  
Actually |eventHelper| in parent process is a CPOW which is a reference to the corresponding object in content process. When I'm calling a method of |eventHelper| in parent process it sends a synchronous message to content process and returns the result. So for readability now I think it would be better to call the object in parent process eventHelperWrapper.

[1] - https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers

Comment 29

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #28)
> Sorry for not explaining this earlier. I'm using here Cross Process Object
> Wrapper (CPOW) [1], to access content process in a synchronous way. The idea
> was to access the necessary info in each content process (which registered
> itself as eventTarget) and decide if the event should be dispatched to it or
> not.  
> Actually |eventHelper| in parent process is a CPOW which is a reference to
> the corresponding object in content process. When I'm calling a method of
> |eventHelper| in parent process it sends a synchronous message to content
> process and returns the result. So for readability now I think it would be
> better to call the object in parent process eventHelperWrapper.
> 
> [1] -
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Cross_Process_Object_Wrappers

First I'd like to move the discussion Bug 1082440.

And back to your comments, I don't suggest to do this.
The reason is it should be the content process to decide whether the event should be fired to Gaia or not, not chrome process should decide this.

Chrome process only knows which child processes should have registered NFC callbacks, but the status of the 'window' or 'document' should be only known by the content process itself.

Otherwise image N apps have registered NFC events, when Chrome process is about to fire the NFC event, there could be (4 * N) IPCs sent between chrome/content processes.
2 IPCs(round-trip) for isVisible checks
2 IPCs for isPeerFoundRegistered checks.
And multiply by N apps.

Comment 30

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Implement-onpeerfound.patch

As discussed on IRC, this is the implementation of onpeerfound only. Dispatching to proper foreground app will be handled in Bug 1082440. Fallback to system app will be handled in Bug 1082443.

I will need to rebase on top of Bug 991970 once it lands, but I think this is ready for review now. Yoshi can you take a look at this?

Comment 31

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

Comment on attachment 8510450 [details] [diff] [review]
Implement-onpeerfound.patch

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

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

Original code is easier.

@@ +255,5 @@
> +    onPeerLost: function onPeerLost(sessionToken) {
> +      for (let target of this.eventTargets) {
> +        this.notifyDOMEvent(target, { event: NFC.NFC_PEER_EVENT_LOST,
> +                                      sessionToken: sessionToken });
> +      }

Merge these two functions.

::: dom/nfc/nsINfcContentHelper.idl
@@ +30,5 @@
> +   * Callback function used to notify peerfound.
> +   * @param sessionToken
> +   *        SessionToken received from Chrome process
> +   */
> +   void notifyPeerFound(in DOMString sessionToken);

refactor notifyPeerReady.

::: dom/nfc/nsNfc.js
@@ +253,5 @@
>      let event = new this._window.MozNFCPeerEvent("peerready", eventData);
>      this.__DOM_IMPL__.dispatchEvent(event);
>    },
>  
> +  notifyPeerFound: function notifyPeerFound(sessionToken) {

try to refine the original notifyPeerReady.

::: dom/webidl/MozNFC.webidl
@@ +76,5 @@
> +   /**
> +    * This event will be fired when NFCPeer is detected.
> +    */
> +   [CheckPermissions="nfc-write"]
> +   attribute EventHandler onpeerfound;

Should move to below of onpeerready/

Comment 32

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8510450 [details] [diff] [review]
Implement-onpeerfound.patch

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

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

Actually the original code does not work, because this.eventTargets is an array. Calling delete  sets this.eventTargets[index] to undefined. In onPeerFound/Lost I iterate over this array so I get TypeError in notifyDOMEvent later on.

Comment 33

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) Implement-onpeerfound.patch

Implemented changes requested in comment 31. peerfound/peerready are dispatched by the same function in nsNfc.js, I've renamed it to notifyPeerFound, since we should remove peerready in future.

Comment 34

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

Comment on attachment 8511732 [details] [diff] [review]
(v1.1) Implement-onpeerfound.patch

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

Thanks for the patch.
Please file another bug for adding test case.

Add r? to smaug for the WebIDL change in MozNFC.webidl

::: dom/nfc/NfcContentHelper.js
@@ +408,5 @@
>          break;
>        case "NFC:DOMEvent":
>          switch (result.event) {
>            case NFC.NFC_PEER_EVENT_READY:
> +            this.eventTarget.notifyPeerFound(result.sessionToken, true);

notifyPeerFound(result.sessionToken, /* isPeerReady */ true);

::: dom/webidl/MozNFC.webidl
@@ +71,5 @@
>     [CheckPermissions="nfc-write"]
>     attribute EventHandler onpeerready;
> +
> +   /**
> +    * This event will be fired when NFCPeer is detected.

when a NFCPeer

@@ +77,5 @@
> +   [CheckPermissions="nfc-write"]
> +   attribute EventHandler onpeerfound;
> +
> +   /**
> +    * This event will be fired when NFCPeer, earlier detected by the device, moves

earlies detected in onpeerready or onpeerfound ...

Comment 35

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

Comment on attachment 8511732 [details] [diff] [review]
(v1.1) Implement-onpeerfound.patch

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

Add r? to smaug for the WebIDL change in MozNFC.webidl

Comment 36

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

Comment on attachment 8511732 [details] [diff] [review]
(v1.1) Implement-onpeerfound.patch

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

Add r? to smaug for the WebIDL change in MozNFC.webidl

::: dom/webidl/MozNFC.webidl
@@ +84,1 @@
>     [CheckPermissions="nfc-write"]

Dimi mentioned why peerlost needs nfc-write permission?
maybe we should remove nfc-write here.

Comment 37

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.2) Implement-onpeerfound.patch

Implemented changes pointed out by Yoshi in comment 34 and comment 36.

Hi Olli, can you review the WebIDL change in MozNFC.webidl?

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8511874 [details] [diff] [review]
(v1.2) Implement-onpeerfound.patch

>+      if (index !== -1) {
>+        this.eventTargets.splice(index,1);
Nit, space after ','

>+   /**
>+    * This event will be fired when a NFCPeer is detected.
>+    */
>    [CheckPermissions="nfc-write"]
>+   attribute EventHandler onpeerfound;
>+
>+   /**
>+    * This event will be fired when NFCPeer, earlier detected in onpeerready
>+    * or onpeerfound, moves out of range.
>+    */
>    attribute EventHandler onpeerlost;
Why different permissions?
If one can't use onpeerfound, one shouldn't be able to use onpeerlost either.
So please use the same permissions for consistency.
(I see this was discussed early.)

Do we check permissions before dispatching these events? Since onfoo properties are just EventHandlers, and one can
always use addEventListener();
I don't see such permission check before dispatching the event. I think there should be (if we have permission check for onfoo properties)
Because of that, r-.


(I noticed now that onpeerready has already similar issue, but it could be fixed simultaneously.)

Comment 39

[Krzysztof Mioduszewski[:tauzen]]

Thanks for pointing this out. I can see that Yoshi already added |checkPermissions| method in his patch for ontagfound/lost (Bug 991970). I'll wait once it gets landed then I will rebase on top of it and address your comments in the next version of the patch.

Comment 40

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.3) Implement-onpeerfound.patch

Fixed all issues pointed out in comment 38, that is:
- missing space added in|this.eventTargets.splice(index, 1)|
- onpeerready/onpeerfound and onpeerlost have all |nfc-write| permission
- permission checks for onpeerready/onpeerfound and onpeerlost are also done before dispatching the event

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8514108 [details] [diff] [review]
(v1.3) Implement-onpeerfound.patch

r+ for the webidl change.

Comment 42

[Krzysztof Mioduszewski[:tauzen]]

try results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84dfa8d3b5de
NFC changes r+ by Yoshi in comment 34

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bff2bc353e7