Comment 1 Markus Neubrand 2012-01-25 13:30:32 PST

Created attachment 591592 [details]
API proposal

Comment 2 Markus Neubrand 2012-01-25 13:31:22 PST

Hi Chris,

Arno and I would like to implement the NFC API as contribution of Deutsche Telekom. Attached you can find a first draft of the API in xpidl format.

The API proposal mainly based on phonegap-nfc (https://github.com/chariotsolutions/phonegap-nfc) and Android API (http://developer.android.com/reference/android/nfc/NfcAdapter.html). We also took some other NFC API's (e.g. Qt Mobility 1.2 Connectivity API) into consideration. The tag/tech nomenclature is from Android. The idl is mainly based on existing ones (netwerk/wifi, dom/telephony, etc.). We decided to use a syntax similar to netwerk/wifi instead of nsIDOMEventListener because you want to be able to attach listener with a filter (e.g. mimetype, etc.)

This is just a first draft and any feedback is welcome. It also leaves out all constants we plan more or less taking one to one from Android (e.g. NDEF tag types, etc.). Furthermore the draft only is for NDEF tag reading/writing we will add more functionality like p2p communication and other tech later but we kept them in mind.

We will be implementing it on top of libnfc-nxp which is already in the B2G source tree.

As we are new to Gecko development we also have several questions:
1. Where should we put that new interface (maybe netwerk/nfc)?
2. Should we moz prefix anything in the idl ?
3. How are we supposed to implement security (e.g. not every app should be able to use this feature. Maybe also only foreground apps should be allowed to be notified). Is there any similar feature we can take a look at. Also is there something like Android's Intent system so apps can be started once a certain tag gets discovered in B2G?

Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-26 02:11:59 PST

Hey Markus,

(In reply to Markus Neubrand from comment #2)
> Hi Chris,
> 
> Arno and I would like to implement the NFC API as contribution of Deutsche
> Telekom. Attached you can find a first draft of the API in xpidl format.
> 

Great!  Your approach looks good, but I'm not the best reviewer of DOM APIs.

> As we are new to Gecko development we also have several questions:

I'm also not the best person to answer these questions, but folks on the CC list can help.

> 1. Where should we put that new interface (maybe netwerk/nfc)?

My guess would be dom/nfc/interfaces.  Others can correct me.

> 2. Should we moz prefix anything in the idl ?

I believe our style is that DOM interfaces are names nsIDOM*, but again I'm not an expert here.

> 3. How are we supposed to implement security (e.g. not every app should be
> able to use this feature. Maybe also only foreground apps should be allowed
> to be notified). Is there any similar feature we can take a look at. Also is
> there something like Android's Intent system so apps can be started once a
> certain tag gets discovered in B2G?

Obviously, I wouldn't let these issues slow down your prototyping.  Currently, we have a very simple but rather hard-to-use permissions band-aid built on Gecko "preferences".  We can point you to an example when it becomes relevant.  We're currently working on a cleaner permissions manager (https://bugzilla.mozilla.org/show_bug.cgi?id=707625), which hopefully will be ready by the time you're done here.  Similarly, we're in the middle of prototyping an intents mechanism, but it's not ready for use within gecko yet.

Let (us) know if you have more questions about gecko hacking.

Comment 4 Mounir Lamouri (:mounir) 2012-01-26 03:44:01 PST

A few random comments:
- Don't use hasSupport() methods. I think it would be better to have navigator.nfc returning null if there is no support or use another API (like Device Capabilities API) to know if the device has NFC support;
- Your listeners objects are not really Weby. you should use addEventListener() and DOM events instead;
- In nsINdef, I would change isWriteable() function to a readonly attribute and I don't really like the readonly handling but I've no better solution off-hand;
- In nsINdefRecord, can the 'create' methods be sync safely? Should we use an async API here?

Generally, I think it would be better to discuss that API in dev-webapi mailing list [1] instead of here.

[1] https://www.mozilla.org/about/forums/#dev-webapi

Comment 5 Philipp von Weitershausen [:philikon] 2012-01-26 06:52:42 PST

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> - Your listeners objects are not really Weby. you should use
> addEventListener() and DOM events instead;

I agree. I realize you mention filtering as a reason for not going with EventTarget/EventListener in comment 2. Event listeners could easily filter themselves, though, right? It's not like NFC events would occur at a high frequency, or am I wrong?

> - In nsINdefRecord, can the 'create' methods be sync safely? Should we use
> an async API here?

In particular, why do those methods exist on nsINdefRecord? Actually, why do they exist at all? :) I'm not 100% sure I understand all the abstractions here yet. It would be good to talk through some use cases with JS pseudocode on the dev-webapi mailinglist that Mounir mentioned, particularly for those who are not super familiar with NFC yet. :)

Comment 6 Markus Neubrand 2012-01-26 13:11:10 PST

@Chris: I will make those changes and DOMify our API.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> A few random comments:
> - Don't use hasSupport() methods. I think it would be better to have
> navigator.nfc returning null if there is no support or use another API (like
> Device Capabilities API) to know if the device has NFC support;

Will do.

> - Your listeners objects are not really Weby. you should use
> addEventListener() and DOM events instead;

I think if we don't build that filter mechanism into the API but instead leave it to the application to filter that there will be huge overhead. Imagine if there are ~10 apps which want to receive NFC tag discoveries. They'll all be notified and have to do the same filtering. I don't think that this is a good solution. Of course that problem won't occur if we only allow e.g. the foreground app to interact with the NFC API.

> - In nsINdef, I would change isWriteable() function to a readonly attribute
> and I don't really like the readonly handling but I've no better solution
> off-hand;

Will do

> - In nsINdefRecord, can the 'create' methods be sync safely? Should we use
> an async API here?

Those are meant as helper methods to be able to create a Uri/Mime NdefRecord without having to know how to fill tnf, type and id with the appropriate constants. In Java/C++ land those would be static methods I don't know how to do that stuff in JS. Maybe we should move them into a separate helper interface to make that clear.

> 
> Generally, I think it would be better to discuss that API in dev-webapi
> mailing list [1] instead of here.
> 
> [1] https://www.mozilla.org/about/forums/#dev-webapi

Once I made the mentioned changes I will post it to the mailing list with some simple use cases. Btw. I am not super familiar with NFC yet by any means either.

Thanks for the feedback!

Comment 7 Philipp von Weitershausen [:philikon] 2012-01-26 13:30:22 PST

(In reply to Markus Neubrand from comment #6)
> Those are meant as helper methods to be able to create a Uri/Mime NdefRecord
> without having to know how to fill tnf, type and id with the appropriate
> constants. In Java/C++ land those would be static methods I don't know how
> to do that stuff in JS. Maybe we should move them into a separate helper
> interface to make that clear.

There are no static methods like that in DOM APIs. I'm curious, though, why content would want to create those NdefRecords at all. It seems to me content merely consumes these objects.

Comment 8 Mounir Lamouri (:mounir) 2012-01-30 02:54:36 PST

(In reply to Markus Neubrand from comment #6)
> I think if we don't build that filter mechanism into the API but instead
> leave it to the application to filter that there will be huge overhead.
> Imagine if there are ~10 apps which want to receive NFC tag discoveries.
> They'll all be notified and have to do the same filtering. I don't think
> that this is a good solution. Of course that problem won't occur if we only
> allow e.g. the foreground app to interact with the NFC API.

I see only two reasons to keep filtering outside of the listening handler code:
1. performance issues
2. security issues

We should be able to solve (2) by simply not firing the event if the listener isn't allowed to know about it. For (1), that would be an issue if there are a lot of events being sent and we really want to minimize the number of applications that are going to be waken to handle them. However, I've the feeling that NFC isn't something used that much and those events will be fired only at some specific moments when the user is actually interacting with something specific. Is that correct?

In my opinion, the cost of using a new way to handle events (inconsistency) seems higher than the win it gives (prevents a few events to be fired).

Comment 9 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-08 16:56:54 PST

there were some interesting attacks and ideas shown at cansec that we may want to be cognizant of as we move forward with this

Comment 10 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-21 14:36:55 PDT

needs to be scheduled for a team review

Comment 11 Markus Neubrand 2012-04-26 13:52:55 PDT

Created attachment 618790 [details] [diff] [review]
First draft implementation of the NFC API in Gecko

See https://wiki.mozilla.org/WebAPI/WebNFC for API draft

Comment 12 Mounir Lamouri (:mounir) 2012-04-27 02:06:50 PDT

Markus, note that you will need to ask a review to a DOM peer and ask a super-review to a super-reviewer. Those two persons have to be different.

Comment 13 Kyle Machulis [:kmachulis] [:qdot] 2012-04-27 10:04:04 PDT

Yeah, I'm doing the first round for cleanup, and part of that is trying to split out the boilerplate and DOM reviewable needs. We'll hopefully have superreviewers in on the second round.

Comment 14 Kyle Machulis [:kmachulis] [:qdot] 2012-05-28 11:21:18 PDT

Unless something has changed drastically, this is not a blocker for V1.

Comment 15 Markus Neubrand 2012-06-15 09:34:57 PDT

Created attachment 633560 [details] [diff] [review]
Rebased first draft implementation

Comment 16 Kyle Machulis [:kmachulis] [:qdot] 2012-07-05 15:54:16 PDT

Comment on attachment 633560 [details] [diff] [review]
Rebased first draft implementation

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

Not too bad on the first pass. Mostly cosmetic issues, plus some changes because of things shifting between gecko and gaia.

A few things to add:
- You'll need a DOM Peer to superreview this as it's a change to the DOM. I'll talk to someone about doing that.
- I can't actually comment on the file specifically since there's nothing there, but you need to add your component info to Nfc.manifest

Also, we need to check for NFC existence, like seeing if the nfc daemon executable is even available. Since this was based off the RIL, it makes assumptions that aren't really allowed for something that's an optional feature. It might be worth adding a static DoesNFCExist() function or something in dom/nfc/Nfc.h that checks whether the NFC executable even exists, otherwise if you try to boot the IO thread for talking to the socket, it'll just sit there and poll for the life of the gecko process, even though it never has a chance of running.

::: b2g/app/b2g.js
@@ +16,4 @@
>  // XXX TODO : we should read them from a file somewhere
>  pref("b2g.privileged.domains", "http://browser.gaiamobile.org,
>  	                            http://calculator.gaiamobile.org,
> +	                            http://nfc-demo.gaiamobile.org,

Remove, we do this over in gaia now.

::: dom/Makefile.in
@@ +82,5 @@
>  endif
>  
> +ifdef MOZ_B2G_NFC
> +DIRS += \
> +  nfc \

Just one DIRS += nfc line here, don't need multiline

::: dom/nfc/Makefile.in
@@ +1,1 @@
> +# ***** BEGIN LICENSE BLOCK *****

Nit: New license block on this

@@ +64,5 @@
> +#LOCAL_INCLUDES = \
> +#  -I$(topsrcdir)/dom/base \
> +#  -I$(topsrcdir)/dom/system/b2g \
> +#  -I$(topsrcdir)/content/events/src \
> +#  $(NULL)

Remove

::: dom/nfc/Nfc.cpp
@@ +1,3 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */
> +/* ***** BEGIN LICENSE BLOCK *****

New license block

@@ +59,5 @@
> +
> +USING_NFC_NAMESPACE
> +using mozilla::Preferences;
> +
> +#define DOM_TELEPHONY_APP_PHONE_URL_PREF "dom.telephony.app.phone.url"

This isn't telephony

@@ +90,5 @@
> +  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aOwner);
> +  NS_ENSURE_TRUE(sgo, nsnull);
> +
> +  nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
> +  NS_ENSURE_TRUE(scriptContext, nsnull);

Why are you doing the above work? You don't need a script context at this point.

@@ +139,5 @@
> +
> +NS_IMETHODIMP
> +Nfc::NdefDiscovered(const nsAString &aNdefRecords)
> +{
> +  //NS_ASSERTION(aNdefRecords, "Null records!");

Nit: remove

@@ +159,5 @@
> +  // Dispatch incoming event.
> +  nsRefPtr<NfcNdefEvent> event = NfcNdefEvent::Create(result);
> +  NS_ASSERTION(event, "This should never fail!");
> +
> +  LOG("DOM: Created event => dispatching");

Nit: Can you wrap these log calls in a DEBUG define or something? otherwise this is going to get really spammy.

@@ +161,5 @@
> +  NS_ASSERTION(event, "This should never fail!");
> +
> +  LOG("DOM: Created event => dispatching");
> +  rv = event->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("ndefdiscovered"));
> +  LOG("DOM: Event dispatched (%d)", NS_ERROR_GET_CODE(rv));

NS_ENSURE_SUCCESS will NS_WARNING for you, so you don't need to double log.

@@ +246,5 @@
> +    if (!allowed) {
> +      *aTelephony = nsnull;
> +      return NS_OK;
> +    }
> +  }*/

I realize this is commented out at the moment, but you're gonna need app permissions (We've got this in current bluetooth if you want to check out how I do it there). Replace this block with URIIsChromeOrInPref from nsContentUtils. We'll have to add whitelisting to gaia too, but that's pretty easy.

::: dom/nfc/Nfc.h
@@ +44,5 @@
> +
> +#include "nsIDOMNfc.h"
> +#include "nsINfc.h"
> +
> +class nsIScriptContext;

Nit: I don't see where this is needed to be predeclared in the header?

@@ +49,5 @@
> +class nsPIDOMWindow;
> +
> +BEGIN_NFC_NAMESPACE
> +
> +class Nfc : public nsDOMEventTargetHelper,

Nit: To avoid conflict with the Nfc stuff over in ipc, we might want to call this nsNfc, since it's the implementation of the nsINfc interface.

@@ +50,5 @@
> +
> +BEGIN_NFC_NAMESPACE
> +
> +class Nfc : public nsDOMEventTargetHelper,
> +                  public nsIDOMNfc

Nit: Alignment

@@ +55,5 @@
> +{
> +  nsCOMPtr<nsINfc> mNfc;
> +  nsCOMPtr<nsINFCCallback> mNFCCallback;
> +
> +  NS_DECL_EVENT_HANDLER(ndefdiscovered)

I don't see in the IDL where you're set up to fire an onndefdiscovered event?

@@ +92,5 @@
> +private:
> +  Nfc();
> +  ~Nfc();
> +
> +  class NFCCallback : public nsINFCCallback

Not sure why this needs to be a member class?

::: dom/nfc/NfcCommon.h
@@ +45,5 @@
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsDebug.h"
> +#include "nsDOMEventTargetHelper.h"
> +#include "nsStringGlue.h"
> +#include "nsTArray.h"

This is a ton of stuff to include everywhere if you aren't actually declaring any types here. Move these to the compilation units that need them.

@@ +55,5 @@
> +#define USING_NFC_NAMESPACE \
> +  using namespace mozilla::dom::nfc;
> +
> +class nsIDOMNfc;
> +class nsIDOMNfcNdef;

What do these need to be predeclared up here for?

::: dom/nfc/NfcFactory.h
@@ +1,3 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: New license header if we keep this.

@@ +45,5 @@
> +
> +// Implemented in Nfc.cpp.
> +
> +nsresult
> +NS_NewNfc(nsPIDOMWindow* aWindow, nsIDOMNfc** aNfc);

This could probably be moved to Nfc.h (or nsNfc.h if it's fixed like I recommended :) )

::: dom/nfc/NfcNdefEvent.cpp
@@ +1,3 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: new license header

@@ +46,5 @@
> +
> +USING_NFC_NAMESPACE
> +
> +// static
> +already_AddRefed<NfcNdefEvent>

Returning a nsRefPtr should return as TemporaryPtr. However, since this is a DOM event, you probably want to be returning nsIDOMNfcNdefEvent?

::: dom/nfc/NfcNdefEvent.h
@@ +1,3 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=40: */
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: new license header

::: dom/nfc/nsIDOMNavigatorNfc.idl
@@ +1,1 @@
> +#include "nsISupports.idl"

Nit: Needs license header.

::: dom/nfc/nsIDOMNfc.idl
@@ +1,1 @@
> +#include "nsIDOMEventTarget.idl"

Nit: Needs license header

@@ +1,3 @@
> +#include "nsIDOMEventTarget.idl"
> +
> +interface nsIDOMNfcNdefRecord;

Nit: Prototype not used, remove.

::: dom/nfc/nsIDOMNfcNdefEvent.idl
@@ +1,1 @@
> +#include "nsIDOMEvent.idl"

Nit: Needs license header

@@ +1,3 @@
> +#include "nsIDOMEvent.idl"
> +
> +interface nsIDOMNfcNdefRecord;

Nit: prototype not used, remove.

::: dom/system/gonk/Nfc.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: Needs new license header.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +536,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +SystemWorkerManager::InitNFC(JSContext *cx)

Nit: This is Nfc everywhere else. Should probably stick with one or the other.

@@ +544,5 @@
> +  // worker lives in RadioInterfaceLayer.js. All we do here is hold it alive and
> +  // hook it up to the NFC thread.
> +  nsresult rv;
> +  nsCOMPtr<nsIWorkerHolder> worker = do_CreateInstance(kNfcWorkerCID, &rv);
> +  LOG("Error: %d", NS_ERROR_GET_CODE(rv));

This'll print out on NS_ENSURE_SUCCESS failing, don't need it.

@@ +546,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIWorkerHolder> worker = do_CreateInstance(kNfcWorkerCID, &rv);
> +  LOG("Error: %d", NS_ERROR_GET_CODE(rv));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);

You should fail before you get here, since rv will have failed?

::: dom/system/gonk/nfc_consts.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Nit: New license header

@@ +37,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +// Allow this file to be imported via Components.utils.import().
> +const EXPORTED_SYMBOLS = Object.keys(this);

Is there gonna be more to this?

::: dom/system/gonk/nfc_worker.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: New license header

@@ +43,5 @@
> +
> +// We leave this as 'undefined' instead of setting it to 'false'. That
> +// way an outer scope can define it to 'true' (e.g. for testing purposes)
> +// without us overriding that here.
> +let DEBUG = true;

Probably want to land this undef'd.

::: dom/system/gonk/nsINfc.idl
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: New license header

@@ +39,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(999fa430-6180-11e1-b86c-0800200c9a66)]
> +interface nsINFCCallback : nsISupports

nit: One interface per idl file (wasn't even aware you could do this). Also: NFC should be Nfc if you're going to stay consistent with the majority of your code thus far.

::: dom/system/gonk/nsNfc.h
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: New license header

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +// This must always match the CID given in RadioInterfaceLayer.manifest!

Nit: This isn't RIL :)

::: ipc/nfc/Makefile.in
@@ +1,1 @@
> +# ***** BEGIN LICENSE BLOCK *****

Nit: new license header

::: ipc/nfc/Nfc.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* vim: set sw=4 ts=8 et ft=cpp: */

Nit: new license header

@@ +83,5 @@
> +struct NfcClient : public RefCounted<NfcClient>,
> +                   public MessageLoopForIO::Watcher
> +
> +{
> +    typedef queue<NfcData*> NfcDataQueue;

Nit: 2 spaces for tabs. Also: can probably use nsTArray instead of queue, but queue is probably fine (since I know this is copied from RIL :) )

@@ +358,5 @@
> +              goto clean_and_return;
> +            } else {
> +              mCurrentWriteOffset += written;
> +            }
> +//            LOG("XXXXXXXXXXXXXXXXXXXXXXX written:%ld offset:%ld\n", written, mCurrentWriteOffset);

Nit: remove

::: ipc/nfc/Nfc.h
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=8 et ft=cpp: */
> +/* ***** BEGIN LICENSE BLOCK *****

Nit: New license header

@@ +38,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef mozilla_ipc_Nfc_h
> +#define mozilla_ipc_Nfc_h 1

Nit: Don't need a 1, just a define is fine. Also keep all lowercase just for consistency sake.

@@ +46,5 @@
> +namespace base {
> +class MessageLoop;
> +}
> +
> +class nsIThread;

nit: Unused prototype

@@ +54,5 @@
> +
> +
> +struct NfcData
> +{
> +    char* json;

Nit: 2 spaces for tabs. Also: If this is json, it's probably coming in as UTF8, right? Might we want to do the conversation to nsCString at the time it's received?

@@ +61,5 @@
> +class NfcConsumer : public RefCounted<NfcConsumer>
> +{
> +public:
> +    virtual ~NfcConsumer() { }
> +    virtual void MessageReceived(NfcData* aMessage) { }

Should be pure virtual

@@ +71,5 @@
> +
> +void StopNfc();
> +
> +} // namespace ipc
> +} // namepsace mozilla

Nit: namespace

Comment 17 Kyle Machulis [:kmachulis] [:qdot] 2012-07-05 16:18:25 PDT

Talked to a few people and we're running low on device reviewing bandwidth right now, so we'll get the DOM Peer review and Module owner/SR Superreviewer after some of our cleanup is done.

Comment 18 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-07-09 09:47:37 PDT

Guys we should have a security review of the proposed architecture of what we are doing here before we get too far down the road. There have been several talks at prominent security conferences this year on NFC and we want to ensure we have everything thought out to avoid problems. Please work with me to find a lead from your end so we can find a date to have a security "chat" about this.

Comment 19 Markus Neubrand 2012-07-30 14:53:21 PDT

Created attachment 647307 [details] [diff] [review]
Implemented review feedback, updated SystemWorker IPC implementation

- SystemWorker IPC updated to reflect RIL
- Updated all headers to include appropriate license
- Implemented feedback from previous review

- Not implemented feedback:
    o Left NfcCallback as member class as it doesn't really make sense to separate it and RIL does this as well ;)
    o Left NfcData as char* cause we treat it like that on the daemon side and base64 encode content anyways

Comment 20 Garner Lee 2012-11-05 16:50:15 PST

Created attachment 678560 [details]
Patch #3 tarball for NFC feature for review (gecko, gaia, nfcd, gonk-misc, b2g)

Comment 21 Garner Lee 2012-11-05 16:52:39 PST

Request for top level review for Andreas Gal.

Comment 22 Garner Lee 2012-11-05 20:52:07 PST

Created attachment 678619 [details]
Patch #3 tarball for NFC feature for review (gecko, gaia, nfcd, gonk-misc, b2g)

Re-upload tarball (the daemon code nfcd's tar file didn't unpack correctly). Each patch goes into the respective directories, and nfcd goes into the <FirefoxOS>/ build root directory.

Comment 23 Kyle Machulis [:kmachulis] [:qdot] 2012-11-05 21:15:29 PST

Comment on attachment 678619 [details]
Patch #3 tarball for NFC feature for review (gecko, gaia, nfcd, gonk-misc, b2g)

... A tarball? Seriously?

Attaching gal to review this since he apparently asked for it.

Comment 24 Garner Lee 2012-11-05 21:24:47 PST

Created attachment 678622 [details] [diff] [review]
Gecko NFC feature patch

Comment 25 Garner Lee 2012-11-05 21:25:37 PST

Created attachment 678623 [details] [diff] [review]
Gaia NFC feature patch

Comment 26 Garner Lee 2012-11-05 21:26:27 PST

Created attachment 678624 [details] [diff] [review]
Gonk-misc NFC feature patch

Comment 27 Garner Lee 2012-11-05 21:28:17 PST

Created attachment 678625 [details] [diff] [review]
B2G (source root) NFC feature patch

Comment 28 Garner Lee 2012-11-05 22:07:24 PST

Created attachment 678633 [details] [diff] [review]
NFC daemon feature patch (without external project source directories)

Comment 29 Kyle Machulis [:kmachulis] [:qdot] 2012-11-05 22:38:50 PST

Comment on attachment 678633 [details] [diff] [review]
NFC daemon feature patch (without external project source directories)

463kb. Yeah. You guys have fun with this.

Comment 30 Andreas Gal :gal 2012-11-06 13:46:23 PST

Ok. I reviewed the patches pretty thoroughly. This is not going to be a popular comment, but please bear with me.

The current architecture uses the JNI library code of the android NFC implementation, which itself is based on libnxp and libhardware. The JNI code is wrapped with a JNI emulation layer, that itself uses the boehm gc and a synchronization library. On top of that code sits a socket daemon that Gecko talks to.

Concerns:
- The nfc daemon runs with device access (root). Adding a ton of new C/C++ code running as root is a serious security risk.
- The quality of the android JNI code is abysmal. We would have to very carefully vet this code.
- Both the boehm gc and the synchronization library are large code bases. Running them as root is questionable. I am also concerned about memory leaks since boehm is a conservative gc.
- The boilerplate to active code ratio of the current architecture is really bad. Disregarding boehm gc and the synchronization library the actual relevant code is less than 10%. The rest is JNI and JNI emulation boilerplate. With the external libraries this drops to below 5%.

Here is what I propose instead:

I would like to replicate the architecture of the wifi driver in Gecko. Instead of an external demo, we have a nfc worker which uses ctypes to call directly into libhardware and libnxp. I am attaching a JS file with the ctypes definition for libhardware as an example how simple this is. We will rewrite the small fraction of actual NFC calls we use in JS in the worker. The work communicates with the existing DOM code using JSON/postMessage. We won't need any changes to gonk any more.

Comment 31 Andreas Gal :gal 2012-11-06 13:46:45 PST

Created attachment 678896 [details]
libhardware.js

Comment 32 Andreas Gal :gal 2012-11-06 13:49:21 PST

I estimate that it will take about 2 weeks to write the JS code for this. The code should be around 1500 lines of code for the worker. Since we already have a working solution, it will be very easy to debug this. We can basically run the two code bases side by side to debug the new production quality worker code.

The proposed architecture should be pretty straight forward to implement, and will add JS code, which is inherently memory safe. Only the actual ctype calls will add significant risk of buffer overflows as root etc. We won't have to touch gonk, and the JS code will be much less code added than the current solution, and use much less memory, so we should be able to ship it with all hardware we ship and make it part of the standard Gecko distro.

Comment 33 Andreas Gal :gal 2012-11-06 13:49:48 PST

Markus, Arno, what do you guys think?

Comment 34 arno 2012-11-06 15:59:17 PST

Thanks for your feedback. We were a little surprised by some of your comments since we have discussed the basic architecture with Philip and Kyle several times before and they have not voiced any objections. Here some specific responses to your comments:

* Running nfcd as non-root is easy to change (and definitely needs to happen)

* You mentioned “synchronization library” in your review. We are not sure what you are referring to. Note that libnxp relies on pthread synchronization so this is not something we can change.

* libnxp is not a widely used library and in terms of API design and documentation no where near the wifi driver that you refer to. To give you an idea of the libnxp API, here just one little example:

typedef NFCSTATUS ( *pphNfcIF_Register_t) (
                                phNfcIF_sReference_t        *psReference,
                                phNfcIF_sCallBack_t         if_callback,
                                void                        *psIFConfig
                                );

We don’t want to appear whiny, but without the slightest documentation API like this is incomprehensible. Note the use of void* that is quite common in libnxp. What it is supposed to point to you can only derive by reverse engineering the JNI code. The Android JNI code was written by NXP people so they know their own library. To reverse engineer this would be a huge effort. The JNI code is used by millions of Android devices which in our opinion vets this code.

* Boehm is a conservative collector, but chances of leaks are statistically negligible. Note that Boehm is also used in dozens of other projects and can be regarded as a mature and reliable library. We might be able to replace this with some reference counting mechanism to remove Boehm.

* Regardless of the worker architecture under the hood we think we should at least land the DOM code to reduce the maintenance burden.

We should have a face-to-face discussion to discuss the details and how to proceed from here.

Comment 35 Andreas Gal :gal 2012-11-07 17:50:07 PST

I definitely agree on the DOM code part. We should get that factored out and landed. It should talk to a service that will implement the NFC backend (which is the piece we still have to figure out how to do exactly).

I agree on the poor quality of the API of libnxp. Generating those calls with ctypes could be painful. I did some experiments around that myself. I am attaching a code snippet I wrote to wrap the crappy library into a cleaner API. We could probably lift large parts of the JNI implementation for this wrapper code (basically copying the code, removing the JNI-isms along the way).

Sorry about the lack of architecture attention for this earlier. We are pretty focused on v1 features, so none of the core architects got around to taking a look here.

Let me know what you think about the copy & paste, remove jni-isms idea.

Comment 36 Andreas Gal :gal 2012-11-07 17:52:03 PST

The other two libraries are jansson-2.3.1/ and libatomic_ops/.

My key concern here is that we are pulling in 160k lines of libraries for 13k lines of actual code, of which more than 10k lines are JNI wrapper code. This doesn't look like a good deal as is.

Comment 37 Andreas Gal :gal 2012-11-07 17:52:29 PST

Created attachment 679491 [details]
bindings example

Comment 38 arno 2012-11-08 12:27:03 PST

Thanks for agreeing to land the DOM-specific portions of the NFC patch. We will update the API proposal as a first step to make this happen.

WRT to the nfcd specific comments, it seems that it boils down to a matter of style. You wrote that the JNI-isms should be removed and this could be accomplished by lifting large portions of the JNI code. You also made a comment that it is not good to have 10k lines of original contributions vs 160k lines of other code such as the JNI code or Boehm GC. Well, you could also look at this from a different perspective: there is nothing wrong to make use of other Open Source software. In fact, perhaps that is what one should do. We’d rather write only 10k lines than ‘reinventing’ the other 160k. The huge benefit of using the JNI code is maintainability: when you guys upgraded to ICS, all we had to do copy over the new JNI code of the NFC implementation in ICS. If we had followed your approach, all the work we would have done for pre-ICS would have to be redone. Likewise the same work would need to be redone for post-ICS updates and bug fixes to the JNI code.

Again, it looks like the discussion boils down to a matter of style. How do you suggest we proceed from here?

Comment 39 Garner Lee 2012-11-08 16:23:35 PST

Created attachment 679896 [details] [diff] [review]
NFC Gecko Feature (disabled state without compile flag)

I spoke with Kyle, the gecko side of landing the DOM mostly already exists (gonk-misc must enable  it via the --enable-b2g-nfc flag), and is okay to land that way. Over the last gecko patch, I've also added a small change in ipc to temporarily disable the remaining part of the NFC gecko implementation. I'll apply for "level 1" access.

Comment 40 Garner Lee 2012-11-20 15:14:21 PST

Created attachment 683765 [details] [diff] [review]
Proposed changes to have android system property to enable/disable nfc.

Comment 41 Garner Lee 2012-11-20 16:09:08 PST

Created attachment 683781 [details] [diff] [review]
Proposed changes to have B2G system property to enable/disable nfc.

Comment 42 Garner Lee 2012-12-07 14:24:47 PST

Created attachment 689930 [details] [diff] [review]
Same NFC gecko patch, on to merge with 12/6/2012 code

This nearly identical code to gecko_05.patch appears to cause constructor count issues now in nsDOMClassInfo.cpp at runtime with the matching 12/6/2012 gecko code. Adding code patch here for feedback/comment:

In debug only code block:

    for (i = 0; i < eDOMClassInfoIDCount; i++) {
      if (!sClassInfoData[i].u.mConstructorFptr ||
          sClassInfoData[i].mDebugID != i) {
        MOZ_NOT_REACHED("Class info data out of sync, you forgot to update "
                        "nsDOMClassInfo.h and nsDOMClassInfo.cpp! Fix this, "
                        "mozilla will not work without this fixed!");

        return NS_ERROR_NOT_INITIALIZED;
      }
    }

Comment 43 Garner Lee 2012-12-07 18:18:30 PST

Created attachment 690026 [details] [diff] [review]
Latest WebNFC gecko patch.

Fixed gecko over the last gecko_06.patch. Can pass DEBUG check on new xpcom object. (not fully working yet in gaia).

Comment 44 Garner Lee 2012-12-21 16:05:51 PST

Created attachment 695045 [details] [diff] [review]
NFC Gaia merge git.mozilla.org e421765cf2f739dbff0051f2f6b0c770e1e9cd51

Comment 45 Garner Lee 2012-12-21 16:06:50 PST

Created attachment 695046 [details] [diff] [review]
NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474 (release)

Comment 46 Garner Lee 2012-12-21 16:14:25 PST

The gecko DOM changes and matching gaia changes are uploaded (latest available code as of 12/21), and work well with the system daemon nfcd.

That daemon (nfcd) is not yet updated with the ref counting code.

Comment 47 Mozilla RelEng Bot 2013-01-04 17:15:54 PST

Try run for 760a45ecdcb7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=760a45ecdcb7
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-760a45ecdcb7

Comment 48 Vicamo Yang [:vicamo][:vyang] 2013-01-04 22:44:41 PST

Comment on attachment 695046 [details] [diff] [review]
NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474 (release)

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +224,5 @@
>    readonly attribute DOMString voicemailDisplayName;
> +
> +  nsIDOMDOMRequest iccOpenChannel(in nsIDOMWindow window, in DOMString aid);
> +  nsIDOMDOMRequest iccExchangeAPDU(in nsIDOMWindow window, in long channel, in jsval apdu);
> +  nsIDOMDOMRequest iccCloseChannel(in nsIDOMWindow window, in long channel);

Are these changes accidentally committed? You don't need them in this patch.

Comment 49 Mozilla RelEng Bot 2013-01-07 03:45:39 PST

Try run for 67e60cd31899 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=67e60cd31899
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-67e60cd31899

Comment 50 Garner Lee 2013-01-07 03:50:31 PST

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48)
> Comment on attachment 695046 [details] [diff] [review]
> NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474
> (release)
> 
> Review of attachment 695046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +224,5 @@
> >    readonly attribute DOMString voicemailDisplayName;
> > +
> > +  nsIDOMDOMRequest iccOpenChannel(in nsIDOMWindow window, in DOMString aid);
> > +  nsIDOMDOMRequest iccExchangeAPDU(in nsIDOMWindow window, in long channel, in jsval apdu);
> > +  nsIDOMDOMRequest iccCloseChannel(in nsIDOMWindow window, in long channel);
> 
> Are these changes accidentally committed? You don't need them in this patch.

Yes, new try submit below has it removed.

Comment 51 Mozilla RelEng Bot 2013-01-30 20:00:48 PST

Try run for ad675f3268e1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ad675f3268e1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-ad675f3268e1

Comment 52 Mozilla RelEng Bot 2013-01-31 15:15:40 PST

Try run for 47cd45afb5a1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=47cd45afb5a1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-47cd45afb5a1

Comment 53 Garner Lee 2013-02-01 18:15:39 PST

Created attachment 709320 [details] [diff] [review]
Latest gecko code, Mercurial modified version of the same patch.

Comment 54 Garner Lee 2013-02-01 18:18:45 PST

Try server comment submission: 
https://tbpl.mozilla.org/?tree=Try&rev=37556585cc69
The (linux) builds appear to be here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-37556585cc69/

The gecko side patch includes the secure element feature needed for mobile wallet implementation.

Comment 55 Vicamo Yang [:vicamo][:vyang] 2013-02-01 21:20:34 PST

Comment on attachment 709320 [details] [diff] [review]
Latest gecko code, Mercurial modified version of the same patch.

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

Hi Garner, I'm glad to review RIL parts, but for others like DOM, IPC, you should go for better reviewers like :mounir & :qDot.  Please separate the files into several patches.  Mine would likely be: ril_consts.js, ril_worker.js, RadioInterfaceLayer.js, RILContentHelper.js.  Besides, I want to know why does NFC need new APIs in RIL.  Even it does, it won't be in WebTelephony.  MobileConnection::icc is a better home for it.

Comment 56 Garner Lee 2013-02-07 14:12:36 PST

Created attachment 711512 [details] [diff] [review]
nfcd system daemon as a patch file with refcount memory management. (part 1)

"nfcd" is the system daemon side of the Nfc DOM patch. I sits between the NXP libraries and hardware, and the rest of the Gecko DOM. The new version implements refcounting for memory management, instead of using the garbage collector. The last specific reference concerning the switch was in Comment 38. Note: the size of the patch includes supporting the jannson JSON library (and test cases...)

Comment 57 Garner Lee 2013-02-07 14:14:46 PST

Created attachment 711514 [details] [diff] [review]
nfcd system daemon as a patch file with refcount memory management. (part 2)

Comment 58 Garner Lee 2013-02-07 15:46:17 PST

Comment on attachment 591592 [details]
API proposal

Marking old *.idl API proposal as obsolete, as they are being updated with new patches.

Comment 59 Garner Lee 2013-02-12 17:31:21 PST

Created attachment 713211 [details] [diff] [review]
NFC DOM Interface files

This Gecko DOM patch contains the interface changes needed to enable NFC.

Comment 60 Garner Lee 2013-02-12 17:34:16 PST

Created attachment 713215 [details] [diff] [review]
NFC DOM cpp/c/h implementation files.

This Gecko DOM implementation code for NFC IDL files.

Comment 61 Garner Lee 2013-02-12 17:36:06 PST

Attachment 713215 [details] [diff] also contains the System/gonk and IPC interface to a nfcd daemon that talks to the nfc hardware.

Comment 62 Garner Lee 2013-02-12 17:39:26 PST

Created attachment 713220 [details] [diff] [review]
NFC system/gonk and permission patch implemenation.

Comment 63 Garner Lee 2013-02-12 17:42:57 PST

Created attachment 713222 [details] [diff] [review]
NFC makefiles

Makefiles for nfc. There is additionally a system property that enables the compiled nfc feature at runtime (setprop nfc.enabled) in the previous attachment 683781 [details] [diff] [review].

Comment 64 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-02-26 00:34:25 PST

Needed for competitive parity.

Comment 65 Paul Theriault [:pauljt] 2013-03-18 21:02:13 PDT

There seems to be some movement on this bug - any idea when a security review might be needed for this feature (or any idea when this is expected to land) ?

Comment 66 Garner Lee 2013-03-18 21:34:21 PDT

I don't have a concrete idea of exactly when, but at last check, it'll be in "v.next". I'm preparing another set of patches to catch up to very recent changes to the Makefiles, as well as more platform integration, using MozActivity for nfc event dispatch.

Comment 67 Garner Lee 2013-03-27 18:26:03 PDT

Created attachment 730465 [details] [diff] [review]
NFC system daemon patch file (nfcd).

NFC system daemon patch file (nfcd). This patch removes Boem GC (obsolete), libatomic (obsolete), and jansson. Jansson, a JSON library, currently lives in a forked repository (github.com/svic/jansson) currently, with extra Android.mk files for compilation.

Comment 68 Garner Lee 2013-03-27 18:29:20 PDT

Created attachment 730467 [details] [diff] [review]
NFC gecko makefiles

Comment 69 Garner Lee 2013-03-27 18:33:12 PDT

Created attachment 730468 [details] [diff] [review]
NFC DOM cpp/c/h implementation files.

Comment 70 Garner Lee 2013-03-27 18:34:15 PDT

Created attachment 730470 [details] [diff] [review]
NFC DOM Interface files

Comment 71 Garner Lee 2013-03-27 18:35:45 PDT

Created attachment 730471 [details] [diff] [review]
NFC DOM JS, and permissions.

Comment 72 Garner Lee 2013-03-27 18:39:17 PDT

Created attachment 730473 [details] [diff] [review]
System app update. Platform integration with NFC MozActivities.

Comment 73 Garner Lee 2013-03-27 18:45:21 PDT

Created attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.

Comment 74 Vicamo Yang [:vicamo][:vyang] 2013-03-27 20:26:28 PDT

Comment on attachment 730470 [details] [diff] [review]
NFC DOM Interface files

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

::: dom/telephony/nsITelephonyProvider.idl
@@ +113,5 @@
> +
> +  // UICC SE API
> +  nsIDOMDOMRequest iccOpenChannel(in DOMString aid);
> +  nsIDOMDOMRequest iccExchangeAPDU(in long channel, in jsval apdu);
> +  nsIDOMDOMRequest iccCloseChannel(in long channel);

Aren't these APIs already landed?

Comment 75 Garner Lee 2013-03-28 10:39:16 PDT

Hi Vicamo,

Good point. I'll check the code origin, and reupload the patch as needed.

Comment 76 Garner Lee 2013-03-28 17:54:15 PDT

Created attachment 730997 [details] [diff] [review]
NFC DOM cpp/c/h implementation files.

Removed obsolete telephony Icc{Open,Close,ExchangeAPDU} in favor of IccManager.

Comment 77 Garner Lee 2013-03-28 17:55:55 PDT

Created attachment 730998 [details] [diff] [review]
Patch 1 (v1) - NFC DOM Interface files

Removed obsolete Icc{Open,Close,ExchangeAPDU} in telephony from last patch.

Comment 78 Garner Lee 2013-04-03 10:15:48 PDT

Comment on attachment 730471 [details] [diff] [review]
NFC DOM JS, and permissions.

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

Hi, Curtis, since you expressed interest last July, I put in some changes relevant to NFC security, and it's likely a good time to get some dialog started on the security design. If you're not the rignt person, (or I flagged it incorrectly) please let me know.

I note that there has been earlier non-security focused reviews of the code (Thanks to Andreas, Kyle, and Vicamo), although an official decision of sorts of how to move forward for reviewing and landing code, and where security review should be present in that process, will be fantastic.

Comment 79 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2013-04-03 10:38:42 PDT

(In reply to Garner Lee from comment #78)
> Comment on attachment 730471 [details] [diff] [review]
> NFC DOM JS, and permissions.
> 
> Review of attachment 730471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi, Curtis, since you expressed interest last July, I put in some changes
> relevant to NFC security, and it's likely a good time to get some dialog
> started on the security design. If you're not the rignt person, (or I
> flagged it incorrectly) please let me know.
I am the right person to get things rolling, however, the person that will actually review the code is Paul, as he owns the sec-review bug that is blocking this.
> 
> I note that there has been earlier non-security focused reviews of the code
> (Thanks to Andreas, Kyle, and Vicamo), although an official decision of
> sorts of how to move forward for reviewing and landing code, and where
> security review should be present in that process, will be fantastic.

Comment 80 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 13:47:27 PDT

Comment on attachment 730465 [details] [diff] [review]
NFC system daemon patch file (nfcd).

Obsoleting, we'll take care of this in bug 860907 since it's b2g specific.

Comment 81 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 14:02:07 PDT

Comment on attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.

Obsoleting and moving to bug 860910

Comment 82 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 14:02:36 PDT

Comment on attachment 730473 [details] [diff] [review]
System app update. Platform integration with NFC MozActivities.

Obsoleting and moving to bug 860910

Comment 83 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 14:05:23 PDT

Comment on attachment 683781 [details] [diff] [review]
Proposed changes to have B2G system property to enable/disable nfc.

Moving to bug 860907

Comment 84 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 14:05:58 PDT

Comment on attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.

Move to bug 860910

Comment 85 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 18:59:50 PDT

Created attachment 736600 [details] [diff] [review]
Patch 1 (v1) - NFC DOM IDL

Comment 86 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:00:41 PDT

Created attachment 736602 [details] [diff] [review]
Patch 2 (v1) - NFC DOM Boilerplate

Comment 87 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:01:27 PDT

Created attachment 736603 [details] [diff] [review]
Patch 3 (v1) - NFC DOM Implementation

Comment 88 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:03:14 PDT

Created attachment 736604 [details] [diff] [review]
Patch 4 (v1) - NFC Chrome Worker Implementation

Comment 89 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:03:56 PDT

Created attachment 736605 [details] [diff] [review]
Patch 5 (v1) - NFC Permissions Changes

Comment 90 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:04:42 PDT

Created attachment 736606 [details] [diff] [review]
Patch 6 (v1) - NFC Makefiles and Manifests

Comment 91 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:05:43 PDT

Created attachment 736607 [details] [diff] [review]
Patch 7 (v1) - NFC B2G IPC Implementation

There's a good chance this is getting rewritten as a UnixSocket implementation, but putting it up for completeness sake for the moment.

Comment 92 Kyle Machulis [:kmachulis] [:qdot] 2013-04-11 19:08:32 PDT

Ok. Resplit and reordered the patches into something a bit easier to divide up and review. A couple of small changes I made on the way through:

- For some reason the nsIDOMDOMRequest was added as an interface prototype in Telephony IDLs? Seemed unneeded since they weren't touched otherwise?

- layout/build makefile conflicted. Just shuffled position.

Comment 93 Garner Lee 2013-04-11 20:42:35 PDT

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #92)
> - For some reason the nsIDOMDOMRequest was added as an interface prototype
> in Telephony IDLs? Seemed unneeded since they weren't touched otherwise?

No reason anymore, nsIDOMDOMRequest is unneeded. The WebICC feature landed in mozilla-central, which meant there was a duplicate implementation in FFOS telephony that apparently is incompletely removed.

Comment 94 Kyle Machulis [:kmachulis] [:qdot] 2013-04-12 10:51:02 PDT

Ok, well, I went ahead and removed those in the above patch set.

BTW, assuming you don't want to rebuild the set by hand, I'm maintaining this git branch while we go through reviews:

https://github.com/qdot/mozilla-central/tree/webnfc-674741

Comment 95 Paul Theriault [:pauljt] 2013-04-14 21:42:49 PDT

Ok I should be able to start the security review this week. I am a little unsure of where to start though. Is there any useful documentation or can someone give me a brief overview (either in the bug, or ping me for a walk-through)?

Comment 96 Paul Theriault [:pauljt] 2013-04-15 00:49:57 PDT

Comment on attachment 736605 [details] [diff] [review]
Patch 5 (v1) - NFC Permissions Changes

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

::: extensions/cookie/Permission.txt
@@ +110,5 @@
>  
> +nfc
> +{}
> +WebNFC
> +Near Field Communications

You update is fine, but this file is out of date and needs to just go away. (bug 818767)

Comment 97 Paul Theriault [:pauljt] 2013-04-15 01:10:43 PDT

Comment on attachment 736604 [details] [diff] [review]
Patch 4 (v1) - NFC Chrome Worker Implementation

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

::: dom/system/gonk/Nfc.js
@@ +75,5 @@
> +    let message = event.data;
> +    debug("Received message: " + JSON.stringify(message));
> +    switch (message.type) {
> +      case "ndefDiscovered":
> +        ppmm.broadcastAsyncMessage("NFC:NdefDiscovered", message);

Is this OK to be broadcastAsyncMessage? The APIs that I have reviewed in the past mainly used sendAsyncMessage(aMsgName, aContent) to send the message to specific listener (at least I think that is what it does). Is a broadcast message sent to all apps, or only those apps which have registered to receive said message type. If its the latter, then I think its no problem, since you added the permissions check to dom/messages/SystemMessagePermissionsChecker.jsm, and on install only those apps with the 'nfc' permission can register to receive these messages. Mainly just asking this question to confirm my own understanding of the system message manager code.

Comment 98 Garner Lee 2013-04-15 02:20:55 PDT

(In reply to Paul Theriault [:pauljt] from comment #97)
> Comment on attachment 736604 [details] [diff] [review]
> Patch 4 (v1) - NFC Chrome Worker Implementation
> 
> Review of attachment 736604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/Nfc.js
> @@ +75,5 @@
> > +    let message = event.data;
> > +    debug("Received message: " + JSON.stringify(message));
> > +    switch (message.type) {
> > +      case "ndefDiscovered":
> > +        ppmm.broadcastAsyncMessage("NFC:NdefDiscovered", message);
> 
> Is this OK to be broadcastAsyncMessage? The APIs that I have reviewed in the
> past mainly used sendAsyncMessage(aMsgName, aContent) to send the message to
> specific listener (at least I think that is what it does). Is a broadcast
> message sent to all apps, or only those apps which have registered to
> receive said message type. If its the latter, then I think its no problem,
> since you added the permissions check to
> dom/messages/SystemMessagePermissionsChecker.jsm, and on install only those
> apps with the 'nfc' permission can register to receive these messages.
> Mainly just asking this question to confirm my own understanding of the
> system message manager code.

Hi Paul,

The DOM callback/listener code is restricted by the certified "nfc" permission as you understand. The object will be null otherwise if the app does not have it declared in the manfest. There's possibly more than one "certified" app with callbacks (and all of them will receive the callback), but that is also up to whoever is designing the device OS release image. If you know the common usecase is to allow us to restrict it to one callback only, I can look into that. I'm not as familiar with sendAsyncMessage, but the broadcast what RIL used in the past. The NFC code is based on that code originally.

Comment 99 Garner Lee 2013-04-16 05:36:32 PDT

Created attachment 737939 [details] [diff] [review]
Patch 5 (v2) - NFC Permissions Changes

Remove obsolete Permissions.txt changes.

Comment 100 Garner Lee 2013-04-23 13:22:03 PDT

Created attachment 740998 [details] [diff] [review]
Patch 1 (v2) - NFC DOM IDL

Update jsval field to nsString

Comment 101 Garner Lee 2013-04-23 13:24:35 PDT

Created attachment 740999 [details] [diff] [review]
Patch 3 (v2) - NFC DOM Implementation

Update jsval field to nsString.

Comment 102 Garner Lee 2013-04-23 13:26:55 PDT

Created attachment 741004 [details] [diff] [review]
Patch 4 (v2) - NFC Chrome Worker Implementation

Update jsval field to nsString.

Comment 103 Kyle Machulis [:kmachulis] [:qdot] 2013-04-29 12:05:32 PDT

Comment on attachment 736607 [details] [diff] [review]
Patch 7 (v1) - NFC B2G IPC Implementation

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

So this is mostly an r- because of design changes since this was first implemented. We've moved most of the functions in here to UnixSocket.h/.cpp, since multiple components (ril, bluetooth, etc...) were copy functions from each other. NFC should also move to UnixSocket. That said, this move should /drastically/ reduce the amount of code in these patches. You should be able to use the current RIL implementation as a guide to do this, as you did the first time around. Basically, you're going to need a "Connector" class that sets up the socket address struct, then possibly a couple of small utility functions to ferry packets to your worker. Let me know if you have any questions.

Comment 104 Kyle Machulis [:kmachulis] [:qdot] 2013-04-29 16:39:24 PDT

Comment on attachment 740998 [details] [diff] [review]
Patch 1 (v2) - NFC DOM IDL

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

::: dom/nfc/nsIDOMMozNdefRecord.idl
@@ +8,5 @@
> +interface nsIDOMMozNdefRecord : nsISupports {
> +  readonly attribute octet tnf;
> +  readonly attribute DOMString type;
> +  readonly attribute DOMString id;
> +  readonly attribute jsval payload;

Comments in general are good when it's not obvious what the fields are, doubly so when it's a jsval.

::: dom/nfc/nsIDOMNavigatorNfc.idl
@@ +7,5 @@
> +interface nsIDOMNfc;
> +
> +[scriptable, uuid(c5814d20-4dcf-11e1-b86c-0800200c9a66)]
> +interface nsIDOMNavigatorNfc : nsISupports {
> +

nit: Kill blank lines

::: dom/nfc/nsIDOMNfcNdefEvent.idl
@@ +9,5 @@
> +[scriptable, builtinclass, uuid(41fcd640-4dd4-11e1-b86c-0800200c9a66)]
> +interface nsIDOMNfcNdefEvent : nsIDOMEvent
> +{
> +  [implicit_jscontext]
> +  readonly attribute jsval ndefMessages;

Comment on what type ndefMessages are would be helpful. Guessing it's just records?

Comment 105 Kyle Machulis [:kmachulis] [:qdot] 2013-04-29 16:40:59 PDT

Comment on attachment 740998 [details] [diff] [review]
Patch 1 (v2) - NFC DOM IDL

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

::: dom/nfc/nsIDOMMozNdefRecord.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, builtinclass, uuid(42a70160-c4a3-11e1-9b21-0800200c9a66)]
> +interface nsIDOMMozNdefRecord : nsISupports {

Any reason why this is MozNdefRecord but everything else isn't prefixed?

Comment 106 Kyle Machulis [:kmachulis] [:qdot] 2013-04-29 16:45:31 PDT

Comment on attachment 736602 [details] [diff] [review]
Patch 2 (v1) - NFC DOM Boilerplate

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

::: content/base/src/nsGkAtomList.h
@@ +722,4 @@
>  GK_ATOM(onlevelchange, "onlevelchange")
>  GK_ATOM(onLoad, "onLoad")
>  GK_ATOM(onload, "onload")
> +GK_ATOM(onndefdiscovered, "onndefdiscovered")

Don't these also need #ifdef MOZ_B2G_NFC?

::: dom/base/Navigator.cpp
@@ +1465,5 @@
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk NFC", args)
> +#else
> +#include <android/log.h>
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk NFC", args)
> +#endif

I don't think this is gonna be a welcomed addition to navigator. Remove the debug messages and use an ifdef'd DEBUG NS_WARNING() if you need to track messages in debug.

Comment 107 Garner Lee 2013-04-29 17:44:38 PDT

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #105)
> Comment on attachment 740998 [details] [diff] [review]
> Patch 1 (v2) - NFC DOM IDL
> 
> Review of attachment 740998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/nsIDOMMozNdefRecord.idl
> @@ +4,5 @@
> > +
> > +#include "nsISupports.idl"
> > +
> > +[scriptable, builtinclass, uuid(42a70160-c4a3-11e1-9b21-0800200c9a66)]
> > +interface nsIDOMMozNdefRecord : nsISupports {
> 
> Any reason why this is MozNdefRecord but everything else isn't prefixed?

No strong reason, (Moz)NdefRecord stands on it's own, but it takes after MozActivity.Any preference either way?

Comment 108 Garner Lee 2013-04-30 16:05:24 PDT

Created attachment 743918 [details] [diff] [review]
Patch 2 (v2) - NFC DOM Boilerplate

Implement review comments: #ifdef for 2 onndefdiscovered, onndefdisconnected, rename MozNdefRecord to NdefRecord.

Comment 109 Garner Lee 2013-04-30 16:42:15 PDT

Created attachment 743943 [details] [diff] [review]
Patch 1 (v3) - NFC DOM IDL

Implement review comments: Comment NdefRecord fields, MozNdefRecord --> NdefRecord

Comment 110 Garner Lee 2013-04-30 16:45:05 PDT

Created attachment 743944 [details] [diff] [review]
Patch 3 (v3) - NFC DOM Implementation

Implement review comments: MozNdefRecord --> NdefRecord

Comment 111 Garner Lee 2013-04-30 16:48:55 PDT

Created attachment 743947 [details] [diff] [review]
Patch 6 (v2) - NFC Makefiles and Manifests

Implement review comments: Rename MozNdefRecord --> NdefRecord

Comment 112 Kyle Machulis [:kmachulis] [:qdot] 2013-04-30 21:17:19 PDT

You, uh, might want to wait 'til I reply to questions before changing things. I haven't had a chance to check with sicking or mrbkap yet about our naming prefs. Will hope I was right on that for now.

Comment 113 Mounir Lamouri (:mounir) 2013-05-01 07:21:38 PDT

(In reply to Garner Lee from comment #107)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #105)
> > Comment on attachment 740998 [details] [diff] [review]
> > Patch 1 (v2) - NFC DOM IDL
> > 
> > Review of attachment 740998 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/nfc/nsIDOMMozNdefRecord.idl
> > @@ +4,5 @@
> > > +
> > > +#include "nsISupports.idl"
> > > +
> > > +[scriptable, builtinclass, uuid(42a70160-c4a3-11e1-9b21-0800200c9a66)]
> > > +interface nsIDOMMozNdefRecord : nsISupports {
> > 
> > Any reason why this is MozNdefRecord but everything else isn't prefixed?
> 
> No strong reason, (Moz)NdefRecord stands on it's own, but it takes after
> MozActivity.Any preference either way?

nsIDOMFoo should always be prefixed because those are interfaces exposed to the Web and visible on the global scope (window.Foo). The "nsIDOM" part is the thing doing the magic. If you don't want to expose your interface to the Web (ie. in the global scope), you should just do NdefRecord, not nsIDOMMozNDefRecord.

Comment 114 Mounir Lamouri (:mounir) 2013-05-01 07:28:59 PDT

Garner, I have a few comments about those reviews:
- you should ask the Makefile/Manifest review to a build config peer;
- same for the dom patches, the reviews should be addressed by a DOM peer;
- you might want to ask bent to review the IPC and Worker-specific code (he is also a DOM peer ;)).

Regarding the general infrastructure of the code, I wonder why you didn't use hal/ instead of dom/system/? Also, you should use WebIDL: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Note that you can ask for a review to anybody for any patch but the review will be good enough to land only if the reviewer is a peer of the associated module.

Comment 115 Kyle Machulis [:kmachulis] [:qdot] 2013-05-01 08:10:20 PDT

Thanks for the update on the Moz prefixing. I couldn't remember the rules.

I'm doing the first round of reviews because we're so overbooked on reviews for things landing in v1.0/v1.1 right now. I've got mrbkap as the DOM peer, and the IPC code is based off my original RIL code so I'm handling that (Especially since it's gonna shrink a bunch). We're WAY backed up on this review so it just needs to start making some progress, but certainly won't land before the proper peers see it.

It wasn't done in hal/ because the code was modeled off RIL and most of that portion was written almost a year ago now.

Most of this was written far before WebIDL had landed. We've already started talking about the conversion, but we're just trying to get through this first review right now.

Comment 116 Garner Lee 2013-05-01 10:17:22 PDT

Thanks for the updates.

I can reasonably easily back out the MozNdefRecord change to go either way (it's in one commit), and it seems there's enough churn that we can get it on the next patch update as needed.

Apps with certified nfc permissions can do "new NdefRecord(args...)" if they need to write new tags. Though it doesn't necessarily have to be that way.

If that can be thought of as exposed to "the web", I will certainly change that back to MozNdefRecord naming.

Comment 117 Kyle Machulis [:kmachulis] [:qdot] 2013-06-03 14:35:40 PDT

BTW, we should probably break patch 7 out into its own bug once you've got the UnixSocket stuff ready to go. We can land the IPC portion without waiting on the DOM if it gets done soon.

Comment 118 psiddh 2013-06-04 17:09:36 PDT

Created attachment 758311 [details] [diff] [review]
Patch 7 (v2) - NFC B2G IPC Implementation

Comment 119 psiddh 2013-06-04 17:22:31 PDT

Created attachment 758319 [details] [diff] [review]
Patch 4 (v3) - NFC Chrome Worker Implementation

Patch 4 :  NFC Chrome Worker Implementation v3 - Modifies the NFC chrome worker to support UnixSocket implementation.

Comment 120 psiddh 2013-06-04 17:29:20 PDT

Kyle, Please mark the patch "Patch 7 (v1) - NFC B2G IPC Implementation" OBSOLETE (I am unable to do it myself) as v2 of the 'Patch 7' is updated to incorporate 
Unix Socket changes.

Comment 121 Kyle Machulis [:kmachulis] [:qdot] 2013-06-04 19:44:11 PDT

Actually, can you go ahead and make Patch 7 v2 another bug completely, blocking 860906? I have a feeling we'll be able to land the IPC before we land the DOM stuff since we've got to look at WebIDLizing anyways. I'll mark v1 obsolete also.

Comment 122 psiddh 2013-06-05 09:22:01 PDT

Here is the new bug that tracks NFC IPC changes : https://bugzilla.mozilla.org/show_bug.cgi?id=879821

Comment 123 Kyle Machulis [:kmachulis] [:qdot] 2013-06-26 14:38:32 PDT

Comment on attachment 743918 [details] [diff] [review]
Patch 2 (v2) - NFC DOM Boilerplate

Removing request for review while this is refactored into WebIDL by implementors.

Comment 124 Kyle Machulis [:kmachulis] [:qdot] 2013-06-26 14:39:49 PDT

Comment on attachment 743943 [details] [diff] [review]
Patch 1 (v3) - NFC DOM IDL

Removing review requests while code is being refactored for WebIDL. Also, will require concrete use cases before proper review can take place.

Comment 125 Kyle Machulis [:kmachulis] [:qdot] 2013-06-26 14:40:47 PDT

Comment on attachment 743944 [details] [diff] [review]
Patch 3 (v3) - NFC DOM Implementation

Removing request for review while this is refactored into WebIDL by implementors.

Comment 126 Kyle Machulis [:kmachulis] [:qdot] 2013-06-26 14:41:34 PDT

Comment on attachment 743947 [details] [diff] [review]
Patch 6 (v2) - NFC Makefiles and Manifests

Removing request for review while refactors are happening that will no doubt change the build files

Comment 127 Kyle Machulis [:kmachulis] [:qdot] 2013-06-26 14:44:34 PDT

Comment on attachment 758319 [details] [diff] [review]
Patch 4 (v3) - NFC Chrome Worker Implementation

Removing request for review while waiting for WebIDL refactor, since that may change communications portions of worker.

Comment 128 Garner Lee 2013-06-27 10:29:40 PDT

Created attachment 768416 [details] [diff] [review]
Patch 1 (v4) - NFC DOM IDL

Added WebIDL support for NfcEvent object.

Comment 129 Garner Lee 2013-06-27 10:30:34 PDT

Created attachment 768417 [details] [diff] [review]
Patch 2 (v3) - NFC DOM Boilerplate

Comment 130 Garner Lee 2013-06-27 10:31:25 PDT

Created attachment 768419 [details] [diff] [review]
Patch 3 (v4) - NFC DOM Implementation

Comment 131 Garner Lee 2013-06-27 10:32:10 PDT

Created attachment 768420 [details] [diff] [review]
Patch 4 (v4) - NFC Chrome Worker Implementation

Comment 132 Garner Lee 2013-06-27 10:33:03 PDT

Created attachment 768421 [details] [diff] [review]
Patch 5 (v3) - NFC Permissions Changes

Comment 133 Garner Lee 2013-06-27 10:33:52 PDT

Created attachment 768423 [details] [diff] [review]
Patch 6 (v3) - NFC Makefiles and Manifests

Comment 134 Garner Lee 2013-07-08 13:46:32 PDT

Created attachment 772296 [details]
This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.

Comment 135 Garner Lee 2013-07-12 18:11:50 PDT

Created attachment 775053 [details]
(v2) This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.

NDEF Protocol updated with additional states.

Comment 136 Garner Lee 2013-07-15 16:52:01 PDT

Created attachment 776039 [details] [diff] [review]
Patch 4 (v5) - NFC Chrome Worker Implementation

Update for new worker API (7/12 merge).

Comment 137 Garner Lee 2013-07-15 16:54:47 PDT

"Patch 4 (v5) - NFC Chrome Worker Implementation" does not have the new NFC JSON API. That will come soon.

Comment 138 psiddh 2013-07-16 11:10:15 PDT

Created attachment 776536 [details]
(v1) This pdf describes the NFC architecture and the responsibilities shared between various teams.

Comment 139 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-18 02:08:08 PDT

(In reply to Siddartha P from comment #138)
> Created attachment 776536 [details]
> (v1) This pdf describes the NFC architecture and the responsibilities shared
> between various teams.

Hi,

Thanks for attaching this document. Some questions:

- Communication is always initiated by the nfcd with a TechDiscoveredNotification? Gonk then has the possibility to use the session by sending a ConnectRequest.
- The session id is generated randomly by nfcd?
- In the diagrams on page 7ff it is hard to understand which component performs which action. Can you provide separate diagrams for gonk and nfcd, and some protocol diagrams as well?

Comment 140 Yoshi Huang[:allstars.chh] 2013-07-18 04:40:43 PDT

My questions are :

1. What the id used for in BasePDU?
Can you provide some example?

2. Should 'Tech Discovered' rename to 'Tag Discovered', and 'NfcA Tag Discovered' rename to 'NfcA Tech Discovered'?

Or What's the difference between a 'Tag is discovered' or 'a Tech is discovered'?

3. You have a option can_be_made_readonly in NDEFDetailsRsp, but no PDU to enable readonly, right?

thanks

Comment 141 arno 2013-07-18 10:14:09 PDT

first of all, please make sure you are looking at v1.1 of the document. Answers inline:

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #139)

> - Communication is always initiated by the nfcd with a
> TechDiscoveredNotification? Gonk then has the possibility to use the session
> by sending a ConnectRequest.

Yes. You can only interact if a NFC tag is in the proximity of the reader. In this case nfcd sends a TechDiscoveredNotification upon which Gonk can trigger further interactions.

> - The session id is generated randomly by nfcd?

Yes. It needs to be unique. Also note (as noted in the specs) that if you bring the same tag to the reader twice, a different session id should be generated. Gonk will just echo the session ID in the actions it wishes to trigger.

> - In the diagrams on page 7ff it is hard to understand which component
> performs which action. Can you provide separate diagrams for gonk and nfcd,
> and some protocol diagrams as well?

the state machine describes the distributed state of nfcd and Gonk. E.g., the "Pending" states denote a state where Gonk triggered an action (such as a connect) but nfcd has not yet responded. I'm not sure that breaking up the state machine for one specific to nfcd and one for Gonk will be helpful. It would be a lot more difficult to correlate transitions. Keep in mind that "Notifications" will always be sent from nfcd to Gonk. "Requests" always from Gonk to nfcd and "Responses" always from nfcd to Gonk. This should make it easier to follow the flow.

Comment 142 arno 2013-07-18 10:53:14 PDT

answers inline:

> 1. What the id used for in BasePDU?
> Can you provide some example?

BasePDU is just used to explain the marshalling. It is meant as an abstract example and the properties of BasePDU and SomePDU have no relevance to NFC.


> 2. Should 'Tech Discovered' rename to 'Tag Discovered', and 'NfcA Tag
> Discovered' rename to 'NfcA Tech Discovered'?

We have chosen the term "Tech Discovered" because it also covers the P2P case (when you hold a second NFC-enabled phone next to yours). In this case the second phone technically is not a "Tag". "Technology" is a broader term that includes both tags and P2P phones.

> Or What's the difference between a 'Tag is discovered' or 'a Tech is
> discovered'?

If it says somewhere "Tag discovered", it is a typo.

> 3. You have a option can_be_made_readonly in NDEFDetailsRsp, but no PDU to
> enable readonly, right?

Good catch. We propose to add "make_read_only:boolean" field to the NDEFWriteRequest PDU.

Comment 143 Yoshi Huang[:allstars.chh] 2013-07-18 17:28:31 PDT

(In reply to arno from comment #142)
> answers inline:
> 
> > 1. What the id used for in BasePDU?
> > Can you provide some example?
> 
> BasePDU is just used to explain the marshalling. It is meant as an abstract
> example and the properties of BasePDU and SomePDU have no relevance to NFC.
> 
Thanks for explaining.
But what I meant is the 'id' property in BasePDU,
I don't understand its usage and what it's for.

Thanks

Comment 144 arno 2013-07-18 17:37:44 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #143)
> But what I meant is the 'id' property in BasePDU,
> I don't understand its usage and what it's for.

BasePDU is just an example. It is NOT part of the nfcd/Gonk protocol.

Comment 145 Yoshi Huang[:allstars.chh] 2013-07-19 02:01:30 PDT

Hi, Anro
I got further questions for the communication protocol

1. The data passed to nfcd should be native binary data, so your proposal should be presented in some C struct format or CPP class. 

So in your nfc_worker, it should do as ril_worker does, convert the json to native format.

Otherwise I think it doesn't make sense that nfcd still needs to covert the binary data to JS object and use some JSON API to extract the data.

2. You didn't expose the detail of NdefMsg.

3. In ReadReq/Rsp, WriteReq/Rsp, and CloseReq/Rsp, they all use the same session_id founded in TechDiscovered, right? But you didn't mention this in the proposal.

4. As I said earlier, no PDU for enable readonly.

5. For WriteRsp, there is no error code for the tag is set to 'readonly', nor error code for the size of Ndef is exeeded the max_ndef_size.

6. NfcATagXXX should be renamed to NfcATechXXX since the other phone (which is not a tag) could also use 'NfcA', right?

7. What's the case when the error code is 'Bag Tag ID', because I saw no requests could specify the 'tag id' in the proposal.

8. Since you maintained this by session_id, is there a error code for 'session id not found' ?

9. In CloseRsp, what's the difference between the error code 'Not Connected' and 'Lost Tag' ?

And I will check the spec of NfcA and reply to you if I get any question. 

Thanks

Comment 146 Yoshi Huang[:allstars.chh] 2013-07-19 04:11:55 PDT

10. There should be also error codes for ReadRsp, WriteRsp.

11. For NfcATagTransceiveResponse, there should be a error code for timeout.

12. For NfcATagDetailsResponse, can it get timeout value from the tag? And can the timeout configurable ?
Can we reset the timeout when NfcATransceivePending?
   

Also some nits,
For Nfef, it's max_ndef_'size',
whereas in NfcA, it's max_transceive_'len',
I think we should choose one in between.

I'd also suggest use the same style to name those variables, either foo_bar or fooBar.

And one last tiny thing, For Javascript, the object properties are seperated by comma (,), not by semicolons(;).

Comment 147 Yoshi Huang[:allstars.chh] 2013-07-19 04:15:59 PDT

Also I think you already got some demos working, right? But how does your 'gecko' link/ipc to those JNI wrappers?
I don't think your nfc_worker could generate some JNIEnv data structure, right?

Thanks

Comment 148 Garner Lee 2013-07-19 11:27:54 PDT

Hi Yoshi,

Yes, the JNIEnv NXP stuff is only in the current nfcd side of the implmentation, and is not exposed to the upper layers in gecko/gonk (gecko/ipc <--> ipc/nfcd/JNIEnv). The NXP implemenation originally was targeting Android/Java.

Comment 149 Yoshi Huang[:allstars.chh] 2013-07-19 21:46:56 PDT

Hi, Garner
So how does your proposal for this architecture work anyway?

Your reference implementation of nfcd uses the JNI to wrap those libnfc-nxp functions,
Can you show us how does 'your gecko' connect to the nfcd actually?
By 'your gecko' I mean in your own internal branch of b2g.

This bug is created for almost two years and I think you already have some of demos working, right?

Again, in your own implementation, how does you 'WebNFC' API link to native libnfc-nxp ?
The JNI one isn't a reasonable way that could possibly work here.

Or in case I am wrong, can you please describe how you call these JNI functions in more detail?

Thanks

Comment 150 Garner Lee 2013-07-19 22:33:18 PDT

The current goals under discussion is for Mozilla to implement the hardware interface layer under "gonk" via a chrome worker (see patch 4, v5), and the IPC layer is used to abstract away the daemon running on the system, so it doesn't link directly. It works the same way as RIL does at the high level.

We have a nfc demo (publicly on github.com/svic/gaia), and a simulator build (not uploaded yet) that also works without hardware (nor does it use JNI). The current implementation worked only for the older Nexus-S NFC chipset, as a chipset vendor must first be selected, and then implemented.

In that way, you are correct, the JNI-isms used for the wrapper isn't required, and can be replaced if a solution exists for the selected chipset. The JNIEnv emulation wrapper was used because of the issues with using the NFC and NXP C library in a straightforward manner (see earlier bug comments). Some newer NFC solutions may have the more recent C Native Controller Interface (NCI), which will help abstract NFC implementations further down the stack, which will be a rather good thing to have long term from the hardware maintenance perspective.

Comment 151 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-22 02:47:25 PDT

Hi,

Bug 838146 recently landed and the NFC patches don't apply any longer. Can you rewrite the bindings in WebIDL instead and rebase the code on top of the latest m-c, please? Thanks.

Ps: Meta bug 580070 tracks all the WebIDL changes.

Comment 152 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-22 06:38:46 PDT

FYI: I applied the patches here and the patches in bug 860910 to a slightly rev of gecko, but booting fails with:

> F/MOZ_CRASH(  689): Hit MOZ_CRASH(Class info data without an interface list! Fix this, mozilla will not work without this fixed!) at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/base/nsDOMClassInfo.cpp:1786
> F/libc    (  689): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)

in the logcat.

Comment 153 Garner Lee 2013-07-22 15:44:39 PDT

Hi Thoamas. Yes, that's a rather large change to WebIDL in navigator. Is this a good template for the conversion (RIL)? https://bug838146.bugzilla.mozilla.org/attachment.cgi?id=773272, also, who's a good person to contact if I have questions on the WebIDL conversion?

Comment 154 arno 2013-07-22 15:50:23 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #145)

> 1. The data passed to nfcd should be native binary data, so your proposal
> should be presented in some C struct format or CPP class. 
> 
> So in your nfc_worker, it should do as ril_worker does, convert the json to
> native format.
> 
> Otherwise I think it doesn't make sense that nfcd still needs to covert the
> binary data to JS object and use some JSON API to extract the data.

Note that we use an abstract syntax to describe the structure of the PDUs. This abstract syntax can be marshalled in different ways. While we use JSON for our reference implementation, you can easily define a binary marshalling (it is similar to ASN.1 and BER). We should make this discussion part of the NFC work week.

> 2. You didn't expose the detail of NdefMsg.

Done.

> 3. In ReadReq/Rsp, WriteReq/Rsp, and CloseReq/Rsp, they all use the same
> session_id founded in TechDiscovered, right? But you didn't mention this in
> the proposal.

Yes, it is always the same session_id. The slide describing the Base PDU explains the semantics of session_id.

> 4. As I said earlier, no PDU for enable readonly.

Done.

> 5. For WriteRsp, there is no error code for the tag is set to 'readonly',
> nor error code for the size of Ndef is exeeded the max_ndef_size.

In the updated protocol specs I've imported all error codes defined in android.nfc.ErrorCodes that should cover this.

> 6. NfcATagXXX should be renamed to NfcATechXXX since the other phone (which
> is not a tag) could also use 'NfcA', right?

To our knowledge, a different protocol is used for P2P (either LLCP or SNEP). NFC-A refers to a tag type so we believe "Tag" is still an appropriate nomenclature.

> 7. What's the case when the error code is 'Bag Tag ID', because I saw no
> requests could specify the 'tag id' in the proposal.

E.g., if you try to do a NDEFReadRequest on an NFC-A tag that has no NDEF message. Technically you are also violating the protocol state machine.

> 8. Since you maintained this by session_id, is there a error code for
> 'session id not found' ?

Good point. We added this.

> 9. In CloseRsp, what's the difference between the error code 'Not Connected'
> and 'Lost Tag' ?

Lost Tech means that the tag/phone was physically removed from the reader field. This can happen at any point in time. We mirrored the Android API where you first have to connect to a tag/technology before you can do any operations. E.g., if you try to do a transceive before connecting to a NFC-A tag, you should get this error.

Comment 155 arno 2013-07-22 15:59:28 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #146)

> 10. There should be also error codes for ReadRsp, WriteRsp.

Done.

> 11. For NfcATagTransceiveResponse, there should be a error code for timeout.

Done.

> 12. For NfcATagDetailsResponse, can it get timeout value from the tag? And
> can the timeout configurable ?
> Can we reset the timeout when NfcATransceivePending?

You can set different timeouts for each transceive() (see NfcATagTransceiveRequest). You cannot change the timeout while you are in NfcATransceivePending. You should use an appropriate timeout value with the initial request.

> Also some nits,
> For Nfef, it's max_ndef_'size',
> whereas in NfcA, it's max_transceive_'len',
> I think we should choose one in between.

I renamed both to 'len'.

> I'd also suggest use the same style to name those variables, either foo_bar
> or fooBar.

I changed all identifiers to camel case.

> And one last tiny thing, For Javascript, the object properties are seperated
> by comma (,), not by semicolons(;).

Since the abstract syntax has no relationship to JS, I'll keep the semicolon. :)

Comment 156 Garner Lee 2013-07-22 19:05:44 PDT

Created attachment 779560 [details]
(v3) This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.

Comment 157 Gabriele Svelto [:gsvelto] 2013-07-22 23:26:56 PDT

(In reply to Garner Lee from comment #153)
> Is this a good template for the conversion (RIL)?
> https://bug838146.bugzilla.mozilla.org/attachment.cgi?id=773272

Yes, it should be a good starting point; many other components have been converted recently so you should find plenty of examples too. Note that we also have pretty extensive documentation on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

> also, who's
> a good person to contact if I have questions on the WebIDL conversion?

Andrea Marchesini (:baku) has converted *lots* of components to WebIDL lately so he should be a good person to ask.

Comment 158 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-23 00:43:28 PDT

(In reply to Garner Lee from comment #153)
> Hi Thoamas. Yes, that's a rather large change to WebIDL in navigator. Is
> this a good template for the conversion (RIL)?
> https://bug838146.bugzilla.mozilla.org/attachment.cgi?id=773272, also, who's
> a good person to contact if I have questions on the WebIDL conversion?

Hi Garner,

In addition to what Gabriel already said, there is the W3C standard [1]. Our docs at [2] suggest the fix for bug 749101 as reference [3].

Best regards
Thomas

[1] http://www.w3.org/TR/WebIDL/
[2] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
[3] https://hg.mozilla.org/mozilla-central/rev/dd08c10193c6

Comment 159 Garner Lee 2013-07-23 15:47:32 PDT

Created attachment 780052 [details] [diff] [review]
Patch 1 (v5) - NFC DOM IDL

Comment 160 Garner Lee 2013-07-23 15:48:23 PDT

Created attachment 780053 [details] [diff] [review]
Patch 2 (v4) - NFC DOM Boilerplate

Comment 161 Garner Lee 2013-07-23 15:49:09 PDT

Created attachment 780054 [details] [diff] [review]
Patch 3 (v5) - NFC DOM Implementation

Comment 162 Garner Lee 2013-07-23 15:50:23 PDT

Created attachment 780055 [details] [diff] [review]
Patch 5 (v4) - NFC Permissions Changes

Comment 163 Garner Lee 2013-07-23 15:51:05 PDT

Created attachment 780056 [details] [diff] [review]
Patch 6 (v4) - NFC Makefiles and Manifests

Comment 164 Yoshi Huang[:allstars.chh] 2013-07-23 19:49:22 PDT

This bug should be focus on WebAPI level.

I filed Bug 897312, we should move the discussion of the interaction of nfcd<->b2g there.

Thanks

Also @Garner, for the patches you just uploaded, so we could just apply those patches and build with latest gecko, then it should work(I mean no crash), right?

Also in your github (svic), the mozilla-central repository has some branches, are those branches the latest update from you?
Can you show us which branch could be useful to us?

Thanks

Comment 165 Garner Lee 2013-07-23 20:42:58 PDT

The new patches converts the navigator.mozNfc to use WebIDL, and should work without crashes I believe, or at least unblock people wanting to run the code on the latest gecko. It's possible I missed a crash assertion signal 11 message, if so, I'll check soon. I do know there's some graphic rendering glitches on nexus-s (some work being done on gralloc I think).

More notes in the new Bug 897312.

Comment 166 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-24 03:55:09 PDT

(In reply to Garner Lee from comment #165)
> The new patches converts the navigator.mozNfc to use WebIDL, and should work
> without crashes I believe, or at least unblock people wanting to run the
> code on the latest gecko. It's possible I missed a crash assertion signal 11
> message, if so, I'll check soon. I do know there's some graphic rendering
> glitches on nexus-s (some work being done on gralloc I think).

Thanks for rebasing the patch set. It applies cleanly now, but it still doesn't work for me. Did you build with debugging enabled?

I use mozilla-central. I had to remove FORCE_STATIC_LIB from dom/nfc/Makefile.in to make the patches compile. The crash of comment 152 still happens during boot. And if you don't set --enable-b2g-nfc, some of the code fragments don't build.

Best regards
Thomas

Comment 167 Yoshi Huang[:allstars.chh] 2013-07-24 20:55:28 PDT

Comment on attachment 780052 [details] [diff] [review]
Patch 1 (v5) - NFC DOM IDL

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

::: dom/nfc/nsIDOMNfc.idl
@@ +23,5 @@
> +  [implicit_jscontext]
> +  attribute jsval onsecureelementdeactivated;
> +
> +  [implicit_jscontext]
> +  attribute jsval onsecureelementtransaction;

Hi, Garner
Do you think we could remove these SE-related interface first,
and we could add it back in Bug 884594?

Also it seems you miss 'TechLost'(or 'TagLost') callback here.

And what's your plan to make these patches reviewed and landed?

Thanks

Comment 168 Garner Lee 2013-07-25 11:48:54 PDT

Created attachment 781128 [details] [diff] [review]
Patch 1 (v6) NFC DOM IDL/WebIDL

Fixes reported crash issue in DEBUG compile (remove extra NfcEvent registration).

Comment 169 Garner Lee 2013-07-25 11:49:58 PDT

Created attachment 781129 [details] [diff] [review]
Patch 2 (v5) - NFC DOM Boilerplate

Comment 170 Garner Lee 2013-07-25 11:50:54 PDT

Created attachment 781130 [details] [diff] [review]
Patch 3 (v6) - NFC DOM Implementation

Comment 171 Garner Lee 2013-07-25 11:52:28 PDT

Created attachment 781132 [details] [diff] [review]
Patch 4 (v6) - NFC Chrome Worker Implementation

Remove Secure Element API (now tracked in separate feature bug).

Comment 172 Garner Lee 2013-07-25 11:53:18 PDT

Created attachment 781133 [details] [diff] [review]
Patch 5 (v5) - NFC Permissions Changes

Comment 173 Garner Lee 2013-07-25 11:54:09 PDT

Created attachment 781135 [details] [diff] [review]
Patch 6 (v5) - NFC Makefiles and Manifests

Comment 174 Garner Lee 2013-07-25 11:59:45 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #167)
> Comment on attachment 780052 [details] [diff] [review]
> Patch 1 (v5) - NFC DOM IDL
> 
> Review of attachment 780052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/nsIDOMNfc.idl
> @@ +23,5 @@
> > +  [implicit_jscontext]
> > +  attribute jsval onsecureelementdeactivated;
> > +
> > +  [implicit_jscontext]
> > +  attribute jsval onsecureelementtransaction;
> 
> Hi, Garner
> Do you think we could remove these SE-related interface first,
> and we could add it back in Bug 884594?
> 
> Also it seems you miss 'TechLost'(or 'TagLost') callback here.
> 
> And what's your plan to make these patches reviewed and landed?
> 
> Thanks

Hi Yoshi,

We're going to introduce the TechLost renames (and new API additions) in the next bug update, landed or not, provided nothing else needs attention in M-C master. I'm not sure about the DOM review schedule however...

Comment 175 Garner Lee 2013-07-25 12:04:29 PDT

(In reply to Garner Lee from comment #173)
> Created attachment 781135 [details] [diff] [review]
> Patch 6 (v5) - NFC Makefiles and Manifests

I just noticed the IPC cpp change probably should be in a separate patch, but was required to not abort in DEBUG mode (RIL doesn't do the isMainThread check either).

This bug's patch should also now compile with or without MOZ_B2G_NFC enabled (at 768786d2fe0461d997db413f077a59a3ec7ef369).

Comment 176 Garner Lee 2013-07-25 14:00:25 PDT

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #166)
> (In reply to Garner Lee from comment #165)
> > The new patches converts the navigator.mozNfc to use WebIDL, and should work
> > without crashes I believe, or at least unblock people wanting to run the
> > code on the latest gecko. It's possible I missed a crash assertion signal 11
> > message, if so, I'll check soon. I do know there's some graphic rendering
> > glitches on nexus-s (some work being done on gralloc I think).
> 
> Thanks for rebasing the patch set. It applies cleanly now, but it still
> doesn't work for me. Did you build with debugging enabled?
> 
> I use mozilla-central. I had to remove FORCE_STATIC_LIB from
> dom/nfc/Makefile.in to make the patches compile. The crash of comment 152
> still happens during boot. And if you don't set --enable-b2g-nfc, some of
> the code fragments don't build.
> 
> Best regards
> Thomas

Hi Thomas, code is rebased again, and DEBUG and disabling MOZ_B2G_NFC should now work.

Comment 177 Dimi Lee[:dimi][:dlee] 2013-07-25 21:07:29 PDT

Created attachment 781461 [details]
NFC Component.pdf

Comment 178 Thomas Zimmermann [:tzimmermann] [:tdz] 2013-07-26 03:17:09 PDT

> Hi Thomas, code is rebased again, and DEBUG and disabling MOZ_B2G_NFC should
> now work.

Thanks a lot. I'll try ASAP.

Comment 179 Yoshi Huang[:allstars.chh] 2013-07-26 08:58:25 PDT

Comment on attachment 781128 [details] [diff] [review]
Patch 1 (v6) NFC DOM IDL/WebIDL

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

::: dom/nfc/nsIDOMNfc.idl
@@ +21,5 @@
> +  boolean validateNdefTag(in jsval records);
> +
> +  // records is an array of 1 or more records
> +  [implicit_jscontext]
> +  nsIDOMDOMRequest writeNdefTag(in jsval records);

Just for your information,
recently whatwg has planed to replace DOMRequest with Promise
http://dom.spec.whatwg.org/#promises

I am not saying now, but one day we should replace it with Promise.

Comment 180 Yoshi Huang[:allstars.chh] 2013-07-26 09:02:30 PDT

(In reply to Garner Lee from comment #174)
>I'm not sure about the DOM review schedule however...

Hi Garner
But you need to send review request first, right?

Aren't your patches not ready yet?

(Or do you plan to move it to WebIDL, or to rewrite the worker in C++, ...etc?)

Thanks

Comment 181 arno 2013-07-26 18:12:13 PDT

(In reply to dlee from comment #177)

Dimi: thanks for the proposal. You included some architectural components to handle handovers. There are also several user stories on Bugzilla that have handovers at their core. We believe that this is a little off-topic and we should focus on NFC support; *not* the kind of apps/services you can build with NFC. In particular, any user stories that deal with handovers are out-of-scope of this Bugzilla entry. From the NFC perspective, a handover request is merely a specific NDEF message that is shared between two phones. We should focus on the exchange of NDEF messages, but not the content of those NDEF messages nor what the recipient does with those messages. To make the point, Android does not handle BT or Wifi Direct Handovers in the NFC stack for things like file/photo/video sharing; this is done by third-party applications such as File Expert (see the video that Ken posted).

From our perspective, there are three issues that need to be discussed in the context of NFC for FFOS: (1) The API that app developers have to use for NFC, (2) the nfcd/Gonk Protocol and (3) the UX related to NFC. Regarding (1), we need to discuss the WebIDL, manifest permissions and MozActivities related to NFC/NDEF handling. For (2) we need to discuss the protocol details as well as the marshalling (JSON vs binary). For (3) we need to discuss the settings as well as the UI that the sender uses during a beam/P2P to confirm the sending of information (the 'shrinking UI' that Android uses).

Comment 182 Garner Lee 2013-08-08 23:41:44 PDT

Created attachment 787980 [details] [diff] [review]
Patch 1 (v7) NFC DOM IDL/WebIDL

Comment 183 Garner Lee 2013-08-08 23:43:39 PDT

Created attachment 787985 [details] [diff] [review]
Patch 2 (v6) - NFC DOM Boilerplate

Comment 184 Garner Lee 2013-08-08 23:45:21 PDT

Created attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

Comment 185 Garner Lee 2013-08-08 23:46:11 PDT

Created attachment 787990 [details] [diff] [review]
Patch 4 (v7) - NFC Chrome Worker Implementation

Comment 186 Garner Lee 2013-08-08 23:46:52 PDT

Created attachment 787992 [details] [diff] [review]
Patch 5 (v6) - NFC Permissions Changes

Comment 187 Garner Lee 2013-08-08 23:47:58 PDT

Created attachment 787993 [details] [diff] [review]
Patch 6 (v6) - NFC Makefiles and Manifests

Comment 188 Garner Lee 2013-08-08 23:51:03 PDT

Created attachment 787994 [details] [diff] [review]
Patch 7 (v2) NFC Gonk Misc

Gonk-misc repo system startup files and config (nfcd related).

Comment 189 Paul Theriault [:pauljt] 2013-08-18 18:55:56 PDT

Looks like a lot of progress has been made here - are the attachments and documents in this bug now at a point where I can start reviewing this from a security perspective?

Comment 190 Garner Lee 2013-08-19 14:54:24 PDT

@Paul: 
The DOM API is pretty close to "final", as in, mostly representative of the final API usage. There are a few application level things left, and in progress before the workweek:

1) Security: Tightening up the messaging between System/gonk and System/js/nfc.js (that is, add another permission to system messages: "nfc-manager", so only certified, system app  can receive NFC hardware events). Apps only get notified of a NFC device being present, or lost, via WebActivities only. Useful should tons of apps support NFC outside certified status (the user can choose which brower to open the URL for example). "nfc" permissions is also certified only, but it makes a difference once it's opened up for external apps.

2) Related to #1, Serialize the DOM calls into the hardware, and allow different NFC apps to get NFC control if brought to foreground, and manage its associated events.

Comment 191 Jonas Sicking (:sicking) 2013-08-21 23:54:58 PDT

I'd love to understand how this API works better before reviewing. Any chance I could find someone on irc as to get help describing on this works?

Comment 192 Paul Theriault [:pauljt] 2013-08-22 00:09:30 PDT

Comment on attachment 787992 [details] [diff] [review]
Patch 5 (v6) - NFC Permissions Changes

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

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +111,5 @@
> +  "nfc-secure-element-transaction": {
> +    "nfc": []
> +  },
> +  "nfc-secure-element-deactivated": {
> +    "nfc": []

Should the following messages also have a permission check?
   * nfc-ndef-details
   * nfc-ndef-read
   * nfc-ndef-write   

If not, how do were prevent apps listening to these messages, and intercepting data?

Comment 193 Garner Lee 2013-08-22 12:40:25 PDT

(In reply to Paul Theriault [:pauljt] from comment #192)

> If not, how do were prevent apps listening to these messages, and
> intercepting data?

NFC DOM calls are protected by requiring manifest declared NFC permissions. The navigator.mozNfc object does not (should not) get become available otherwise.

NFC aware apps also require manifest declared "nfc-ndef-discovered" activity messages (see  the modified browser manifest), which is also controlled by the user, during installation. If a website can add such manifest declared permissions, then that would be bad.

By convention, if the system app nfc-manager is only allowed to send explicity nfc-ndef-discovered activity messages, then the manifest from my current understanding, should be enough to stop websites/apps from getting the activity. If that's not correct (and there's a security hole), we can discuss how to tighten it up.

Comment 194 Raj Masud Forhad 2013-08-27 07:33:58 PDT

Hi Guys,
I am new here. 
I have been following the web api for NFC api.
I have seen the NFC Web Api and tried to implement using html5.
But i could not find any sample implementation code or sample app using the NFC Api. 

Thanks for your help.

Comment 195 Jonas Sicking (:sicking) 2013-08-29 20:49:08 PDT

Comment on attachment 787980 [details] [diff] [review]
Patch 1 (v7) NFC DOM IDL/WebIDL

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

based on the discussions at the work week, this needed some updates.

Something that occurred to me yesterday: Does calling connect(tech) and disconnect() result in any transmissions over the nfc hardware? Or does it just change state inside the API? If it's just changing state inside the API then I don't think we need those two functions.

Comment 196 Jonas Sicking (:sicking) 2013-08-29 20:51:27 PDT

Comment on attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

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

Someone other than me should review this. Ideally someone on Ken's team.

Comment 197 Garner Lee 2013-08-30 13:34:24 PDT

(In reply to Jonas Sicking (:sicking) from comment #195)
> Comment on attachment 787980 [details] [diff] [review]
> Patch 1 (v7) NFC DOM IDL/WebIDL
> 
> Review of attachment 787980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> based on the discussions at the work week, this needed some updates.
> 
> Something that occurred to me yesterday: Does calling connect(tech) and
> disconnect() result in any transmissions over the nfc hardware? Or does it
> just change state inside the API? If it's just changing state inside the API
> then I don't think we need those two functions.

Connect/Close (disconnect), does do something at the NFC hardware, so we'll need to keep them. It does things like select the technology used to read the tag or device on the other end.

Concerning updates, yes, it's been picking up some new stuff from the week's discussion. UX for push, Foreground dispatch for NFC Writing apps, and a new "makeReadOnly" function call to tell the tag to become permanently read only. This should not have any effect on existing APIs, aside from not being able to write after making things read only.

Comment 198 Ken Chang[:ken] 2013-09-03 02:37:39 PDT

Comment on attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

Olli will help to review this patch.

Comment 199 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-09-03 08:29:45 PDT

Comment on attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

We should not be using XPIDL for new DOM objects.  WebIDL is the way to go.

Also the jsval handling in ndefrecord is totally wrong.  We need to trace the value or whatever is in there could get collected.  I'm surprised this works at all.

Comment 200 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-09-03 08:31:23 PDT

If you need help figuring out how to fix this up feel free to send email.

Comment 201 Garner Lee 2013-09-05 11:21:07 PDT

Created attachment 800277 [details] [diff] [review]
Patch 1 (v8) NFC DOM IDL/WebIDL

Updates. Add config and ndefMakeReadOnly to DOM/nfcd.

Comment 202 Garner Lee 2013-09-05 11:22:36 PDT

Created attachment 800278 [details] [diff] [review]
Patch 2 (v7) - NFC DOM Boilerplate

Comment 203 Garner Lee 2013-09-05 11:24:40 PDT

Created attachment 800284 [details] [diff] [review]
Patch 3 (v8) - NFC DOM Implementation

Comment 204 Garner Lee 2013-09-05 11:27:50 PDT

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #199)
> Comment on attachment 787989 [details] [diff] [review]
> We need to trace the value or whatever is in there could get collected.

As a note, Patch 3 (v8) in the new patchset does not yet address this comment yet.

Comment 205 Garner Lee 2013-09-05 11:28:43 PDT

Created attachment 800288 [details] [diff] [review]
Patch 4 (v8) - NFC Chrome Worker Implementation

Comment 206 Garner Lee 2013-09-05 11:29:42 PDT

Created attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes

Comment 207 Garner Lee 2013-09-05 11:31:16 PDT

Comment on attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes

Add tighter permissions to read and write NFC calls.

Comment 208 Garner Lee 2013-09-05 11:32:37 PDT

Created attachment 800290 [details] [diff] [review]
Patch 6 (v7) - NFC Makefiles and Manifests

Comment 209 Garner Lee 2013-09-05 11:44:43 PDT

Created attachment 800301 [details] [diff] [review]
Fix debug build - remove unneeded NS_IsMainThread assertion r=allstars.chh

Comment 210 [:fabrice] Fabrice Desré 2013-09-05 11:46:50 PDT

(In reply to Garner Lee from comment #205)
> Created attachment 800288 [details] [diff] [review]
> Patch 4 (v8) - NFC Chrome Worker Implementation

We know that js workers have a high memory impact because they need their own JS runtime. It would be much better to write that in c++ (it looks pretty simple) instead. We did that for the wifi workers and it's ongoing for the net_worker.js also.

Comment 211 Garner Lee 2013-09-05 11:53:25 PDT

The last patch set for gecko is based on: 2a2ff267b86346ae80e453a814c442da82583ab1

Comment 212 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-05 16:59:02 PDT

Comment on attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

Yes, WebIDL should be used for this. Makes C++ much nicer :)

Hmm, /* Copyright © 2013 Deutsche Telekom, Inc. */
Better to ask Gerv if that is ok.

Comment 213 Yoshi Huang[:allstars.chh] 2013-09-05 19:22:04 PDT

Comment on attachment 800301 [details] [diff] [review]
Fix debug build - remove unneeded NS_IsMainThread assertion r=allstars.chh

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

Add r=me, and you need to update the title of the patch.

Comment 214 Yoshi Huang[:allstars.chh] 2013-09-05 19:51:01 PDT

Comment on attachment 800288 [details] [diff] [review]
Patch 4 (v8) - NFC Chrome Worker Implementation

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

Hi, Garner
I mentioned this last week,
we can make the object flat.

::: dom/system/gonk/Nfc.js
@@ +169,5 @@
> +
> +  ndefRead: function ndefRead(message) {
> +    debug("ndefReadRequest message: " + JSON.stringify(message));
> +    var outMessage = {
> +      type: "NDEFReadRequest",

here's a type property called "NDEFReadRequest",

@@ +175,5 @@
> +      requestId: message.requestId
> +    };
> +
> +    debug("ndefReadRequest message out: " + JSON.stringify(outMessage));
> +    this.worker.postMessage({type: "ndefRead", content: outMessage});

Here's another type called "ndefRead"

It seems we can use only one here, and remove the 'content' property
then we could do 

this.worker.postMessage(outMessage);

Comment 215 :Ms2ger 2013-09-09 03:25:46 PDT

Comment on attachment 800277 [details] [diff] [review]
Patch 1 (v8) NFC DOM IDL/WebIDL

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

Those should be implemented as WebIDL, not XPIDL.

Comment 216 Jonas Sicking (:sicking) 2013-09-09 06:00:43 PDT

We're definitely going to move it to WebIDL. However that shouldn't block the initial landing of this bug. There are a bunch of more important polish to figure out and that has a higher priority right now.

Garner has offered to do the move to WebIDL and that will happen within a few gecko releases after the initial landing. We are aware that it means that various edge cases are not going to behave perfectly until the API has been converted to WebIDL, but due to the small exposure of this API (FirefoxOS devices that has NFC hardware only), that should be no problem.

Comment 217 Jonas Sicking (:sicking) 2013-09-09 06:01:27 PDT

Garner, please do file a followup bug for the WebIDL work and assign to yourself before landing this bug though.

Comment 218 Garner Lee 2013-09-09 06:11:33 PDT

(In reply to Olli Pettay [:smaug] from comment #212)
> Comment on attachment 787989 [details] [diff] [review]
> Patch 3 (v7) - NFC DOM Implementation
> 
> Yes, WebIDL should be used for this. Makes C++ much nicer :)
> 
> Hmm, /* Copyright © 2013 Deutsche Telekom, Inc. */
> Better to ask Gerv if that is ok.

I'll email the relevant lawyers.

Comment 221 Jonas Sicking (:sicking) 2013-09-09 09:03:21 PDT

Obviously security bugs needs to be fixed before we can land anything.

Comment 222 Garner Lee 2013-09-10 00:59:30 PDT

Created attachment 802122 [details] [diff] [review]
Patch 7 (v3) NFC Gonk Misc

Remove nfc.enabled system property from init patch.

Comment 223 Michael Wu [:mwu] 2013-09-10 06:00:16 PDT

Comment on attachment 802122 [details] [diff] [review]
Patch 7 (v3) NFC Gonk Misc

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

::: default-gecko-config
@@ +23,4 @@
>  ac_add_options --with-gonk-toolchain-prefix="$TARGET_TOOLS_PREFIX"
>  
>  ac_add_options --enable-application=b2g
> +ac_add_options --enable-b2g-nfc

If this is unconditionally enabled, we should enable it via configure.in or eliminate the config options and simply build it on gonk.

Comment 224 Garner Lee 2013-09-10 06:13:23 PDT

(In reply to Jonas Sicking (:sicking) (vacation until 9/9. In norway until 9/16) from comment #221)
> Obviously security bugs needs to be fixed before we can land anything.

I believe Paul indicated we should type check the NdefRecord field, and that will likely be a typed array once our binary protocol is done. At that time, the security issue should be resolved (as well as get rooted).

Comment 225 Garner Lee 2013-09-10 07:29:29 PDT

Created attachment 802341 [details] [diff] [review]
Patch 7 (v4) NFC Gonk Misc r=mwu

As per conversation, we're checking Nfc in disabled initially, so the enable flag is removed. Patch updated.

Comment 226 Paul Theriault [:pauljt] 2013-09-12 05:32:50 PDT

Comment on attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes

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

Looks good to me, one minor nit. But I am not a peer, so this probably needs approval.

::: dom/apps/src/PermissionsTable.jsm
@@ +300,5 @@
> +                           "nfc-manager": {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION,
> +                             access: ["read"]

I don't think we need access: ["read"] here for the manager permission. There is no need for an access property if there is only one value I think.

Comment 227 Garner Lee 2013-09-12 08:28:45 PDT

Created attachment 803761 [details] [diff] [review]
Patch 1 (v9) NFC DOM IDL/WebIDL

Comment 228 Garner Lee 2013-09-12 08:30:38 PDT

Created attachment 803763 [details] [diff] [review]
Patch 2 (v8) - NFC DOM Boilerplate

Comment 229 Garner Lee 2013-09-12 08:33:41 PDT

Created attachment 803764 [details] [diff] [review]
Patch 3 (v9) - NFC DOM Implementation

Review comments implemented: rooted playload field in NdefRecord, and additional type checks (String) for security.

Comment 230 Garner Lee 2013-09-12 08:38:39 PDT

Created attachment 803769 [details] [diff] [review]
Patch 4 (v9) - NFC Chrome Worker Implementation

Note: this is the non-binary protocol version

Comment 231 Garner Lee 2013-09-12 08:40:46 PDT

Created attachment 803771 [details] [diff] [review]
Patch 6 (v8) - NFC Makefiles and Manifests

MOZ_B2G_NFC now set to build, but operate only if nfc.powerlevel is 1 (enabled).

Comment 232 Garner Lee 2013-09-12 08:48:24 PDT

(In reply to Paul Theriault [:pauljt] from comment #226)
> Comment on attachment 800289 [details] [diff] [review]
> Patch 5 (v7) - NFC Permissions Changes
> 
> Review of attachment 800289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, one minor nit. But I am not a peer, so this probably needs
> approval.
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +300,5 @@
> > +                           "nfc-manager": {
> > +                             app: DENY_ACTION,
> > +                             privileged: DENY_ACTION,
> > +                             certified: ALLOW_ACTION,
> > +                             access: ["read"]
> 
> I don't think we need access: ["read"] here for the manager permission.
> There is no need for an access property if there is only one value I think.

Thanks for the review. I see more than one permission with single access? What happens if apps request a permisison, but not specify the access level? Do they get all access levels?

Who should review the new permissions?

Comment 233 Garner Lee 2013-09-13 03:57:17 PDT

Created attachment 804387 [details] [diff] [review]
Patch 1 (v10) NFC DOM IDL/WebIDL

Add MOZ_B2G_NFC Compile flag around MozNfcEvent.webidl in new moz.build.

Comment 234 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-13 04:33:46 PDT

Comment on attachment 803769 [details] [diff] [review]
Patch 4 (v9) - NFC Chrome Worker Implementation

This touches Workers -> bent

Comment 235 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-13 04:43:16 PDT

I don't have access to bug 913336, and I think I should have before I can review this stuff.

Comment 236 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-09-13 06:30:01 PDT

(In reply to Olli Pettay [:smaug] from comment #235)
> I don't have access to bug 913336, and I think I should have before I can
> review this stuff.

Done.

Comment 237 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-13 07:58:08 PDT

Comment on attachment 804387 [details] [diff] [review]
Patch 1 (v10) NFC DOM IDL/WebIDL

After looking at the API (which when using .idl requires tons of JSAPI usage),
and reading Bug 913336 I definitely would like to see this implemented using WebIDL. Also, use of WebIDL was asked already over 3 months ago. 
(By default, and there are only few exceptions, manual JSAPI usage in DOM code is a bug which should be fixed hopefully using the new bindings.)

Some comments anyway

>+  /* Get metadata details of the discovered and connected NDEF message */
>+  [implicit_jscontext]
>+  nsIDOMDOMRequest ndefDetails();
I don't see any need for [implicit_jscontext].
The implementation of this method doesn't use cx for anything 
Same with several other methods.



>+interface MozNfcEvent : Event
>+{
>+  [Throws]
>+  readonly attribute any message;
>+};
Hmm, this type of events b2g has are a bit odd. They certainly should have a constructor.
One problem is that with this setup event.message !== event.message.
But I can see we need something for simple JSON usage so I guess I can live with this setup for now.

Comment 238 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-13 08:05:44 PDT

Comment on attachment 803763 [details] [diff] [review]
Patch 2 (v8) - NFC DOM Boilerplate

None of the DOMClassinfo stuff would be needed if WebIDL was used.

Comment 239 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-13 08:22:04 PDT

Comment on attachment 803764 [details] [diff] [review]
Patch 3 (v9) - NFC DOM Implementation


>+
>+NS_IMETHODIMP
>+NdefRecord::Initialize(nsISupports* aOwner,
>+                     JSContext* aContext,
>+                     JSObject* aObject,
>+                     const JS::CallArgs& aArgv)
This would be a lot simpler with WebIDL bindings

>+nsNfc::ValidateNdefTag(const JS::Value& aRecords, JSContext* aCx, bool* result)
Also this would be *significantly* simpler with WebIDL bindings

Comment 240 Gregory Szorc [:gps] 2013-09-13 11:43:51 PDT

Comment on attachment 803771 [details] [diff] [review]
Patch 6 (v8) - NFC Makefiles and Manifests

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

Please address feedback and then resubmit for a likely rubber stamp r+.

::: configure.in
@@ +7468,5 @@
> +    MOZ_B2G_NFC= )
> +if test -n "$MOZ_B2G_NFC"; then
> +   AC_DEFINE(MOZ_B2G_NFC)
> +fi
> +AC_SUBST(MOZ_B2G_NFC)

I'm not sure why you need "B2G" in the name. Isn't this a generic Gecko feature? If so, we should stop using "B2G" to describe potentially cross-platform features.

::: dom/nfc/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# 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/.
> +#
> +# Copyright © 2013 Deutsche Telekom, Inc.

Is this really copyright Deutsche Telekom? I don't see this copyright notice in any files currently in m-c. But I'm also not sure what the current licensing policy is.

@@ +6,5 @@
> +
> +DEPTH            = ../..
> +topsrcdir        = @top_srcdir@
> +srcdir           = @srcdir@
> +VPATH            = @srcdir@

You don't need this variable boilerplate any more.

@@ +11,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +include $(topsrcdir)/dom/dom-config.mk
> +include $(topsrcdir)/config/rules.mk

You don't need to include rules.mk any more.

Comment 241 Garner Lee 2013-09-16 09:26:03 PDT

Thanks Olli and Gregory, we'll apply the feedback. Concerning the copyright issue, I'm waiting on a response on how to handle it (source control history (mozilla preferred) versus in file (more difficult to manage)). As for WebIDL, we'll attempt another conversion (mozNfc uses js workers, and later, C++ workers from Thomas (Tdz)), and move NdefRecord over to WebIDL.

Comment 242 [:fabrice] Fabrice Desré 2013-09-16 14:20:22 PDT

Comment on attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes

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

::: dom/apps/src/PermissionsTable.jsm
@@ +300,5 @@
> +                           "nfc-manager": {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION,
> +                             access: ["read"]

I agree with Paul, unless we plan to add other access modes later, which I doubt for the manager.

Comment 243 David Chan [:dchan] 2013-09-18 01:40:07 PDT

Comment on attachment 803764 [details] [diff] [review]
Patch 3 (v9) - NFC DOM Implementation

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

::: dom/nfc/nsNfc.cpp
@@ +114,5 @@
> +    uint32_t namelen;
> +    if (JS_GetElement(aCx, &obj, index, &val)) {
> +      const char *name = JS_GetClass(JSVAL_TO_OBJECT(val))->name;
> +      namelen = strlen(name);
> +      if (strncmp(ndefRecordName, name, namelen)) {

What is the reasoning behind using strncmp instead of strcmp? There is a potential edgecase where name is a prefix of ndefRecordName and the comparison returns 0. I don't believe this is likely to happen, but something to consider.

Comment 244 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-18 04:56:33 PDT

(Note, after WebIDLification that JS_GetElement/JS_GetClass()->name thing can be removed, as far as I see)

Comment 245 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-18 04:58:00 PDT

Btw, if you need help with WebIDLification, don't hesitate to ask in #content IRC channel.
And we have reasonable good documentation too https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Comment 246 Garner Lee 2013-09-18 10:38:28 PDT

(In reply to Olli Pettay [:smaug] from comment #245)
> #content IRC channel.

Thanks for the pointer to the IRC channel.

Comment 247 Yoshi Huang[:allstars.chh] 2013-09-24 20:26:31 PDT

Dimi mentioned that in ontechdiscovered, 'P2P' is actually not a type of technology.
Also NDEF is a kind of data-format, not a type of technology either.
(I know Android puts NDEF in the tech package, http://developer.android.com/reference/android/nfc/tech/Ndef.html).

On W3C NFC API, http://w3c.github.io/nfc/proposals/common/nfc.html
it uses a different callback when the tag/device is discovered.

We should use a better description for the callback(ontechdiscovered) as well.

What do you think? 
File another bug?

Thanks

Comment 248 Garner Lee 2013-09-24 21:08:29 PDT

It is possible to switch back to on<messagename> messages, given that I'm re-writing it all for WebIDL. However, it's also draft 1. The key issue is identifying which app should get the discovery messages, given that WebActivity is the go-to selector currently (thus, nfc_manager fires off the UI request to the user selected app). We should file a new bug.

Comment 249 Garner Lee 2013-09-24 21:35:06 PDT

Actually, with all the work to switch to WebIDL, and then again to the "new, new" draft standards, we should be careful about scheduling intermediate and final standard changes: they probably won't have the same deadlines.

Comment 250 Garner Lee 2013-09-25 17:31:51 PDT

Created attachment 810257 [details] [diff] [review]
Patch 1 (v11) NFC DOM IDL/WebIDL

WebIDL conversion of navigator.mozNfc (in JS), NfcEvent (C++), MozNdefRecord (C++).

Comment 251 Garner Lee 2013-09-25 17:34:11 PDT

Created attachment 810258 [details] [diff] [review]
Patch 2 (v9) - NFC DOM Boilerplate

Comment 252 Garner Lee 2013-09-25 17:36:02 PDT

Created attachment 810259 [details] [diff] [review]
Patch 3 (v10) - NFC DOM Implementation

nsNfc.js navigator WebIDL DOM implementation.

Comment 253 Garner Lee 2013-09-25 17:37:56 PDT

Created attachment 810261 [details] [diff] [review]
Patch 4 (v10) - NFC Chrome Worker Implementation

Comment 254 Garner Lee 2013-09-25 17:39:35 PDT

Created attachment 810262 [details] [diff] [review]
Patch 5 (v8) - NFC Permissions Changes r=fabrice

Move nfc-manager back down to a simple permission declaration without "read".

Comment 255 Garner Lee 2013-09-25 17:41:06 PDT

Created attachment 810266 [details] [diff] [review]
Patch 6 (v9) - NFC Makefiles and Manifests

Updated makefiles for WebIDL implementation.

Comment 256 Garner Lee 2013-09-26 10:19:49 PDT

Hi, the try server output of the current patch set is here:

https://tbpl.mozilla.org/?tree=Try&rev=d4bf6d92bc2a

Comment 257 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-30 13:50:05 PDT

Comment on attachment 810258 [details] [diff] [review]
Patch 2 (v9) - NFC DOM Boilerplate


>+  NS_WARNING("XXXXXX Checking NFC Support");
remove this

>+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
>+  return win && CheckPermission(win, "nfc-read") ||
>+                CheckPermission(win, "nfc-write");
Please write this
win && (Check... || Check...);

Comment 258 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-30 14:02:43 PDT

Comment on attachment 810257 [details] [diff] [review]
Patch 1 (v11) NFC DOM IDL/WebIDL


>+[scriptable, builtinclass, uuid(41fcd640-4dd4-11e1-b86c-0800200c9a66)]
>+interface nsIDOMMozNfcEvent : nsIDOMEvent
>+{
>+  /**
>+   * NFC Tag message data
>+   */
>+  [implicit_jscontext]
>+  readonly attribute jsval message;
>+};
There shouldn't be any need for this interface.

>+
>+  /**
>+   * payload - Binary data blob. The meaning of this field is application dependent.
>+   */
>+  readonly attribute DOMString payload;
Ok, so this ended up being DOMString. Perhaps some example would be good what kind of data
this can be.


>+
>+   /* Get metadata details of the discovered and connected NDEF message */
>+   DOMRequest detailsNDEF();
hmm, a bit odd method name. Sounds like an attribute name, not a getter.
Perhaps getDetailsNDEF() ?

>+
>+   /* NDEF Read returns an array of NDEF Records consisting of 1 or more elements */
>+   DOMRequest readNDEF();
>+
>+   /* NDEF Write records that is an array of 1 or more records */
>+   [Throws]
>+   DOMRequest writeNDEF(sequence<MozNdefRecord> records);
>+
>+   /* Permanently make a physical NFC tag read only */
>+   DOMRequest makeReadOnlyNDEF();
>+
>+   /* Sets a callback to notifiy when NDEF Push message communication is available for use. (future API)
>+   boolean registerNDEFPushMessageCallback(in nsINdefPushMessageCallback aCallback);
>+   */
>+
>+   /**
>+    * NFCA functions (future API)
>+    */
>+
>+   DOMRequest detailsNfcATag();
>+
>+   [Throws]
>+   DOMRequest transceiveNfcATag(sequence<octet> buf);
>+
>+   /**
>+    * Generic tag/tech functions
>+    */
>+   [Throws]
>+   DOMRequest connect(unsigned long techType);
>+
>+   DOMRequest close();
>+
>+   /* Foreground dispatch allows the app, if in the foreground, to get routed all
>+      NFC messages. Useful for applications that write NFC tags. Privilaged API. (future API)
>+   boolean registerForegroundDispatch(in nsIForegroundDispatchCallback aCallback);
>+   */
Please don't add future APIs (unless the API is actually spec'ed somewhere) to the .webidl


>+interface MozNfcEvent : Event
>+{
>+  [Throws]
>+  readonly attribute any message;
>+};

Hmm, it is a bit misfortune if a readonly attribute returns a new object every time it is accessed.
Could we not have any other kind of API here?
(I know StkCommandEvent has similar setup, but I'm not quite happy with it.)
Where and how is the event supposed to be used?

r- anyway, because this is adding an unneeded .idl.

Comment 259 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-09-30 14:04:22 PDT

Comment on attachment 810259 [details] [diff] [review]
Patch 3 (v10) - NFC DOM Implementation

This will change a bit too if the .idl will be removed.

And I'd like to see some example code using the new event.

Comment 260 Garner Lee 2013-09-30 17:25:46 PDT

(In reply to Olli Pettay [:smaug] from comment #258)
> Comment on attachment 810257 [details] [diff] [review]
> Patch 1 (v11) NFC DOM IDL/WebIDL
> 
> 
> >+[scriptable, builtinclass, uuid(41fcd640-4dd4-11e1-b86c-0800200c9a66)]
> >+interface nsIDOMMozNfcEvent : nsIDOMEvent
> >+{
> >+  /**
> >+   * NFC Tag message data
> >+   */
> >+  [implicit_jscontext]
> >+  readonly attribute jsval message;
> >+};
> There shouldn't be any need for this interface.
> 
> >+
> >+  /**
> >+   * payload - Binary data blob. The meaning of this field is application dependent.
> >+   */
> >+  readonly attribute DOMString payload;
> Ok, so this ended up being DOMString. Perhaps some example would be good
> what kind of data
> this can be.
> 
> 
> >+
> >+   /* Get metadata details of the discovered and connected NDEF message */
> >+   DOMRequest detailsNDEF();
> hmm, a bit odd method name. Sounds like an attribute name, not a getter.
> Perhaps getDetailsNDEF() ?
> 
> >+
> >+   /* NDEF Read returns an array of NDEF Records consisting of 1 or more elements */
> >+   DOMRequest readNDEF();
> >+
> >+   /* NDEF Write records that is an array of 1 or more records */
> >+   [Throws]
> >+   DOMRequest writeNDEF(sequence<MozNdefRecord> records);
> >+
> >+   /* Permanently make a physical NFC tag read only */
> >+   DOMRequest makeReadOnlyNDEF();
> >+
> >+   /* Sets a callback to notifiy when NDEF Push message communication is available for use. (future API)
> >+   boolean registerNDEFPushMessageCallback(in nsINdefPushMessageCallback aCallback);
> >+   */
> >+
> >+   /**
> >+    * NFCA functions (future API)
> >+    */
> >+
> >+   DOMRequest detailsNfcATag();
> >+
> >+   [Throws]
> >+   DOMRequest transceiveNfcATag(sequence<octet> buf);
> >+
> >+   /**
> >+    * Generic tag/tech functions
> >+    */
> >+   [Throws]
> >+   DOMRequest connect(unsigned long techType);
> >+
> >+   DOMRequest close();
> >+
> >+   /* Foreground dispatch allows the app, if in the foreground, to get routed all
> >+      NFC messages. Useful for applications that write NFC tags. Privilaged API. (future API)
> >+   boolean registerForegroundDispatch(in nsIForegroundDispatchCallback aCallback);
> >+   */
> Please don't add future APIs (unless the API is actually spec'ed somewhere)
> to the .webidl
> 
> 
> >+interface MozNfcEvent : Event
> >+{
> >+  [Throws]
> >+  readonly attribute any message;
> >+};
> 
> Hmm, it is a bit misfortune if a readonly attribute returns a new object
> every time it is accessed.
> Could we not have any other kind of API here?
> (I know StkCommandEvent has similar setup, but I'm not quite happy with it.)
> Where and how is the event supposed to be used?
> 
> r- anyway, because this is adding an unneeded .idl.

I'll look into removing the IDL (it's actually still the same way with StkCommandEvent). The NfcEvent was not currently used. It is a new param ("NFCPeer" in the onpeerfound/onpeerlost W3C draft NFC APIs): http://htmlpreview.github.io/?https://github.com/w3c/nfc/blob/master/proposals/common/nfc.html, 5.1 Attributes.

We don't have NFCPeer as we use connect() and close() to manage sessions since it's currently understood that WebActivities can't yet deliver a non-dictionary type. We use WebActivities to get the application picker for multiple application matches.

It's a new planned App centric API, and there's no usage yet, but it'll look like this:
navigator.mozNfc.onpeerfound =
    function main_handleOnPeerFound(event) {
      writeNDEF([new MozNdefRecord(....)]);
      // Ex. NDEF containing the absolutely most current YouTube URL + time index in the senders's video stream.
    };

Following the previous comment, remove "Future APIs", then we can actually remove it for now, along with the extra IDL. If that's preferable, I will remove it (and use the generic Event base if possible).

Comment 261 Garner Lee 2013-09-30 18:44:32 PDT

> >+   /* Get metadata details of the discovered and connected NDEF message */
> >+   DOMRequest detailsNDEF();
> hmm, a bit odd method name. Sounds like an attribute name, not a getter.
> Perhaps getDetailsNDEF() ?

It's possible. This follows the naming scheme of read and writeNDEF based on other feedback from Yoshi. It was earlier named

  "ndef{read, write, details}"

Any strong opinions (there's no corresponding "set" beyond writeNDEF)?

Comment 262 Ken Chang[:ken] 2013-10-01 00:52:14 PDT

Comment on attachment 810261 [details] [diff] [review]
Patch 4 (v10) - NFC Chrome Worker Implementation

Bent is very busy. Need Yoshi's help to review this patch.

Comment 263 Yoshi Huang[:allstars.chh] 2013-10-01 02:00:57 PDT

Comment on attachment 810261 [details] [diff] [review]
Patch 4 (v10) - NFC Chrome Worker Implementation

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

::: dom/system/gonk/Nfc.js
@@ +10,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

/* This Source Code Form is subject to the terms of the Mozilla Public
 * 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/. */

@@ +12,5 @@
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +/* Copyright © 2013, Deutsche Telekom, Inc. */

Is this okay?

@@ +71,5 @@
> +  Services.obs.addObserver(this, TOPIC_XPCOM_SHUTDOWN, false);
> +
> +  let lock = gSettingsService.createLock();
> +  lock.get("nfc.powerlevel", this);
> +  this.powerlevel = 0; // default to off (FIXME: add get preference)

What does 0 mean here?
can you const it?

@@ +73,5 @@
> +  let lock = gSettingsService.createLock();
> +  lock.get("nfc.powerlevel", this);
> +  this.powerlevel = 0; // default to off (FIXME: add get preference)
> +
> +  debug("Starting Worker");

The worker starts in line 62

@@ +101,5 @@
> +   */
> +  onmessage: function onmessage(event) {
> +    let message = event.data;
> +    debug("Received message: " + JSON.stringify(message));
> +    if (this.powerlevel < 1) {

const 1 here.

@@ +120,5 @@
> +        this._connectedSessionId = null;
> +        gSystemMessenger.broadcastMessage("nfc-manager-tech-lost", message);
> +        ppmm.broadcastAsyncMessage("NFC:TechLost", message);
> +        break;
> +      case "NDEFDetailsResponse":

You use 'NDEF' for messages received from workers,
and 'Ndef' for messages from Content,
Can you make it consistent?

@@ +143,5 @@
> +        ppmm.broadcastAsyncMessage("NFC:CloseResponse", message);
> +        break;
> +      case "ConfigResponse":
> +        // Config changes. No notification.
> +        debug("ConfigResponse" + JSON.stringify(message));

Why Config is doing nothing here?

@@ +145,5 @@
> +      case "ConfigResponse":
> +        // Config changes. No notification.
> +        debug("ConfigResponse" + JSON.stringify(message));
> +        break;
> +

nit, extra line

@@ +154,5 @@
> +
> +  // nsINfcWorker
> +
> +  worker: null,
> +  powerlevel: 0,

'powerLevel'.
Can you put more comments about powerLevel?

@@ +157,5 @@
> +  worker: null,
> +  powerlevel: 0,
> +
> +  ndefDetails: function ndefDetails(message) {
> +    debug("ndefDetailsRequest message: " + JSON.stringify(message));

Didn't you already print the debug message when the message is received?
Why do you print it again?

@@ +158,5 @@
> +  powerlevel: 0,
> +
> +  ndefDetails: function ndefDetails(message) {
> +    debug("ndefDetailsRequest message: " + JSON.stringify(message));
> +    var outMessage = {

s/var/let/
And why cannot you reuse the 'message' object?

@@ +165,5 @@
> +      requestId: message.requestId
> +    };
> +    debug("ndefDetailsRequest message out: " + JSON.stringify(outMessage));
> +
> +    this.worker.postMessage({type: "ndefDetails", content: outMessage});

Can we reuse the object above so we don't have to create another one?
Same for all functions below.

@@ +302,5 @@
> +    }
> +
> +    // Enforce NFC Write permissions.
> +    switch (message.name) {
> +      case "NFC:NdefWrite":

s/NFC/Nfc/

@@ +303,5 @@
> +
> +    // Enforce NFC Write permissions.
> +    switch (message.name) {
> +      case "NFC:NdefWrite":
> +      case "NFC:NdefMakeReadOnly":

nit, add // Fall through

@@ +365,5 @@
> +        if (setting) {
> +          switch(setting.key) {
> +            case "nfc.powerlevel":
> +              debug("Reached NFC powerlevel setting.");
> +              let powerlevel = (setting.value > 0) ? 1 : 0;

What possible values will it be?

@@ +385,5 @@
> +    this.powerlevel = powerlevel;
> +    this.setConfig({powerlevel: powerlevel});
> +  },
> +
> +  setConfig: function setConfig(prop) {

What's the difference between setNfcPowerConfig and setConfig?

::: dom/system/gonk/NfcContentHelper.js
@@ +41,5 @@
> +  "NFC:NfcATagDetailsResponse",
> +  "NFC:NfcATagTransceiveResponse",
> +  "NFC:ConnectResponse",
> +  "NFC:CloseResponse"
> +];

Consistent naming please.
Sometimes you use NFC, sometimes 'Nfc'.

@@ +51,5 @@
> +function NfcContentHelper() {
> +  this.initDOMRequestHelper(/* aWindow */ null, NFC_IPC_MSG_NAMES);
> +  Services.obs.addObserver(this, "xpcom-shutdown", false);
> +
> +  this._requestMap = new Array();

this._requestMap = [];

@@ +69,5 @@
> +
> +  _requestMap: null,
> +  _connectedSessionId: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {

Why you need to encode this?

@@ +70,5 @@
> +  _requestMap: null,
> +  _connectedSessionId: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {
> +    var encodedRecords = new Array();

let encodedRecords = [];

@@ +71,5 @@
> +  _connectedSessionId: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {
> +    var encodedRecords = new Array();
> +    for(var i=0; i < records.length; i++) {

nit, add space after 'for' and between operator=.

@@ +77,5 @@
> +      encodedRecords.push({
> +        tnf: record.tnf,
> +        type: btoa(record.type),
> +        id: btoa(record.id),
> +        payload: btoa(record.payload),

btoa? why?

@@ +90,5 @@
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +
> +    let request = Services.DOMRequest.createRequest(window);
> +    let requestId = btoa(this.getRequestId(request));

btoa for requestId? why?

@@ +307,5 @@
> +
> +    if (DEBUG) {
> +      debug("fire request success, id: " + requestId +
> +            ", result: " + JSON.stringify(result));
> +    }

remove this DEBUG

@@ +392,5 @@
> +    debug('TechLost. Clear session:');
> +    this._connectedSessionId = null;
> +  },
> +
> +  handleNfcATagDiscoveredNofitication: function handleTechDiscovered(message) {

Who calls this funciton?

@@ +449,5 @@
> +       debug('Returning: requester: ' + requester);
> +       debug('Returning: sessionId matches?: ' + (message.sessionId == this._connectedSessionId));
> +       debug('Returning: sessionId matches? sessionId: ' + message.sessionId);
> +       debug('Returning: sessionId matches?: saved sessionId: ' + this._connectedSessionId);
> +       return; // Nothing to do in this instance.

Don't flood the debug message.

@@ +455,5 @@
> +    delete this._requestMap[message.requestId];
> +    let result = message.content;
> +    let requestId = atob(message.requestId);
> +
> +    if (result.status != "OK") {

Can you use a boolean for this?

@@ +574,5 @@
> +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester.win));
> +    }
> +
> +    // Cleanup lost session.
> +    this._connectedSessionId = null;

Close is to close this session?
So App cannot call ndefRead/Write anymore?

@@ +600,5 @@
> +      }
> +    }
> +  },
> +
> +

extra lines.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +71,5 @@
> +#ifdef MOZ_B2G_NFC // {
> +class ConnectWorkerToNfc : public WorkerTask
> +{
> +public:
> +  virtual bool RunTask(JSContext *aCx);

nit, I know some existing code has this problem,
but from Mozilla Coding Style,
snuggle pointer stars with the type, not the variable name.
So it should be JSContext* aCx here.

@@ +77,5 @@
> +
> +class SendNfcSocketDataTask : public nsRunnable
> +{
> +public:
> +  SendNfcSocketDataTask(UnixSocketRawData *aRawData)

ditto.

@@ +89,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  UnixSocketRawData *mRawData;

ditto.

@@ +93,5 @@
> +  UnixSocketRawData *mRawData;
> +};
> +
> +bool
> +PostToNfc(JSContext *cx, unsigned argc, JS::Value *vp)

ditto.

::: dom/system/gonk/nfc_worker.js
@@ +157,5 @@
> +
> +function onNfcMessage(data) {
> +  let nfcJsonStr = "";
> +  // convert uint8 typed array to a string
> +  for (var i=0; i<data.byteLength - 1; i++) {

let
and space between operators.

::: dom/system/gonk/nsINfcCallback.idl
@@ +15,5 @@
> +  /* Foreground dispatch allows the app, if in the foreground, to get routed all
> +     NFC messages. Useful for applications that write NFC tags. Privilaged API.
> +   */
> +  //[implicit_jscontext] boolean registerForegroundDispatch(
> +  //                               in nsIForegroundDispatchCallback aCallback);

All commented out?

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +19,5 @@
> +  nsIDOMDOMRequest ndefWrite(in nsIDOMWindow window, in nsIVariant records);
> +  nsIDOMDOMRequest ndefMakeReadOnly(in nsIDOMWindow window);
> +
> +  nsIDOMDOMRequest ndefPush(in nsIDOMWindow window, in jsval records);
> +

nit, extra line

@@ +28,5 @@
> +  nsIDOMDOMRequest connect(in nsIDOMWindow window, in unsigned long techType);
> +  nsIDOMDOMRequest close(in nsIDOMWindow window);
> +
> +  // DEBUG only:
> +  void sendToNfcd(in DOMString message);

Remove this.

Comment 264 Gregory Szorc [:gps] 2013-10-01 03:37:24 PDT

Comment on attachment 810266 [details] [diff] [review]
Patch 6 (v9) - NFC Makefiles and Manifests

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

There are many comments from my previous review not incorporated into this patch.

I still see licensing questions. Hopefully Gerv can deliver answers.

Anyway, this patch is functionality correct, just not good enough to land. Only semantics will change before landing.

::: configure.in
@@ +7419,5 @@
> +    MOZ_B2G_NFC= )
> +if test -n "$MOZ_B2G_NFC"; then
> +   AC_DEFINE(MOZ_B2G_NFC)
> +fi
> +AC_SUBST(MOZ_B2G_NFC)

You didn't answer my earlier review comment (comment #240) about having "B2G" in the name. It seems you are adding a generic feature for NFC support to Gecko. Therefore "B2G" shouldn't be in its feature name. Please ignore the existing B2G features that get the naming wrong as a defense - they mostly landed without build peer review.

::: dom/nfc/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# 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/.
> +#
> +# Copyright © 2013 Deutsche Telekom, Inc.

Do we have an answer on the Deutsche Telekom copyright?

@@ +8,5 @@
> +topsrcdir        = @top_srcdir@
> +srcdir           = @srcdir@
> +VPATH            = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

You don't need the make lines up through here. You only need the dom-config.mk line (and only until bug 922566 kills it).

::: dom/system/gonk/Nfc.manifest
@@ +11,5 @@
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +# Copyright © 2013, Deutsche Telekom, Inc.

Why is this Apache 2 licensed? The code in here is trivial and boilerplate. I don't see why the MPL isn't acceptable.

::: layout/build/Makefile.in
@@ +137,5 @@
>    $(NULL)
>  endif #}
>  
> +ifdef MOZ_B2G_NFC #{
> +SHARED_LIBRARY_LIBS     += $(DEPTH)/dom/nfc/$(LIB_PREFIX)domnfc_s.$(LIB_SUFFIX)

SHARED_LIBRARY_LIBS are in moz.build now.

@@ +345,5 @@
>  endif #}
>  
> +ifdef MOZ_B2G_NFC #{
> +LOCAL_INCLUDES  += -I$(topsrcdir)/dom/nfc
> +endif #}

LOCAL_INCLUDES are in moz.build files now.

Comment 265 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-01 05:39:33 PDT

(In reply to Garner Lee from comment #261)
> > >+   /* Get metadata details of the discovered and connected NDEF message */
> > >+   DOMRequest detailsNDEF();
> > hmm, a bit odd method name. Sounds like an attribute name, not a getter.
> > Perhaps getDetailsNDEF() ?
> 
> It's possible. This follows the naming scheme of read and writeNDEF based on
> other feedback from Yoshi. It was earlier named
> 
>   "ndef{read, write, details}"
> 
> Any strong opinions (there's no corresponding "set" beyond writeNDEF)?
Well, read and write are clearly verbs. details not so much.
so read*, write*, getDetails* sound ok to me ( I'm so non-native-English speaker ;) ).



And about the event. I don't see the interface in the spec proposal.
There is  NFCTagEvent, which has attribute NFCTag tag, there is NFCPeerEvent which has
attribute NFCPeer peer, there is NDEFMessageEvent which has attribute NDEFMessage message, but
not anything which has attribute any message.
That 'any' makes the interface a bit hard to understand. What would the .message contain?
Also, hopefully an attribute wouldn't return a new object each time it is accessed.

Comment 266 Andreas Gal :gal 2013-10-01 08:28:41 PDT

Copyright assignment to Deutsche Telekom is ok as long its licensed under MPL2 or a compatible license (it is).

Comment 267 Garner Lee 2013-10-01 14:01:38 PDT

gps: I'm adding all the reqeusted changes, including MOZ_B2G_NFC (sorry I missed some earlier ones). MOZ_NFC applies mainly to the device builds currently (nexus-S and nexus-4), as those have NFC hardware. Copyrights: I'll go with what Andreas says.

> ::: layout/build/Makefile.in
> @@ +137,5 @@
> >    $(NULL)
> >  endif #}
> >  
> > +ifdef MOZ_B2G_NFC #{
> > +SHARED_LIBRARY_LIBS     += $(DEPTH)/dom/nfc/$(LIB_PREFIX)domnfc_s.$(LIB_SUFFIX)
> 
> SHARED_LIBRARY_LIBS are in moz.build now.
> 
> @@ +345,5 @@
> >  endif #}
> >  
> > +ifdef MOZ_B2G_NFC #{
> > +LOCAL_INCLUDES  += -I$(topsrcdir)/dom/nfc
> > +endif #}
> 
> LOCAL_INCLUDES are in moz.build files now.

A grep on the current 10/1 gecko sources sill have these two in Makefile.in not moz.build? When will the change be merged to master? Can it be done now (move only NFC into layout/build/moz.build)?

Comment 268 Garner Lee 2013-10-01 14:28:01 PDT

Upcoming patch comments: 
(In reply to Olli Pettay [:smaug] from comment #258)
> Comment on attachment 810257 [details] [diff] [review]
> Patch 1 (v11) NFC DOM IDL/WebIDL

> >+interface nsIDOMMozNfcEvent : nsIDOMEvent
> >+{
> >+  /**
> >+   * NFC Tag message data
> >+   */
> >+  [implicit_jscontext]
> >+  readonly attribute jsval message;
> >+};
> There shouldn't be any need for this interface.
Removing in next patch upload.

> 
> >+
> >+  /**
> >+   * payload - Binary data blob. The meaning of this field is application dependent.
> >+   */
> >+  readonly attribute DOMString payload;
> Ok, so this ended up being DOMString. Perhaps some example would be good
> what kind of data
> this can be.

I actually have a Uint8Array version waiting in another branch. The changes are minor on the application side, though the nfcd/gonk binary protocol is going to need a few tweaks.

> Please don't add future APIs (unless the API is actually spec'ed somewhere)
> to the .webidl
Removed by next DOM update for landing.

> 
> >+interface MozNfcEvent : Event
> >+{
> >+  [Throws]
> >+  readonly attribute any message;
> >+};
> 
> Hmm, it is a bit misfortune if a readonly attribute returns a new object
> every time it is accessed.
> Could we not have any other kind of API here?
> (I know StkCommandEvent has similar setup, but I'm not quite happy with it.)
> Where and how is the event supposed to be used?
IDL for MozNfcEvent will be removed by next patch, unless needed. The draft spec will differ from our implementation (see the earlier onpeerfound example https://bugzilla.mozilla.org/show_bug.cgi?id=674741#c260). "Event" will not be a NFCPeer object.

Comment 269 Garner Lee 2013-10-01 17:23:12 PDT

Yoshi: Powelevels. We currently define powerlevels as this (see updated 1.6 protocol):

// NFC powerlevels must match config PDUs.
const NFC_POWER_LEVEL_DISABLED       = 0;
const NFC_POWER_LEVEL_LOW            = 1;
const NFC_POWER_LEVEL_ENABLED        = 2;

They're meta states at the moment:

Low can mean a few things: screen locked, screen off, low power NFC technology presence polling (such as, leaving NFC on for airplane mode).
The integers don't denote any particular sequence. Disable means entirely off, enabled means "normal" NFC operation (when screen is on, and/or application using NFC is active).

Comment 270 :Ms2ger 2013-10-01 23:30:42 PDT

(In reply to Garner Lee from comment #267)
> gps: I'm adding all the reqeusted changes, including MOZ_B2G_NFC (sorry I
> missed some earlier ones). MOZ_NFC applies mainly to the device builds
> currently (nexus-S and nexus-4), as those have NFC hardware. Copyrights:
> I'll go with what Andreas says.
> 
> > ::: layout/build/Makefile.in
> > @@ +137,5 @@
> > >    $(NULL)
> > >  endif #}
> > >  
> > > +ifdef MOZ_B2G_NFC #{
> > > +SHARED_LIBRARY_LIBS     += $(DEPTH)/dom/nfc/$(LIB_PREFIX)domnfc_s.$(LIB_SUFFIX)
> > 
> > SHARED_LIBRARY_LIBS are in moz.build now.
> > 
> > @@ +345,5 @@
> > >  endif #}
> > >  
> > > +ifdef MOZ_B2G_NFC #{
> > > +LOCAL_INCLUDES  += -I$(topsrcdir)/dom/nfc
> > > +endif #}
> > 
> > LOCAL_INCLUDES are in moz.build files now.
> 
> A grep on the current 10/1 gecko sources sill have these two in Makefile.in
> not moz.build? When will the change be merged to master? Can it be done now
> (move only NFC into layout/build/moz.build)?

These variables are supported in moz.build per http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/sandbox_symbols.py?force=1#55 and ./mach mozbuild-reference; not all existing usage has been moved to moz.build, but new code should be added to moz.build rather than Makefile.in.

Comment 271 Yoshi Huang[:allstars.chh] 2013-10-01 23:52:16 PDT

Hi, Garner
Do you think if it's easier if we can remove those NfcA features first?

Also another reason is there's another bug discussing IsoDep, and I don't think adding interfaces like
nfcAXXX, isoDepXXX, is a good idea.

Comment 272 Garner Lee 2013-10-02 10:12:28 PDT

(In reply to Yoshi Huang(OOO ~ 10/14)[:allstars.chh][:yoshi] from comment #271)
> Hi, Garner
> Do you think if it's easier if we can remove those NfcA features first?
> 
> Also another reason is there's another bug discussing IsoDep, and I don't
> think adding interfaces like
> nfcAXXX, isoDepXXX, is a good idea.

I agree :) I have already removed them from the master branch of the SVIC gecko to simplify landing the DOM, until we think of how to better group/name the functions when we actually need them implemented.

Comment 273 Garner Lee 2013-10-02 14:09:13 PDT

(In reply to :Ms2ger from comment #270)
> (In reply to Garner Lee from comment #267)
> > gps: I'm adding all the reqeusted changes, including MOZ_B2G_NFC (sorry I
> > missed some earlier ones). MOZ_NFC applies mainly to the device builds
> > currently (nexus-S and nexus-4), as those have NFC hardware. Copyrights:
> > I'll go with what Andreas says.
> > 
> > > ::: layout/build/Makefile.in
> > > @@ +137,5 @@
> > > >    $(NULL)
> > > >  endif #}
> > > >  
> > > > +ifdef MOZ_B2G_NFC #{
> > > > +SHARED_LIBRARY_LIBS     += $(DEPTH)/dom/nfc/$(LIB_PREFIX)domnfc_s.$(LIB_SUFFIX)
> > > 
> > > SHARED_LIBRARY_LIBS are in moz.build now.
> > > 
> > > @@ +345,5 @@
> > > >  endif #}
> > > >  
> > > > +ifdef MOZ_B2G_NFC #{
> > > > +LOCAL_INCLUDES  += -I$(topsrcdir)/dom/nfc
> > > > +endif #}
> > > 
> > > LOCAL_INCLUDES are in moz.build files now.
> > 
> > A grep on the current 10/1 gecko sources sill have these two in Makefile.in
> > not moz.build? When will the change be merged to master? Can it be done now
> > (move only NFC into layout/build/moz.build)?
> 
> These variables are supported in moz.build per
> http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> frontend/sandbox_symbols.py?force=1#55 and ./mach mozbuild-reference; not
> all existing usage has been moved to moz.build, but new code should be added
> to moz.build rather than Makefile.in.

Question: How does the script to access "LIB_PREFIX" (lib) and "LIB_SUFFIX" (a)?

Comment 274 Garner Lee 2013-10-02 15:52:34 PDT

Created attachment 813356 [details] [diff] [review]
Patch 1 (v12) NFC DOM WebIDL

Compile flag MOZ_B2G_NFC --> MOZ_NFC, remove NfcEvent as unused.

Comment 275 Garner Lee 2013-10-02 15:55:05 PDT

Created attachment 813357 [details] [diff] [review]
Patch 1 (v12) NFC DOM WebIDL

Compile flag MOZ_B2G_NFC --> MOZ_NFC, remove NfcEvent as unused.

Comment 276 Garner Lee 2013-10-02 15:56:58 PDT

Created attachment 813358 [details] [diff] [review]
Patch 2 (v10) - NFC DOM Boilerplate r=bugs

MOZ_B2G_NFC --> MOZ_NFC compile flag name change. Remove debug statement.

Comment 277 Garner Lee 2013-10-02 15:59:37 PDT

Created attachment 813359 [details] [diff] [review]
Patch 3 (v11) - NFC DOM Implementation

MOZ_B2G_NFC --> MOZ_NFC name change. Payload change to Uint8Array will come as part of binary protocol updates in lower layers.

Comment 278 Garner Lee 2013-10-02 16:10:03 PDT

Created attachment 813362 [details] [diff] [review]
Patch 4 (v11) - NFC Chrome Worker Implementation

Implement requested changes.

ConfigResponse will post to settings (Bug 904246) when implemented. Likely via a system message. All other applications can just watch for the settings topic nfc.powerlevel.

Reuse Msg objects: There's a bit of string name translation between Nfc.js and nfc_worker.js, but PDU doesn't match the function names in nfc_worker.js. The binary protocol switch will update this code.

Close session will close the session, yes. Application can reconnect to get a new session to get details, read, write, and push.

detailsNDEF --> getDetailsNDEF as per suggestions.

NFC/Nfc/NDEF naming consistency updates (there's also a bit of competition between W3C draft naming, and camelCase), and misc nits applied.

Comment 279 Garner Lee 2013-10-02 16:12:57 PDT

Created attachment 813364 [details] [diff] [review]
Patch 6 (v10) - NFC Makefiles and Manifests

Update makefiles. Update gecko/layout/build/Makefile.in NFC to use moz.build instead.

Comment 280 Garner Lee 2013-10-02 16:36:54 PDT

Created attachment 813378 [details] [diff] [review]
Fix Debug ipc nfc build. Update compile flag to MOZ_NFC.

Compile flag update to patch.

Comment 281 Garner Lee 2013-10-02 16:54:37 PDT

Created attachment 813382 [details] [diff] [review]
Patch 3 (v11) - NFC DOM Implementation

MOZ_B2G_NFC --> MOZ_NFC name change. Payload change to Uint8Array will come as part of binary protocol updates in lower layers.

Comment 282 Garner Lee 2013-10-03 09:32:25 PDT

Comment on attachment 813362 [details] [diff] [review]
Patch 4 (v11) - NFC Chrome Worker Implementation

Question: The IDL files (including other modules like Volume, RIL, and now NFC) in gonk seem to be Mozilla license, and the *.js files appear to be Apache Licensed. Was there a reason for the dual license scheme in the system/gonk directory?

Comment 283 Garner Lee 2013-10-03 10:16:44 PDT

Try build: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-834d09bfdbd0/

Comment 284 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-04 07:10:05 PDT

Comment on attachment 813357 [details] [diff] [review]
Patch 1 (v12) NFC DOM WebIDL

>From 259f6e32b954a5f212e1dd8d2be709dbe999514e Mon Sep 17 00:00:00 2001
>From: Garner Lee <garner.lee@telekom.com>
>Date: Wed, 2 Oct 2013 15:07:46 -0700
>Subject: [PATCH 1/7] Patch 1 (v12) - NFC DOM IDL
>
>---
> dom/webidl/MozNdefRecord.webidl | 38 ++++++++++++++++++++++++++++++++++++++
> dom/webidl/MozNfc.webidl        | 36 ++++++++++++++++++++++++++++++++++++
> dom/webidl/moz.build            |  6 ++++++
> 3 files changed, 80 insertions(+)
> create mode 100644 dom/webidl/MozNdefRecord.webidl
>+interface MozNdefRecord
>+{
>+  /**
>+   * Type Name Field (3-bits) - Specifies the NDEF record type in general.
>+   *   tnf_empty: 0x00
>+   *   tnf_well_known: 0x01
>+   *   tnf_mime_media: 0x02
>+   *   tnf_absolute_uri: 0x03
>+   *   tnf_external type: 0x04
>+   *   tnf_unknown: 0x05
>+   *   tnf_unchanged: 0x06
>+   *   tnf_reserved: 0x07
>+   */
>+  readonly attribute octet tnf;
Actually, in which way will this be used?
Could this be an enum?

enum TFNRecordType {
  "empty",
  "well_known",
  "mime_media",
  "absolute_uri",
  "external",
  "unknown",
  "unchanged",
  "reserved"
};

Comment 285 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-04 07:15:26 PDT

And silly me. getNDEFDetails() sounds better than getDetailsNDEF()

Comment 286 Garner Lee 2013-10-04 09:17:33 PDT

(In reply to Olli Pettay [:smaug] from comment #284)
> Comment on attachment 813357 [details] [diff] [review]
> Patch 1 (v12) NFC DOM WebIDL
> 
> >From 259f6e32b954a5f212e1dd8d2be709dbe999514e Mon Sep 17 00:00:00 2001
> >From: Garner Lee <garner.lee@telekom.com>
> >Date: Wed, 2 Oct 2013 15:07:46 -0700
> >Subject: [PATCH 1/7] Patch 1 (v12) - NFC DOM IDL
> >
> >---
> > dom/webidl/MozNdefRecord.webidl | 38 ++++++++++++++++++++++++++++++++++++++
> > dom/webidl/MozNfc.webidl        | 36 ++++++++++++++++++++++++++++++++++++
> > dom/webidl/moz.build            |  6 ++++++
> > 3 files changed, 80 insertions(+)
> > create mode 100644 dom/webidl/MozNdefRecord.webidl
> >+interface MozNdefRecord
> >+{
> >+  /**
> >+   * Type Name Field (3-bits) - Specifies the NDEF record type in general.
> >+   *   tnf_empty: 0x00
> >+   *   tnf_well_known: 0x01
> >+   *   tnf_mime_media: 0x02
> >+   *   tnf_absolute_uri: 0x03
> >+   *   tnf_external type: 0x04
> >+   *   tnf_unknown: 0x05
> >+   *   tnf_unchanged: 0x06
> >+   *   tnf_reserved: 0x07
> >+   */
> >+  readonly attribute octet tnf;
> Actually, in which way will this be used?
> Could this be an enum?
> 
> enum TFNRecordType {
>   "empty",
>   "well_known",
>   "mime_media",
>   "absolute_uri",
>   "external",
>   "unknown",
>   "unchanged",
>   "reserved"
> };

It's currently defined up in gaia right now (there's a separate TBD on providing library to help developers build a valid NDEF, and W3C draft has WebIDL proposals of providing a few common NDEF types, and then failing that, a base NDEF type to fill in): https://github.com/svic/gaia/blob/master/apps/nfc-demo/js/nfc_consts.js, and in system/js/nfc_manager.js

Comment 287 Garner Lee 2013-10-04 10:07:16 PDT

> Actually, in which way will this be used?
> Could this be an enum?
> 
> enum TFNRecordType {
>   "empty",
>   "well_known",
>   "mime_media",
>   "absolute_uri",
>   "external",
>   "unknown",
>   "unchanged",
>   "reserved"
> };

To answer more specifically, I think it makes sense to consider moving unchanging constants into the DOM, though perhaps in another bug?

Naming scheme: We'll have some possibility of more NFC tech types (NFCA, IsoDep, etc), so naming schemes will need to take that into account. https://bugzilla.mozilla.org/show_bug.cgi?id=674741#c271

ex: 
getNDEFDetails, getNFCADetails, getIsoDepDetails, readNDEF, writeNDEF

versus:
getDetailsNDEF, getDetailsNDEF, getDetailsIsoDep, readNDEF, writeNDEF

Comment 288 Yoshi Huang[:allstars.chh] 2013-10-06 14:27:11 PDT

Comment on attachment 813378 [details] [diff] [review]
Fix Debug ipc nfc build. Update compile flag to MOZ_NFC.

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

I think I already reviewed Nfc.cpp before.
For moz.build, please ask review for :gps,
as he's the right guy to review this.

Comment 289 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-09 10:04:02 PDT

Comment on attachment 813357 [details] [diff] [review]
Patch 1 (v12) NFC DOM WebIDL

It is still a bit odd that tnf is octet and not an enum, but I guess
it maps quite naturally to NDEF spec where that all is defined.

I don't quite understand the setup though. MozNfc can deal with one NDEF at once.
The spec is much more flexible, there an even is dispatched and that
contains property for NDEF read/write. Why aren't we implementing the W3C draft but our own draft?

Comment 290 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-09 10:16:29 PDT

Comment on attachment 813382 [details] [diff] [review]
Patch 3 (v11) - NFC DOM Implementation


>+MozNdefRecord::Constructor(const GlobalObject& aGlobal,
>+  uint8_t aTnf, const nsAString& aType, const nsAString& aId, const nsAString& aPayload,
>+  ErrorResult& aRv)
Would be a bit more readable with parameters were aligned under the first param

>+{
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aGlobal.GetAsSupports());
>+  nsIDocument* doc;
>+  if (!win || !(doc = win->GetExtantDoc())) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
You don't use doc for anything, so just remove it.

>+#include "nsTraceRefcnt.h"
>+#include "js/GCAPI.h"
>+
>+#include "nsIDocument.h"
Why you need these?

>+//#include "nsDOMEventTargetHelper.h"
?

>+  static already_AddRefed<MozNdefRecord> Constructor(const GlobalObject& aGlobal, uint8_t aTnf, const nsAString& aType, const nsAString& aId, const nsAString& aPayload, ErrorResult& aRv);
Looks like over long line

>+const DEBUG = true;
false by default, please

>+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
>+                                   "@mozilla.org/childprocessmessagemanager;1",
>+                                   "nsIMessageSender");
This isn't used for anything

Comment 291 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-09 10:19:19 PDT

Comment on attachment 813357 [details] [diff] [review]
Patch 1 (v12) NFC DOM WebIDL

Clearing this request until it is explained why we want to expose this rather
limited API and not the W3C proposal, which seems to be a lot more flexible
and forwards compatible.

Comment 292 Garner Lee 2013-10-09 15:20:31 PDT

Just to summarize a discussion today:
1) The MozNFC api will be updated to include global functions like "getNFCTag" and "getNFCPeer" that return objects with function interfaces when the application is notified via an activity. Resolves the limitation with passing non-trivial objects in WebActivities (remote service interfaces).
2) Associate app sessions with window object.
3) The difference in API is attributable for needing to a mechanism to do application launches, and conditional message routing, as opposed to just the DOM api.

Comment 293 Gregory Szorc [:gps] 2013-10-10 15:42:13 PDT

Comment on attachment 813364 [details] [diff] [review]
Patch 6 (v10) - NFC Makefiles and Manifests

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

::: dom/dom-config.mk
@@ +56,5 @@
>  endif
>  
> +ifdef MOZ_NFC
> +DOM_SRCDIRS += dom/nfc
> +endif

Is this actually needed or are you cargo culting?

Comment 294 Gregory Szorc [:gps] 2013-10-10 15:44:00 PDT

Comment on attachment 813378 [details] [diff] [review]
Fix Debug ipc nfc build. Update compile flag to MOZ_NFC.

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

r+ does *not* encapsulate the changes to the .cpp file. I have no clue if those are safe to remove.

Comment 295 Yoshi Huang[:allstars.chh] 2013-10-13 20:10:34 PDT

Comment on attachment 813362 [details] [diff] [review]
Patch 4 (v11) - NFC Chrome Worker Implementation

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

Cancelling the r? as I'd like to review this again when the nits and naming are fixed.

::: dom/system/gonk/Nfc.js
@@ +100,5 @@
> +                                         Ci.nsINfc,
> +                                         Ci.nsIObserver]),
> +
> +  _connectedSessionId: null,
> +  onerror: function onerror(event) {

nit, add an extra line here.

@@ +115,5 @@
> +    debug("Received message: " + JSON.stringify(message));
> +    if (this.powerLevel <= NFC_POWER_LEVEL_DISABLED) {
> +      debug("NFC is not enabled.");
> +      return null;
> +    }

If we turn off nfc in Settings,
shouldn't we pass the techLost to gaia?

@@ +117,5 @@
> +      debug("NFC is not enabled.");
> +      return null;
> +    }
> +
> +

nit, remove extra line

@@ +131,5 @@
> +        this._connectedSessionId = null;
> +        gSystemMessenger.broadcastMessage("nfc-manager-tech-lost", message);
> +        ppmm.broadcastAsyncMessage("NFC:TechLost", message);
> +        break;
> +      case "NDEFDetailsResponse":

nit, GetDetailsNDEFRespone, and the same for the messages below.

@@ +159,5 @@
> +    }
> +  },
> +
> +  // nsINfcWorker
> +

nit, remove extra line

@@ +161,5 @@
> +
> +  // nsINfcWorker
> +
> +  worker: null,
> +  powerLevel: NFC_POWER_LEVEL_DISABLED,

nit, need an extra line before powerLevel

@@ +171,5 @@
> +      requestId: message.requestId
> +    };
> +    debug("NDEFDetailsRequest message out: " + JSON.stringify(outMessage));
> +
> +    this.worker.postMessage({type: "getDetailsNDEF", content: outMessage});

As I asked before, reuse the object.
Same for below.

@@ +242,5 @@
> +    debug("Received '" + message.name + "' message from content process");
> +
> +    if (this.powerLevel < NFC_POWER_LEVEL_ENABLED) {
> +      debug("NFC is not enabled.");
> +      return null;

Shouldn't we report some error here?
If the app calls some NFC api without enabling NFC,
so the call fails silently.

@@ +245,5 @@
> +      debug("NFC is not enabled.");
> +      return null;
> +    }
> +
> +    if (!message.target.assertPermission("nfc-read")) {

Where does nfc-read come from?
Also Connect also needs nfc-read permission?

@@ +254,5 @@
> +
> +    // Enforce NFC Write permissions.
> +    switch (message.name) {
> +      case "NFC:WriteNDEF":
> +        // Fall through...

Merge two lines into one.

@@ +321,5 @@
> +
> +  /**
> +   * NFC Config API. Properties is a set of name value pairs.
> +   */
> +  setNFCPowerConfig: function setNFCPowerConfig(powerLevel) {

Use consistent naming, Nfc

@@ +339,5 @@
> +      type: "ConfigRequest",
> +      powerLevel: prop.powerLevel
> +    };
> +    debug("OutMessage: " + JSON.stringify(outMessage));
> +    this.worker.postMessage({type: "configRequest", content: outMessage});

Reuse the message object.

::: dom/system/gonk/NfcContentHelper.js
@@ +37,5 @@
> +
> +const NFCCONTENTHELPER_CID =
> +  Components.ID("{4d72c120-da5f-11e1-9b23-0800200c9a66}");
> +
> +

remove extra line

@@ +46,5 @@
> +  "NFC:NDEFReadResponse",
> +  "NFC:NDEFWriteResponse",
> +  "NFC:NDEFMakeReadOnlyResponse",
> +  "NFC:ConnectResponse",
> +  "NFC:CloseResponse"

Update the naming as well.
and below.

@@ +77,5 @@
> +  _connectedSessionId: null,
> +
> +  // FIXME: btoa's will be unneeded when binary nfcd/gonk protocol is merged.
> +  encodeNdefRecords: function encodeNdefRecords(records) {
> +    var encodedRecords = [];

s/var/let/
and below.

@@ +78,5 @@
> +
> +  // FIXME: btoa's will be unneeded when binary nfcd/gonk protocol is merged.
> +  encodeNdefRecords: function encodeNdefRecords(records) {
> +    var encodedRecords = [];
> +    for(var i=0; i < records.length; i++) {

extra space after for

Comment 296 Yoshi Huang[:allstars.chh] 2013-10-13 20:17:14 PDT

(In reply to Gregory Szorc [:gps] from comment #294)
> Comment on attachment 813378 [details] [diff] [review]
> Fix Debug ipc nfc build. Update compile flag to MOZ_NFC.
> 
> Review of attachment 813378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ does *not* encapsulate the changes to the .cpp file. I have no clue if
> those are safe to remove.

I agree with gps here,
Garner, can you split this patch, or merge it into Part 6 and Part 7?

Comment 297 Garner Lee 2013-10-14 21:26:43 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #296)
> (In reply to Gregory Szorc [:gps] from comment #294)
> Garner, can you split this patch, or merge it into Part 6 and Part 7?

Ok. I'll split them in the next patch upload.

Other: 
Concerning Comment 286, we had a meeting with fabrice and Olli shortly after the post where we discussed why the API is the way it is (an Activity picker was needed), and it was agreed the way to resolve part of it is to introduce a new DOM object (the W3C defined NFCTag object, currently MozNFCTag due to differences, for example, the API has connect/close on the NFCTag). I'm currently debugging it.

Comment 298 Garner Lee 2013-10-16 19:34:43 PDT

We had a discussion. Ongoing development has NFCPeer and NFCTag implemented. We also have a "binary" protocol version

Comment 299 Yoshi Huang[:allstars.chh] 2013-10-16 19:41:11 PDT

I am okay if you want to replace the json with binary protocol here.

Can you ask reviewers if it's okay we split NFCPeer and NFCTag to another follow up bug?

Comment 300 Garner Lee 2013-10-16 19:55:33 PDT

Yoshi,

Actually, MozNFCPeer and MozNFCTag exist already in our ongoing development master (Nexus S). If the Nexus 4 supporting version (aka, "binary protocol version") is fine as the version of the DOM for landing, I'll regenerate new patches based on that (with review changes implemented).

Olli,

What isn't in the proposed patch, would be onpeerfound/onpeerlost (peer to peer) and forground dispatch (as they're still being worked on). Is it acceptable for those to come in a follow up patch?

Comment 301 Olli Pettay [:smaug] (pto-ish May 4 - May 8) 2013-10-17 00:42:04 PDT

Yeah, I think followups are fine.

Comment 302 Garner Lee 2013-10-17 18:11:47 PDT

> If we turn off nfc in Settings,
> shouldn't we pass the techLost to gaia?
> 

Should the currently connected device or tag get a techlost message automatically when the hardware (or Nfcd/NFC library) is told to do a "clean" shutdown/lower level change? The settings UI is disabled while waiting for the config message response before flipping the value.

  case "ConfigResponse":
        gSystemMessenger.broadcastMessage("nfc-powerlevel-change", message);
        break;

Comment 303 Yoshi Huang[:allstars.chh] 2013-10-20 23:44:07 PDT

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #295)
> @@ +245,5 @@
> > +      debug("NFC is not enabled.");
> > +      return null;
> > +    }
> > +
> > +    if (!message.target.assertPermission("nfc-read")) {
> 
> Where does nfc-read come from?
> Also Connect also needs nfc-read permission?
> 

It turned out we cannot use assertPermission("nfc") here,
so I am okay to check nfc-read permission even when calling connect ,...etc here.

Comment 304 Yoshi Huang[:allstars.chh] 2013-10-21 00:24:19 PDT

(In reply to Garner Lee from comment #302)
> > If we turn off nfc in Settings,
> > shouldn't we pass the techLost to gaia?
> > 
> 
> Should the currently connected device or tag get a techlost message
> automatically when the hardware (or Nfcd/NFC library) is told to do a
> "clean" shutdown/lower level change? 

Yes, nfcd will send a TechLostNotification when NFC hardware is turned off if it has fired a TechDiscoveredNotification for the same session before.

Comment 305 Garner Lee 2013-10-21 17:01:04 PDT

Created attachment 820050 [details] [diff] [review]
Patch 1 (v13) NFC DOM WebIDL

MozNFCTag, MozNFCPeer implemented. Some differences from draft standard exist still, due to use of MozActivities.

Comment 306 Garner Lee 2013-10-21 17:02:33 PDT

Created attachment 820051 [details] [diff] [review]
Patch 3 (v12) - NFC DOM Implementation

Comment 307 Garner Lee 2013-10-21 17:05:29 PDT

Created attachment 820052 [details] [diff] [review]
Patch 4 (v12) - NFC Chrome Worker Implementation

Apply review comments. App sessions. Single cast messages to contenthelper. Binary protocol.

Comment 308 Garner Lee 2013-10-21 17:08:15 PDT

Created attachment 820053 [details] [diff] [review]
Patch 6 (v11) - NFC Makefiles and Manifests

Add review comment fixes. Bundle makefile changes from gecko/ipc/nfc patch.