Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Description • 11 years ago

Jonas mentioned this during WebAPI review.

We should find a more consistent way to dispatch NFCTag or NFCPeer to the web, instead of exposing sessionToken and use mozNfc.getTag(sessionToken) or mozNfc.getPeer(sessionToken) to get the object.

Also we have to consider about in ndef-tech-discovered, it seems we cannot pass NFCTag in that system message (Structure Clone).
In that case, how should we pass NFCTag ?

	Ken Chang[:kenkai]
	Comment 1 • 11 years ago

Garner, what do you think?

	Garner Lee
	Comment 2 • 11 years ago

It may be possible to remove the session token. If we can ensure only a single instance of an application can be launched at once (manifest URL), the token may be hidden from the application. The token will need to be stored in the parent process, where it is already checked for validity currently.

	Garner Lee
	Comment 3 • 11 years ago

Sid, please look into whether the current framework for P2P can be extended to handle parent/content app exchange of sessionTokens.

	psiddh Assignee
	Comment 4 • 11 years ago

Yes, I think we can leverage the existing P2P framework and do away with exposing 'sessoinTokens'

	Garner Lee
	Comment 5 • 11 years ago

Note from a different discussion: For Activities, where the session tokens for tags arrive, we'd need a new/modified message type to route the NFC object to the W3C compiliant "ontechdiscovered" (create interface object at the application level with the internal sessionToken).

	psiddh Assignee
	Comment 10 • 11 years ago

With the above attachments / changes, tested the following scenarios:-
1) Tag Usecases - Reading / Writing. Validated most of the APIs in the interface 'MozNFCTag'.
2) P2P Usecases - Tested onpeerfound & onpeerlost handlers. Validated all the APIs of the interface 'MozNFCPeer'
3) Tested scenarios when the Nfc gaia application is killed
4) Validated P2P sharing scenario (of URL) from browser.
5) Validated P2P sendFile scenario partly (as there is a regression caused by earlier changes)

I have also attached Gaia patches for everyone's review.
Part3 : (NFC) System App changes - I will issue a pull request for Alive. But attaching a patch for now
Part 4: Gaia App changes to use NFC peer API ( getNFCPeer() )without 'session' + 1 test case is also fixed in contacts app.

Once the patches get reviewed / landed, I will fix the Nfc wiki as well accordingly

	psiddh Assignee
	Comment 11 • 11 years ago

For Gaia apps changes (Part 4), do I have to create separate patches per application (Browser, Contacts, Gallery etc..)(or) could it be accepted as one single patch with a reviewer. I will accordingly create a pull request for this change. Ken , can you please suggest ?

	Ken Chang[:kenkai]
	Comment 12 • 11 years ago

