[NFC] APIs for ISO 14443-4 tags (IsoDep Support)

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: xju-hashimoto, Assigned: dimi)

Tracking

unspecified
2.2 S4 (23jan)
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(tracking-b2g:backlog, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [p=5])

Attachments

(7 attachments, 7 obsolete attachments)

3.66 KB, text/plain
Details
8.47 KB, patch
Details | Diff | Splinter Review
11.82 KB, patch
Details | Diff | Splinter Review
349 bytes, patch
Details | Diff | Splinter Review
7.70 KB, patch
Details | Diff | Splinter Review
102.12 KB, image/png
Details
11.85 KB, patch
dimi
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.65 Safari/537.36

Steps to reproduce:

Touching to a ISO14443-4 tag causes nothing.


Actual results:

Touching to a ISO14443-4 tag causes nothing.


Expected results:

FirefoxOS should support various NFC tags including IS014443-4 and provide a developer friendly interface for this purpose.
Blocks: b2g-nfc
Summary: NFC APIs for various tags → NFC APIs for ISO 14443-4 tags
(Reporter)

Updated

6 years ago
Blocks: 911905
No longer blocks: b2g-nfc
Hi Hashimoto-san
We use Bug 860906 to track all B2G-NFC bugs, so I will add the dependency again here
Blocks: b2g-nfc
(Reporter)

Updated

5 years ago
Group: kddi-confidential
(Reporter)

Updated

5 years ago
Summary: NFC APIs for ISO 14443-4 tags → [Madai][NFC] APIs for ISO 14443-4 tags (IsoDep Support)
(Reporter)

Updated

5 years ago
Group: kddi-confidential
Summary: [Madai][NFC] APIs for ISO 14443-4 tags (IsoDep Support) → [NFC] APIs for ISO 14443-4 tags (IsoDep Support)
(Reporter)

Comment 4

5 years ago
A diff of MozNFCTag.webidl from gecko.patch
It simply add transceive() method to MozNFCTag.

Sid, could you give me any feedback?
Flags: needinfo?(psiddh)

Comment 5

5 years ago
Posted patch IsoDEP.patch (obsolete) — Splinter Review
In order to add IsoDep (or any other future technology classes)to the master and ensuring that current NDEF functionality also stays in tact, I have attached a rough patch to convey my thoughts (This patch is not compiled / tested against). I will do it shortly and submit another cleaner patch. Following things were done
1) First rename current MozNFCTag to 'MozNDEFTag' (Since the current MozNFCTag is pretty much NDEF functionality only) and commented out 'connect' & 'close'.
2) Then I copied your base interface 'nsIDOMNfcTag' from NfcTag.idl and named it MozNFCTag (with no interface . yes this can be a base class)
3) I derived 'MozNDEFTag' from 'MozNFCTag'.
4) Extended getNFCTag function in nsNfc.js to accept an optional parameter 'techType'. (Default is 'NDEF'.)
i;e; getNFCTag(sessionToken) --> will return NDEF object,
but say getNFCTag(sessionToken, 'ISO_DEP') should return IsoDep object 
This way we can extend into supporting new technologies and at the same time we can maintain the existing nomenclature.
What do you think? Also I will test out the patches (fix bugs if there are any) and submit a cleaner patch for yours & Mozilla's feedback as soon as I can.,
Flags: needinfo?(psiddh)
(Reporter)

Comment 6

5 years ago
Posted image NFCTag class diagram (obsolete) —
The attach is a diagram based on your suggestion (with moving transceive to IsoDepTag)
About getDetailsNDEF():
	(1) Can we use NFCTag.getDetails(tech) instead?
	(2) Does NFCTag.getDetails(tech) meet a use case where developer wants to access details *before* deciding which technology to connect?
About connect(), getNFCTag():
	(3) What if they are called more than twice? Should all previous tag-connections are canceled? (rel. bug963541)

Takagi-san, could you show your view especially on (2).
Flags: needinfo?(yh-takagi)

Comment 7

5 years ago
About (1), 
    We can use NFCTag.getDetails() instead of getDetailsNDEF(), but a parameter 'tech' isn't necessary since you have already specified a tech-type by 'getNFCTag(sessionId, tech).

