Krzysztof Mioduszewski[:tauzen] Assignee
	Description • 10 years ago

This is a followup to the discussion in Bug 1039239. 
Currently getNFCPeer can throw an error when the session token passed as an argument is not valid. This can happen when other device is quickly moved away after the onpeerready callback was fired. Error throwing was introduced in this Bug 976402. Gaia apps which are using sharing (contacts, browser, gallery, music) do not have a try-catch block and are handling getNFCPeer inconsistently. 

In this bug we should discuss how to fix getNFCPeer API. Currently there are two propositions:
1. Removing the error throwing in getNFCPeer and return null instead - quicker needs small changes in gecko, needs small changes in current gaia apps.
2. Remove getNFCPeer and reimplement onpeerready to be compatible with W3C onpeerfound[1]. This would require more effort, but would simplify the API usage considerably and solve our problem in an elegant way (rejected promise when the other device is out of reach). Also we would not expose the session token to app developer. 

http://www.w3.org/TR/nfc/#widl-NFCManager-onpeerfound

	arno
	Comment 1 • 10 years ago

I suggest to make this bug more generic: getNFCTag() also throws an error in an error case.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 2 • 10 years ago

Yoshi is currently reimplementing onpeerready handling in Bug 1046554, after this change it will be possible to obtain MozNFCPeer directly in the callback. Once he'll land this we will need to:
1. Fix gaia apps (contacts, gallery, music, system browser) to not use getNFCPeer
2. Remove getNFCPeer completely

This will solve the initial problem. I'm renaming the bug so the title will be more appropriate.

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

The problem is in nfc_handover_manager.js still uses getNFCPeer to doing handover,
which I think it's still a long way to go to completely remove getNFCPeer.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 4 • 10 years ago

Right, I didn't think this all through, sorry for that. As Yoshi pointed out we still need to figure out how to handle this in NfcHandoverManager which is not listening for onpeerready.

Additionally, Sid performed a couple of tests and it's not possible to return null or undefined in getNFCPeer as it results in the following error:
>JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object.

Should getNFCPeer return a DOMRequest or a Promise then?

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 5 • 10 years ago

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #4)
> Additionally, Sid performed a couple of tests and it's not possible to
> return null or undefined in getNFCPeer as it results in the following error:
> >JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object.

Where does this error come from? Isn't it because the user of mozNFC.getNFCPeer expect it is an object and use it right away?

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 6 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> Where does this error come from? Isn't it because the user of
> mozNFC.getNFCPeer expect it is an object and use it right away?
The app does not use it right away, but the error is visible.

This is a more detailed log from a test which I've done:
>I/Gecko   ( 3391): -*- NfcContentHelper: Message received: {"target":{},"name":"NFC:DOMEvent","sync":false,"json":{"event":1,"sessionToken":"{c9778234-d3af-4fc7-a0c6-471013d66b05}","data":null},"data":{"event":1,"sessionToken":"{c9778234-d3af-4fc7-a0c6-471013d66b05}","data":null},"objects":{}}
>E/GeckoConsole( 3391): [JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object."]
>I/Gonk    ( 3391): Setting nice for pid 3552 to 1
>I/Gonk    ( 3391): Changed nice for pid 3552 from 18 to 1.
>E/GeckoConsole( 3391): Content JS LOG at app://browser.gaiamobile.org/js/nfc.js:69 in nfc_handlePeerConnectvity: Browser: trying to getNFCPeer

I've checked Browser app and added there additional logging before and after call to MozNFC.getNFCPeer. It just retrieves MozNFCPeer. I'm not sure why "Return value of MozNFC.getNFCPeer is not an object." is printed out first, but no log after call to getNFCPeer is visible.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 7 • 10 years ago

Ok, I've modified MozNFC.webidl, so MozNFCPeer can be nullable. It's possible to remove error throwing and just return null. I'll prepare a patch with some tests and ask for a review.

	psiddh
	Comment 8 • 10 years ago

This is nice. We can then remove the 'throwing of exception' from webidl

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 9 • 10 years ago

Hi Yoshi, can you review this? Should I ask somebody for the feedback on WebIDL changes?

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

I will ask for review if Gecko part will get r+.

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

Comment on attachment 8466074 [details] [diff] [review]
Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable

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

::: dom/nfc/nsNfc.js
@@ +195,5 @@
>      obj.initialize(this._window, sessionToken);
>      if (this._nfcContentHelper.setSessionToken(sessionToken)) {
>        return this._window.MozNFCPeer._create(this._window, obj);
>      }
> +    return null;

rebase here.

::: dom/nfc/tests/marionette/test_nfc_peer.js
@@ +91,1 @@
>    }

shouldn't use try-catch anymore.

@@ +102,5 @@
> +    let tag = nfc.getNFCTag("fakeSessionToken");
> +    is(tag, null, "NFCTag should be null on wrong session token");
> +  } catch (ex) {
> +    ok(false, "Exception should not be thrown");
> +  }

ditto

::: dom/webidl/MozNFC.webidl
@@ +45,5 @@
>  [JSImplementation="@mozilla.org/navigatorNfc;1",
>   NavigatorProperty="mozNfc",
>   Func="Navigator::HasNFCSupport"]
>  interface MozNFC : EventTarget {
> +   MozNFCTag? getNFCTag(DOMString sessionId);

Should add comments on this.
Also rename sessionId to session.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 12 • 10 years ago

Hi Yoshi, thank you for the review. I've addressed all your comments. Could you take a look on the new version?

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

Comment on attachment 8467714 [details] [diff] [review]
Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable, v 1.1

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

looks good, thank you.

Forward r? to smaug for the WebIDL change.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 14 • 10 years ago

Comment on attachment 8466983 [details]
Gaia changes, NfcHandoverManger try-catch removal

Hi Alive, since Gecko changes are r+ could you take a look at NfcHandoverManager patch? Try-catch was removed as you requested and I've updated the test cases.

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 15 • 10 years ago

Hi Yoshi could you push the gecko part for try build?

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

If you have level 1 access you can do it by yourself, other you need to get Level 1 access first.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 17 • 10 years ago

Comment on attachment 8466983 [details]
Gaia changes, NfcHandoverManger try-catch removal

Nice.

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

rebase

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

b2g-inbound is close now, add checkin-needed

	Krzysztof Mioduszewski[:tauzen] Assignee
	Comment 20 • 10 years ago

Important note, Gecko and Gaia part need to land together.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

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

Master: https://github.com/mozilla-b2g/gaia/commit/09365cfc38e1381c81a0a86b4ef22bcd16babe98

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 10 years ago

Comment on attachment 8468311 [details] [diff] [review]
Gecko Patch. v1.2

v1.2 was rebased on top of bug 1046554, which was backed out on m-c. I backed out v1.2, merged m-c to b-i, then re-landed the v1.1 patch here.

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 10 years ago

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