(In reply to Siddartha P from comment #11)
> For Gaia apps changes (Part 4), do I have to create separate patches per
> application (Browser, Contacts, Gallery etc..
Yes, this way would be easier for reviewer to do a review. Thanks.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 14 • 11 years ago

Comment on attachment 8403001 [details] [diff] [review]
Part 1: v1 - Do not expose session tokens to content process. r=allstars.chh

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

Can't this have test cases?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 11 years ago

Comment on attachment 8402146 [details] [diff] [review]
Part 2: v1 - Remove the usage of session from Nfc APIs getNFCTag() & getNFCPeer(). r=khuey, sr=smaug

>--- a/dom/webidl/MozNfc.webidl
>+++ b/dom/webidl/MozNfc.webidl
>@@ -26,18 +26,18 @@ interface MozNfcManager {
>     */
>    void notifySendFileStatus(octet status, DOMString requestId);
> };
> 
> [JSImplementation="@mozilla.org/navigatorNfc;1",
>  NavigatorProperty="mozNfc",
>  Func="Navigator::HasNfcSupport"]
> interface MozNfc : EventTarget {
>-   MozNFCTag getNFCTag(DOMString sessionId);
>-   MozNFCPeer getNFCPeer(DOMString sessionId);
>+   MozNFCTag getNFCTag();
>+   MozNFCPeer getNFCPeer();
So how is this API supposed to work? When can one call getNFCTag() and will it
return the same object if re-called right after? And what about calling it later?

	psiddh Assignee
	Comment 16 • 11 years ago

This API getNFCTag() is supposed to be called when there is an active Nfc Tag (passive tag) in phone's near field. 

To give the context, When a tag is brought in close proximity to the FxOS phone, nfcd (Gonk) receives the event (with the data). nfcd relays this info to Gecko (Nfc.js - Parent Process) which in turn notifies System app about 'nfc-manager-tech-discovered' event. System manager through mozActivity forwards the event to selected application by the user.
nfcd-->Nfc.js-->System App (Moz Activity) -->Nfc App1-->getNFCTag()

Currently it will not return the same object when called say later. (Should a singleton like object be enforced?)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

So how does the system app notify some selected application?
Given that MozNfc is an EventTarget couldn't we dispatch an Event there which has .nfcTag or some such
as a readonly property?

	psiddh Assignee
	Comment 18 • 11 years ago

Gaia (nfc) Applications declare/register 'nfc-ndef-discovered' in their manifest.webapp. When the system app receives the event from lower layers (Nfc.js), it sends the mozActivity. If there are more than one registered application that declared 'nfc-ndef-discovered', a picker would be shown allowing user to make a selection. After user makes a selection, the application selected will be launched (if not launched already) or brought into foreground (if it is in background)

This is what Gaia applications can do currently..

After registering in their manifest.webapp,
navigator.mozSetMessageHandler('activity', NfcActivityHandler);
.
.
function NfcActivityHandler(activity) {
  var activityName = activity.source.name;
  var data = activity.source.data;
  switch (activityName) {
    case 'nfc-tech-discovered':
        var nfcdom = window.navigator.mozNfc;
        var nfcTag = nfcdom.getNFCTag(); 
        var readreq = nfcTag.readNDEF();  
        .
        .
  }

If I understand your suggestion correctly, this is what you propose. Pls correct me otherwise

var nfcdom = window.navigator.mozNfc;
nfcdom.onnfctagfound = function(event) {   // 'onnfctagfound' say, is new dom event handler
  var tag = nfcdom.nfcTag;   // readonly attr
         (or)
  var tag = event.details; 
  var readreq = tag.readNDEF();
}

If we do adopt the notion of dispatching the event, we may lose out on showing a picker (MozActivity)
and allowing user to make a choice. Also for the dom event to be dispatched, the application needs to be launched / or running in background.
With the picker approach an application can be launched upon user selection

Garner has raised this bug recently :https://bugzilla.mozilla.org/show_bug.cgi?id=991970
Ideally, getNFCTag() and getNFCPeer() should be removed in future.

So yes, your suggestion is a good idea to get rid of getNFC* interfaces.
Do you want me to raise separate bug to discuss the changes suggested by you , the user scenarios and its implication?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 11 years ago

I guess in my mind nfcactivityhandler should be an event listener and an event would be dispatch
when there is nfc available.
But I'm not familiar with our activity API, so it depends on whether that activity stuff could
use normal event handling.


Could we even keep the current activity stuff (which is just giving us a 'nfc-tech-discovered' notification?) and add a new event which would be dispatched to mozNfc.
The event would be dispatched after the notification.

	psiddh Assignee
	Comment 20 • 11 years ago

I think , Activities are a kind of system messages internally.

> Could we even keep the current activity stuff (which is just giving us a 'nfc-tech-discovered' 
> notification?) and add a new event which would be dispatched to mozNfc.
> The event would be dispatched after the notification.

I think, for Gaia developers, this may not be intuitive (Please correct me if I have not understood your point here. Maybe I am missing your point.) as they may expect some data / info as part of activityHandler. 
Nfc Gaia apps will have to specify an entry in their manifest webapp to receive activity messages + register for an event (dom call) for example 'onnfctagfound'. There needs to be a glue in lower layers to bind these two messaging ways (Activities are System messages & EventHandlers are dom way) to ensure that there are no race conditions and ensure that dom event call does not happen before activity is launched. There needs to be some co-ordination in place. I think with the current design of Nfc this may be cumbersome. What do you think?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 11 years ago

Comment on attachment 8402146 [details] [diff] [review]
Part 2: v1 - Remove the usage of session from Nfc APIs getNFCTag() & getNFCPeer(). r=khuey, sr=smaug

It looks like Olli is handling this.  If I'm wrong please rerequest review.

	Garner Lee
	Comment 23 • 10 years ago

Hi, what's the status on this bug?

	iacobs
	Comment 24 • 10 years ago

just a heads up: NFC functionality has been broken in master and 2.1 ever since since bug 1069188 landed: getNfcTag expects a session token, but it is no longer being provided via nfc-ndef-discovered, so getting the tag object is impossible right now.

	psiddh Assignee
	Comment 25 • 10 years ago

Hi  Krzysztof,

Could you please respond to the concern raised in the comment#24 please, since you have dealing with these changes now ?

Thanks

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 26 • 10 years ago

getNFCTag should be only used by System app.
To get a tag object should be done in Bug 991970.

	Krzysztof Mioduszewski[:tauzen]
	Comment 27 • 10 years ago

(In reply to iacobs from comment #24)
> just a heads up: NFC functionality has been broken in master and 2.1 ever
> since since bug 1069188 landed: getNfcTag expects a session token, but it is
> no longer being provided via nfc-ndef-discovered, so getting the tag object
> is impossible right now.
Bug 1069188 has landed only on master, there were no changes introduced in 2.1. I'm not sure if you have any particular use case in mind, but unless you're not planning to write something onto NFC tag you don't need MozNFCTag object for now. Whole contents of a valid NDEF Message is attached to nfc-ndef-discovered (or different nfc originated activity), see here [1]. BTW, If you do plan to write something on NFC tag you're app needs to be certified. 

[1] - https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L405

	iacobs
	Comment 28 • 10 years ago

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #27)
> Bug 1069188 has landed only on master, there were no changes introduced in
> 2.1. 

Mkay, I'll rebuild for 2.1 then and check why it doesn't work then; I remember moving to master because the thing stopped working at some point.

> I'm not sure if you have any particular use case in mind, but unless
> you're not planning to write something onto NFC tag you don't need MozNFCTag
> object for now. 

Well, tags are blank when you first get them (I got some of these: http://www.rfidshop.com/45mm-label-tag-mifare-ultralightnfc-393-p.asp - they are NDEF formatable); my app needs to put some NDEF info into them (basically an UUID), so it can retrieve it later.

> Whole contents of a valid NDEF Message is attached to
> nfc-ndef-discovered (or different nfc originated activity), see here [1].

I know; however, I still need to write, so...

> BTW, If you do plan to write something on NFC tag you're app needs to be
> certified. 

The app needs to be certified for any kind of NFC access from what I could see back when I wrote it, even for activities, so I wasn't hoping I'd ever put it in the app store anyway (I'll refrain from commenting on this because I'm not familiar with the thinking process that lead to it and I'd come off as abrasive / impolite).

Can't wait for proper events, to be honest (the ontagfound / ontaglost thing mentioned above), the current ways are beyond clunky...

	Garner Lee
	Comment 29 • 10 years ago

I haven't been checking tags recently (the main focus turned to P2P user stories that don't involve tags since that was generating the most interest) but there should be some kind of "empty" NDEF message generated by NFC manager if it finds a completely empty, NDEF-compatible tag. Create a message handler for that in the (certified) app to get the empty tag as well as the specific self-selected mime-type filter for your app. Any ambiguities will trigger a application picker, so be careful with empty.

There are bugs associated with making the APIs more W3C complaint for the purposes of privilaged permission usage (marketplace). It's been a bit of an uphill battle to find the best way to resolve for the available platform APIs for processes mapping to Web APIs (for apps). Bug 991970, parent is Bug 1042851.

	iacobs
	Comment 30 • 10 years ago

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L175

nfc-ndef-discovered is only fired when something that has NDEF information in it, which kinda makes sense; my app associates photos with tags (the photo is named with the tag id and is recalled next time you scan), and when formatting it just writes a text NDEF message with UUID4 in the id field and empty payload (I think there is only room for 5 or 6 characters on my 512bit tags, after the UUID and the NDEF overhead - they are made for things like smart posters).

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 31 • 10 years ago

Now we will implement ontagfound/onpeerfound, and in these callbacks we will provide MozNFCTag and MozNFCPeer so sessionToken can be removed.
I'll make this bug WONTFIX, also remove the blocking of Bug 991970.