About (2),
    A developer can access details *after" getNFCTag() and *before* connect(). I think it is reasonable because a developer should determine a tech-type before accessing details. 

    Before that, developer wants to filter types, and know which types are supported by a discovered tag.

    Here is a rough idea of manifest.webapp for filtering nfc-tech-type.
      "activities": {
          "nfc-technology": {
              "filters": {
                  "type": [ "NDEF", "IsoDep", ... ]
              },
          "disposition": "window"
          }
      }

    In addition to that, an app needs to receive a nfc-tech-list by an activity handler.

About (3),
    When nfc-tech-discovered is occured, Gaia has already know all tech-types supported by a discovered tag (otherwize filtering is impossible).
    If Gaia can keeps tech-list and details of a tag, getNFCTag() (and getDetails()) for different types can be called in parallel. But 'connect()' shall be called exclusively.
Flags: needinfo?(yh-takagi)

Comment 8

5 years ago
Posted patch IsoDep_v01.patch (obsolete) — Splinter Review
Attaching the patch that refactors existing MozNFCTag interface to support addition of new technology types such as 'ISO_DEP' or 'NFC_A' etc
Attachment #8394542 - Attachment is obsolete: true

Comment 9

5 years ago
I concur with Yoshihiko TAKAGI-san mostly.

Yes after the call, 
ndef = getNFCTag(session, 'NDEF');
.
.
ndef.getDetails();   // No need of 'getDetailsNdef()'
.
ndef.connect();      // connect doesn't require 'tech' field


nfc-tech-list is already received at System app level.

Yes agreed that we should now have a way to allow Gaia apps to filter on tech types, now that we are supporting multiple techs. Maybe we should stage this changes in two phases, one for basic refactoring of MozNFCTag ( on the lines of attached patch) and other patch to support providing filters.

Yoshi, what is your view on the reafactoring part ?

Comment 10

5 years ago
Requesting Yoshi's inputs on the refactoring of webIDLs to support new tech types.
Flags: needinfo?(allstars.chh)

Comment 11

5 years ago
Attachment #8396152 - Attachment is obsolete: true
(Reporter)

Comment 12

