If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

B2G NFC: remove NFCTag.connect and NFCTag.close

RESOLVED FIXED in 2.1 S3 (29aug)

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

(Blocks: 1 bug)

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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);
(Assignee)

Updated

4 years ago
Blocks: 948721
(Assignee)

Updated

4 years ago
Assignee: nobody → allstars.chh
(Assignee)

Comment 1

4 years ago
(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

4 years ago
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.
(Assignee)

Comment 3

4 years ago
(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.
Depends on: 916428
(Assignee)

Updated

4 years ago
Depends on: 976457

Updated

4 years ago
Blocks: 978707

Updated

4 years ago
Blocks: 949293

Updated

4 years ago
No longer blocks: 949293
blocking-b2g: --- → backlog
(Assignee)

Updated

3 years ago
Assignee: allstars.chh → allstars.chh
(Assignee)

Updated

3 years ago
Assignee: allstars.chh → allstars.chh
(Assignee)

Updated

3 years ago
No longer depends on: 916428
(Assignee)

Updated

3 years ago
Blocks: 976457
No longer depends on: 976457

Comment 4

3 years ago
Bug 1043889 is unrelated to this - typo?
No longer blocks: 1043889
(Assignee)

Comment 5

3 years ago
yeah, typo. sorry for my fat finger. :P
Blocks: 1043899

Comment 6

3 years ago
Np :-)
(Assignee)

Updated

3 years ago
Blocks: 1042851
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Updated

3 years ago
Summary: B2G NFC: Have a clear definition with NFCTag.connect and NFCTag.close → B2G NFC: remove NFCTag.connect and NFCTag.close
(Assignee)

Comment 8

3 years ago
NFCTechType will still be used in Bug 1057264, so I don't remove them for now.
(Assignee)

Comment 9

3 years ago
Created attachment 8477275 [details] [diff] [review]
Patch
Attachment #8477275 - Flags: review?(dlee)
Attachment #8477275 - Flags: review?(dlee) → review+
(Assignee)

Comment 10

3 years ago
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()
Attachment #8477275 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8477275 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8479842 [details] [diff] [review]
Patch

Add r=smaug, dimi
Attachment #8477275 - Attachment is obsolete: true
Attachment #8479842 - Flags: review+
(Assignee)

Comment 12

3 years ago
add checkin-needed for b2g-inbound is close.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 964194
https://hg.mozilla.org/integration/b2g-inbound/rev/8af301918828
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8af301918828
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
(Assignee)

Updated

3 years ago
No longer blocks: 1042851
(Assignee)

Updated

3 years ago
Blocks: 1042851
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.