This bug is for supporting NfcA (ISO 14443-3A) technology tags.

Is it required by bug 916428? Not sure if KDDI would like to take this.

Two bugs are independent from each other. API should be designed for both but NfcA (ISO 14443-3A) implementation is not in our scope currently.

Yoshi raised a question about the tag technology relationship design, Ex, if ISO_DEP should inherit from NFC_A. I will check this in this bug.

Since ISO_DEP may base on NFC_A or NFC_B, also NFC_A and NFC_B do not have directly inheritance relationship between each other. So for this case, i think it may not suitable use inheritance for ISO_DEP : NFC_A or ISO_DEP : NFC_B

This patch basically only support transceive function.

Comment on attachment 8551663 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v1 Review of attachment 8551663 [details] [diff] [review]: ----------------------------------------------------------------- I guess you miss MozNFCTag.webidl. ::: dom/nfc/MozNfcATech.cpp @@ +39,5 @@ > +/* static */ > +already_AddRefed<MozNfcATech> > +MozNfcATech::Constructor(const GlobalObject& aGlobal, > + MozNFCTag& aNFCTag, > + ErrorResult& aRv) Align @@ +59,5 @@ > + if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) { > + aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); > + return nullptr; > + } > + Maybe create a helper to do this. @@ +88,5 @@ > + return nullptr; > + } > + > + return promise.forget(); > +} perhaps this too. ::: dom/webidl/MozNfcATech.webidl @@ +7,5 @@ > +interface MozNfcATech { > + /** > + * Send raw command to tag and receive the response. > + */ > + [Throws] You don't have to [Throws] Bug 1113827.

Comment on attachment 8551663 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v1 Review of attachment 8551663 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/MozNfcATech.webidl @@ +7,5 @@ > +interface MozNfcATech { > + /** > + * Send raw command to tag and receive the response. > + */ > + [Throws] Oh, I think I am wrong, ingore this.

Comment on attachment 8553591 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v2 Review of attachment 8553591 [details] [diff] [review]: ----------------------------------------------------------------- I move |static const NFCTechType mTechnology = NFCTechType::ISO_DEP;| to cpp because of bug 1121900, It seems in certain version of gcc may cause build error by using original coding style

Comment on attachment 8553591 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v2 Review of attachment 8553591 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/MozIsoDepTech.h @@ +44,5 @@ > > nsRefPtr<nsPIDOMWindow> mWindow; > nsRefPtr<MozNFCTag> mTag; > > + static const NFCTechType mTechnology; fix Bug 1121900 first. ::: dom/nfc/TagUtils.h @@ +12,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +/* Javadoc style. /** * ... */

(In reply to Yoshi Huang[:allstars.chh] from comment #6) > Comment on attachment 8551663 [details] [diff] [review] > WebAPI for supporting NFC_A tags patch v1 > > Review of attachment 8551663 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess you miss MozNFCTag.webidl. > > ::: dom/nfc/MozNfcATech.cpp > @@ +39,5 @@ > > +/* static */ > > +already_AddRefed<MozNfcATech> > > +MozNfcATech::Constructor(const GlobalObject& aGlobal, > > + MozNFCTag& aNFCTag, > > + ErrorResult& aRv) > > Align > > @@ +59,5 @@ > > + if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) { > > + aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); > > + return nullptr; > > + } > > + > > Maybe create a helper to do this. > > @@ +88,5 @@ > > + return nullptr; > > + } > > + > > + return promise.forget(); > > +} > > perhaps this too. > > ::: dom/webidl/MozNfcATech.webidl > @@ +7,5 @@ > > +interface MozNfcATech { > > + /** > > + * Send raw command to tag and receive the response. > > + */ > > + [Throws] > > You don't have to [Throws] > Bug 1113827. We may have diversity tech types tag (NFCA in this bug, and Mifare Ultralight in bug 884478 and maybe other types later). We could refactor to create a based class ( MozBasicTag includes: transceive(...) and checkTechnologySupport(...) Other tag type class (MozNfcATech for example) will be derived from the based class. It looks better in term of pattern design. Do you agree with that?

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #11) > We may have diversity tech types tag (NFCA in this bug, and Mifare > Ultralight in bug 884478 and maybe other types later). We could refactor to > create a based class ( MozBasicTag includes: transceive(...) and > checkTechnologySupport(...) See MozNFCTag.webidl > Other tag type class (MozNfcATech for example) will be derived from the > based class. It looks better in term of pattern design. Do you agree with > that? No, this is wrong. NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag. A tag is not neccessary to have a NFC_A technology, how do you inherit it?

(In reply to Yoshi Huang[:allstars.chh] from comment #12) > (In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #11) > > We may have diversity tech types tag (NFCA in this bug, and Mifare > > Ultralight in bug 884478 and maybe other types later). We could refactor to > > create a based class ( MozBasicTag includes: transceive(...) and > > checkTechnologySupport(...) > See MozNFCTag.webidl > > > Other tag type class (MozNfcATech for example) will be derived from the > > based class. It looks better in term of pattern design. Do you agree with > > that? > > No, this is wrong. > NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag. > > A tag is not neccessary to have a NFC_A technology, how do you inherit it? Thank you. I mean create a NEW internal based class (for example MozBasicTech) to use later. MozNfcATech will extend MozBasicTech

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #13) > > > Other tag type class (MozNfcATech for example) will be derived from the > > > based class. It looks better in term of pattern design. Do you agree with > > > that? > > > > No, this is wrong. > > NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag. > > > > A tag is not neccessary to have a NFC_A technology, how do you inherit it? > > Thank you. > I mean create a NEW internal based class (for example MozBasicTech) to use > later. > MozNfcATech will extend MozBasicTech No, I didn't choose this design because not every technology could do 'transceive'.

Changes compared to patch v2 - Rebased and fixed conflicts - Fix mode line due to bug 1152551 Thanks

Comment on attachment 8683466 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v3 Rebased Review of attachment 8683466 [details] [diff] [review]: ----------------------------------------------------------------- Please use 8 lines of context. Mostly okay, but it would be great if we can test this first, for example, try to add somes tests for NFC_A in https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/nfc-api-test/js/nfc_tech.js ::: dom/nfc/MozNfcATech.cpp @@ +56,5 @@ > + return nullptr; > + } > + > + RefPtr<MozNfcATech> nfcA = new MozNfcATech(win, aNFCTag); > + remove extra line ::: dom/nfc/TagUtils.cpp @@ +19,5 @@ > + Nullable<nsTArray<NFCTechType>> techList; > + aTag.GetTechList(techList, rv); > + if (rv.Failed()) { > + return false; > + } NS_ENSURE_SUCCESS(rv, false); @@ +25,5 @@ > + if (techList.IsNull() || !(techList.Value().Contains(aTechnology))) { > + return false; > + } > + > + return true; return !A && B; ::: dom/nfc/TagUtils.h @@ +10,5 @@ > +#include "mozilla/dom/MozNFCTagBinding.h" > + > +namespace mozilla { > +namespace dom { > + should have another namespace nfc @@ +11,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +/* Use /** same for others. @@ +12,5 @@ > +namespace mozilla { > +namespace dom { > + > +/* > + * This a helper function for various nfc tag. Please rephrase here. @@ +22,5 @@ > + /* > + * Check if specified technogy is supported in this tag. > + */ > + static bool > + CheckTechnologySupport(const MozNFCTag& aNFCTag, IsTechSupported @@ +31,5 @@ > + */ > + static already_AddRefed<Promise> > + Transceive(MozNFCTag* aTag, > + const Uint8Array& aCommand, > + const NFCTechType& aTechnology, aTechnology should be the 2nd argument.

Add some tests for NFCA transceive API in gaia/dev-app/nfc-nci-test including: - Write data to 1 block of NFCA tag type 2 - Read data from 4 blocks of NFCA tag type 2 - Get UID of NFCA tag type 1

Why do we need both MozNfcATech and MozIsoDepTech webidl interfaces? The APIs look exactly the same. Why not just rename MozIsoDepTech to MozTagTech and remove the typedef? And if the implementation needs tech type, add that as param to the ChromeConstructor?

Comment on attachment 8684833 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v4 So, don't understand the reason for functionally duplicate interfaces. Either merge them and upload a new patch, or explain why two different webidl interfaces are needed and ask review again.

Thank you for your review. There's diversity of nfc tech (ISO-DEP, NFC-A in this bug, Mifare-Ultralight in bug 884478, etc ..), and most of them have capability of "transceive command". Reference NFC digital Protocol Technical Specification http://www.cardsys.dk/download/NFC_Docs/NFC%20Digital%20Protocol%20Technical%20Specification.pdf Apparently, the interfaces look exactly the same in term of coding design, however the transceive command protocols are totally different. For example, ISO-DEP interface transceive supports ADPU command sending to NFC tag, but NFC-A interface uses low level command to send to the tag. It would be better if there're 2 separated interfaces Thanks

Changes: - Update comment in WebIDL transceive operations more clearly

(In reply to Olli Pettay [:smaug] from comment #19) > Why do we need both MozNfcATech and MozIsoDepTech webidl interfaces? The > APIs look exactly the same. > Why not just rename MozIsoDepTech to MozTagTech and remove the typedef? > > And if the implementation needs tech type, add that as param to the > ChromeConstructor? So far we only implemented transceive for these two technologies, but they still have other interfaces we didn't implement yet. See IsoDep [1] and NfcA [2] on Android [1] : http://developer.android.com/reference/android/nfc/tech/IsoDep.html [2] : http://developer.android.com/reference/android/nfc/tech/NfcA.html Also we didn't choose to add a base class like TagTechology [3] to be inherited by NFC techonogies on Android, because some techonogy like NFCBarcode doesn't have trasnceive. So adding a base class like TagTechonology which has only 1~2 interfaces (trasnceive and getTransceiveTimeout), but could not be inherited by all NFC techonology classes, doesn't look like a cleaner solution to us with regarding to implementation. [3]: http://developer.android.com/reference/android/nfc/tech/TagTechnology.html

Comment on attachment 8685227 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v5 >+ >+ static const NFCTechType mTechnology; Nit, static member variables should have s-prefix, not m-prefix. So, sTechnology Ok, thanks for the explanation.

Comment on attachment 8685227 [details] [diff] [review] WebAPI for supporting NFC_A tags patch v5 Review of attachment 8685227 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/MozIsoDepTech.cpp @@ +50,5 @@ > aRv.Throw(NS_ERROR_FAILURE); > return nullptr; > } > > + using namespace mozilla::dom::nfc; put using namespace mozilla::dom::nfc::TagUtils; at top, same for MozNfcATech.cpp

Comment on attachment 8684836 [details] [diff] [review] Add NFCA transceive test functions to gaia nfc-api-test app use PR

- Rename mTechnology - Move using namespace to the top

Hi Tim, Sorry to flag you as I have no idea who is available to review this gaia PR (nfc-test-api) at the moment. Could you please tell me I could tag whom to this bug? Thanks for your help

Comment on attachment 8687112 [details] (gaia)Add NFCA tech tests api PR v2 you can land anything for the eng build and for manual testing.

Try results: - Gecko https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74c44ca331e - Gaia https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=5d5cd515339fa844af410938295a14442168a91b

gaia: https://github.com/mozilla-b2g/gaia/commit/089eb0fe17c9809a5465f92ec82b63a990413698