5 years ago
Summary
- Common function
-- getNFCTag() modification (let's use this bug)
-- tech filtering (no bug?)
- various tags
-- MiFare https://bugzilla.mozilla.org/show_bug.cgi?id=884478
-- NFC_A https://bugzilla.mozilla.org/show_bug.cgi?id=976457
-- IsoDEP(this) https://bugzilla.mozilla.org/show_bug.cgi?id=916428
Attachment #8395339 - Attachment is obsolete: true
I'll intend to make things simpler, and I suggest we can split into some new bugs.

1. Keep MozNFCTag, please file a new bug for discussing whether we should have NDEFTag or not.
   Also given that NDEF is the format should be supported bt NFC-Forum. So I think it's okay for now we handle NDEF in MozNFCTag.

2. File a new bug for discussing rename getDetailsNDEF to getDetails. Given that right now it returns some attribute from NDEF, and not from other technology.

At this bug, we should discuss when should we get the IsoDep object first?
in mozNfc.getNFCTag()? or in MozNFCTag.connect() ?

If we plan to put 'connect' in MozNFCTag (the base class), I think we need to have the techType or tech as the parameter, i.e. connect("IsoDep");

In Bug 963541 it's already being discussed for the 'connect',
Only IsoDep tags need to switch RF interface.
Should we use FRAME as default so 'connect' can be moved to IsoDep class?

Also if we plan to get the IsoDep object in getNFCTag, are connect/close still neccesary?
Flags: needinfo?(allstars.chh)
I think Junichi-san is taking this bug, assign assignee to him.
Assignee: nobody → xju-hashimoto
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 15

5 years ago
A metabug such as 'Support various NFC tag technologies" would be required for 1 and 2. Does it exists?
---

The idea of connect()/close() interfaces come from android[1].
If we plan to provide unique API for firefox OS, connect()/close() can be omitted.
Then pseudocode will be:

ndef = nfc.getNFCTag(session);
isodep.getDetails();
ndef.readNdef();
isodep = nfc.getNFCTag(session, "IsoDep"); // reconnect
isodep.getDetails();
isodep.transcieve(apdu);

[1] http://developer.android.com/reference/android/nfc/tech/TagTechnology.html
---
 
> In Bug 963541 it's already being discussed for the 'connect',
> Only IsoDep tags need to switch RF interface.
> Should we use FRAME as default so 'connect' can be moved to IsoDep class?

What do you mean by RF and FRAME?
(In reply to Junichi Hashimoto from comment #15)
> A metabug such as 'Support various NFC tag technologies" would be required
> for 1 and 2. Does it exists?
> ---
> 
No, but if we only see 2 bugs for now, it's okay to not to have a meta bug for it.
But it's up to you.
 
> What do you mean by RF and FRAME?
NCI spec, table 99 (p 140).

IsoDep tags will need to switch RF interface, that's what connet() is doing.
(Reporter)

Comment 17

5 years ago
I think I understand the needs of connect(). In order to make changes small it is better to keep connect() as is; with no parameters.

ndef = nfc.getNfcTag(session);           // returns MozNFCTag object (will be renamed as MozNDEFTag)
ndef.connect(/* nothing */);

iso = nfc.getNfcTag(session, "IsoDep");  // returns MozIsoDep object
iso.connect(/* nothing */);
blocking-b2g: --- → backlog
Duplicate of this bug: 911905
No longer blocks: 963541
Depends on: 963541
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1115605
(Assignee)

Comment 20

4 years ago
Hi Junichi-san
Since MozNFCTag.webidl has been changed, i will update new proposal.

Set assignee to me.
Assignee: xju-hashimoto → dlee
(Assignee)

Comment 21

4 years ago
This patch only supports transceive function, and this patch depends on bug 1109456
Attachment #8541631 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Depends on: 1109456
Comment on attachment 8541631 [details] [diff] [review]
WebIDL for ISO 14443_4 tags patch v1

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

::: dom/webidl/MozNFCTag.webidl
@@ +33,5 @@
>  };
>  
> +[Func="Navigator::HasNFCSupport", AvailableIn="PrivilegedApps",
> + Constructor(MozNFCTag tag)]
> +interface MozNFCIsoDep {

Rename to MozIsoDepTech

@@ +37,5 @@
> +interface MozNFCIsoDep {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */
> +  Promise<sequence<byte>> transceive(sequence<byte> command);

Use Uint8Array.
Attachment #8541631 - Flags: review?(allstars.chh)
(Assignee)

Comment 23

4 years ago
Posted patch WIP Patch (obsolete) — Splinter Review
Attachment #8541631 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Attachment #8545173 - Attachment is obsolete: true
Attachment #8546450 - Flags: review?(allstars.chh)
Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

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

r+ on JS impl.
WebIDL/C++ code needs to get review from smaug.

::: dom/nfc/MozIsoDepTech.cpp
@@ +1,1 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

tag-width

@@ +37,5 @@
> +/* static */
> +already_AddRefed<MozIsoDepTech>
> +MozIsoDepTech::Constructor(const GlobalObject& aGlobal,
> +                          MozNFCTag& aNFCTag,
> +                          ErrorResult& aRv)

align

@@ +58,5 @@
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<MozIsoDepTech> isodep = new MozIsoDepTech(win, aNFCTag);

nit, isoDep

@@ +92,5 @@
> +  return promise.forget();
> +}
> +
> +JSObject*
> +MozIsoDepTech::WrapObject(JSContext* aCx)

inline

::: dom/nfc/MozIsoDepTech.h
@@ +1,1 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

tag-width should be 80.

@@ +19,5 @@
> +
> +class Promise;
> +
> +class MozIsoDepTech : public nsISupports,
> +                     public nsWrapperCache

nit, align.

@@ +32,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  static
> +  already_AddRefed<MozIsoDepTech> Constructor(const GlobalObject& aGlobal,

usually 
static already_AddRefed<MozIsoDepTech>
Constructor(..)

@@ +34,5 @@
> +
> +  static
> +  already_AddRefed<MozIsoDepTech> Constructor(const GlobalObject& aGlobal,
> +                                             MozNFCTag& aNFCTag,
> +                                             ErrorResult& aRv);

align

@@ +41,5 @@
> +  MozIsoDepTech(nsPIDOMWindow* aWindow, MozNFCTag& aNFCTag);
> +  ~MozIsoDepTech();
> +
> +  nsRefPtr<nsPIDOMWindow> mWindow;
> +  nsRefPtr<MozNFCTag> mNFCTag;

should be nsCOMPtr?
call it 'mTag' should be enough

::: dom/webidl/MozIsoDepTech.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * Part of this idl is from:
> + * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface

remove

@@ +4,5 @@
> + *
> + * Part of this idl is from:
> + * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface
> + *
> + * Copyright © 2013 Deutsche Telekom, Inc.

You changed career?

@@ +12,5 @@
> + ChromeConstructor(MozNFCTag tag)]
> +interface MozIsoDepTech {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */

Throws

perhaps MozNFCTag.transceive doesn't have to do [Throws].
Attachment #8546450 - Flags: review?(allstars.chh) → review+
Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

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

::: dom/nfc/MozIsoDepTech.cpp
@@ +92,5 @@
> +  return promise.forget();
> +}
> +
> +JSObject*
> +MozIsoDepTech::WrapObject(JSContext* aCx)

ingore this. I found many code also put it in cpp.
(Assignee)

Comment 27

4 years ago
Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

r? to smaug because of add MozIsoDepTech.webIdl and modify MozNFCTag.webidl

I will address yoshi's comment in next patch.
Attachment #8546450 - Flags: review?(bugs)
At some point we need to get rid of Moz prefixes. Do we know if W3C spec will be close to our implementation any time soon?
Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(MozIsoDepTech)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
>+NS_IMPL_CYCLE_COLLECTION_TRACE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MozIsoDepTech)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNFCTag)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
Missing NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

