963541 - B2G NFC: remove NFCTag.connect and NFCTag.close

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

We need to have a clear definition of what does NFCTag.connect() and NFCTag.close() should do precisely.

What does connect/close do? and what happened if we don't call them correctly.
And should we really need them ?

connect() maybe still needed as it is used to switch RF interface.

But what about close? should we rename it to disconnect() ?
If the app calls NFCTag.close(), actually NFC library will close the physical location to that tag, however since Tag is still in proximity another tech-discovered will be fired immediately, this will cause a bad user experience.

And what will happen if we calls two connect()? 
like :

tag.connect(NFC_A); // didn't call close before calling next connect()
tag.connect(NFC_B);

Comment 1

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

(In reply to Yoshi Huang[:allstars.chh] from comment #0)
> What does connect/close do? and what happened if we don't call them
> correctly.
> And should we really need them ?
> 
> connect() maybe still needed as it is used to switch RF interface.
> 
connect is used to switch to IsoDep or Frame RF interfaces.

Currently we don't support IsoDep yet. See Bug 916428

> But what about close? should we rename it to disconnect() ?

From Android  
http://developer.android.com/reference/android/nfc/tech/TagTechnology.html#close()
This function actually calls NfcService.reconnect();

So back to our WebNFC API:
Should we rename close to reconnect?
And under what circumstance we will need this 'reconnect' ?

> And what will happen if we calls two connect()? 
> like :
> 
> tag.connect(NFC_A); // didn't call close before calling next connect()
> tag.connect(NFC_B);

In the example above, no error should be thrown, as NFC_A and NFC_B use the same Frame RF interface.

However what should happen if 
tag.connect(NFC_A);
tag.connect(ISO_DEP);

Should the second connect could switch to IsoDep RF interface successfully? or it should throw because not calling close() before ?

Comment 2

[Garner Lee]

I think it makes sense to track (re)connect/close() states, and throw an JS exception if it isn't closed before switching technologies so it can potentially also properly finish whatever it was doing. It may also encourage proper application cleanup (close ICC channels, etc.), for future feature development.

This happen whether or not the application remembers whether it is connected or not.

Comment 3

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

(In reply to Garner Lee from comment #2)
> I think it makes sense to track (re)connect/close() states, 

I think you mean 'connect/close(reconnect)' 

> and throw an JS
> exception if it isn't closed before switching technologies

Yeah, again, define 'close'.
I think you're still talking 'reconnect' from my previous comment.
However I agree that we need to throw error at this case.

We need some TagTechnology support first, like NfcA or IsoDep(Bug 916428).
But before they are supported, I think NFCTag.connect/close are not neccesary.

Comment 4

[Ed Morley [:emorley]]

Bug 1043889 is unrelated to this - typo?

Comment 5

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

yeah, typo. sorry for my fat finger. :P

Comment 6

[Ed Morley [:emorley]]

Np :-)

Comment 7

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

These functions are actually useless now, since when the tag is discovered nfcd already connects to it.

So in this bug I'll remove these two interfaces(connect/close) from MozNFCTag.webidl, but still keep the internal impl because we might still need this when we support multi-protocol tags.

Comment 8

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

NFCTechType will still be used in Bug 1057264, so I don't remove them for now.

Comment 9

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

Attachment: Patch

Comment 10

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

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

Hi, Smaug
Jonas has asked the purpose of these two interfaces earlier this year.
These two interfaces are added when WebNFC firstly landed Bug 674741, and actually it uses the same interface from Android. See [1]

After investigation we found that these interfaces are used to switch RF interface for different Technologies.

However right now we didn't support any NFC Tag Technology yet, and nfc daemon already doing the RF-connect when the tag is discovered.(Like when a MozNFCPeer is discoverd, nfcd will connect to it by itself). So actully the two interaces are dummy, so we propose to remove these two.


[1]: http://developer.android.com/reference/android/nfc/tech/TagTechnology.html#connect()

Comment 11

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

Attachment: Patch

Add r=smaug, dimi

Comment 12

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

add checkin-needed for b2g-inbound is close.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/8af301918828