>+MozIsoDepTech::Constructor(const GlobalObject& aGlobal,
>+                          MozNFCTag& aNFCTag,
>+                          ErrorResult& aRv)
align

>+{
>+  ErrorResult rv;
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aGlobal.GetAsSupports());
>+  if (!win) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  Nullable<nsTArray<NFCTechType>> techList;
>+  aNFCTag.GetTechList(techList, rv);
>+  if (rv.Failed()) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) {
>+    aRv.Throw(NS_ERROR_FAILURE);
NS_ERROR_FAILURE is Gecko specific error, and looks like a web page could trigger this code path easily.
Perhaps NS_ERROR_DOM_NOT_SUPPORTED_ERR or some such would be better


>+  nsRefPtr<MozIsoDepTech> isodep = new MozIsoDepTech(win, aNFCTag);
>+  if (!isodep) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
Useless null check. 'new' is infallible by default

>+class MozIsoDepTech : public nsISupports,
>+                     public nsWrapperCache
align

>+ * Part of this idl is from:
>+ * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface
>+ *
>+ * Copyright © 2013 Deutsche Telekom, Inc.
Is this Copyright correct?

>+typedef MozIsoDepTech MozTagTech;
Why do we need this typedef? If not needed, remove.
Attachment #8546450 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #28)
> At some point we need to get rid of Moz prefixes. Do we know if W3C spec
> will be close to our implementation any time soon?

Hi, Smaug
Actually W3C has changed their API dramatically recently (Maybe because of different editor) 
http://w3c.github.io/nfc/

Can you suggest a timing to remove Moz prefix?
Given that we're going to make our NFC API as privileged.

Thanks
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #29)
> >+typedef MozIsoDepTech MozTagTech;
> Why do we need this typedef? If not needed, remove.
Hi smaug
Sorry for not explaining this.

Next we're going to implement some other technologies, like MifareClassicTech.
So the method 'selectTech(NFCTechType tech)' will create the according technology and return it.

For example:

var isoDep = tag.selectTech("ISO-DEP"); // isoDep should be instance of MozIsoDepTech

var mifareClassic = tag.selectTech("MIFARE-Classic"); // mifareClassic will be instance of MozMifareClassicTech.

So next time the typedef will become

typedef (MozIsoDepTech or MozMifareClassicTech) TagTechnology;

interface MozNFCTag {
...
TagTechnology selectTech(NFCTechType tech);
};

Can we do like this?
Or we should create an empty base interface TagTechnology, and it will be inherited by those technologies?
(In reply to Yoshi Huang[:allstars.chh] from comment #30) 
> Hi, Smaug
> Actually W3C has changed their API dramatically recently (Maybe because of
> different editor) 
> http://w3c.github.io/nfc/
> 
> Can you suggest a timing to remove Moz prefix?
> Given that we're going to make our NFC API as privileged.
Oh, maybe then never, but it also probably means that we aren't going to expose the current NFC API to the web pages ever,
but have another API which also other browsers may support.
Are we actively participating the W3C work here? Giving comments and so.
Flags: needinfo?(bugs)
(In reply to Yoshi Huang[:allstars.chh] from comment #31) 
> Can we do like this?
Sounds good. thanks for the explanation.
(In reply to Olli Pettay [:smaug] from comment #32)
> (In reply to Yoshi Huang[:allstars.chh] from comment #30) 
> > Hi, Smaug
> > Actually W3C has changed their API dramatically recently (Maybe because of
> > different editor) 
> > http://w3c.github.io/nfc/
> > 
> > Can you suggest a timing to remove Moz prefix?
> > Given that we're going to make our NFC API as privileged.
> Oh, maybe then never, but it also probably means that we aren't going to
> expose the current NFC API to the web pages ever,
> but have another API which also other browsers may support.
> Are we actively participating the W3C work here? Giving comments and so.

No, we don't have any interactions with W3C NFC.
(Assignee)

Comment 35

4 years ago
(In reply to Yoshi Huang[:allstars.chh] from comment #25)
> Comment on attachment 8546450 [details] [diff] [review]
> Transceive API for ISO DEP patch v1
> 
> Review of attachment 8546450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +41,5 @@
> > +  MozIsoDepTech(nsPIDOMWindow* aWindow, MozNFCTag& aNFCTag);
> > +  ~MozIsoDepTech();
> > +
> > +  nsRefPtr<nsPIDOMWindow> mWindow;
> > +  nsRefPtr<MozNFCTag> mNFCTag;
> 
> should be nsCOMPtr?
> call it 'mTag' should be enough
should use nsRefPtr because MozNFCTag is a concrete class.
(Assignee)

Comment 36

4 years ago
- Fix Nits
- Rebase to latest code
Attachment #8546450 - Attachment is obsolete: true
Attachment #8547935 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [p=5]
(Assignee)

Comment 38

4 years ago
Clearcheckin-need before i would like do another test before checkin
Keywords: checkin-needed
(Assignee)

Comment 41

4 years ago
Hi Yoshi,
Sorry, the test is passed
Keywords: checkin-needed
(Assignee)

Comment 42

4 years ago
After checking with yoshi, i will update comment and update patch again.
Keywords: checkin-needed
(Assignee)

Comment 43

4 years ago
Modify comment
Attachment #8547935 - Attachment is obsolete: true
Attachment #8547995 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 44

4 years ago
Comment on attachment 8547995 [details] [diff] [review]
Transceive API for ISO DEP patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547995 - Flags: approval-mozilla-b2g37?
(Assignee)

Updated

4 years ago
Attachment #8547995 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/7d7a31553aa1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
(Assignee)

Comment 47

4 years ago
Comment on attachment 8547995 [details] [diff] [review]
Transceive API for ISO DEP patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
NFC feature

User impact if declined: 
Third party Apps can not use NFC transceive function to get information
from tag

Testing completed: 
Manually.

Risk to taking this patch (and alternatives if risky): 
No.

String or UUID changes made by this patch:
No.
Attachment #8547995 - Flags: approval-mozilla-b2g37?
Blocks: 1121900
Attachment #8547995 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.