Closed Bug 674741 (webnfc) Opened 13 years ago Closed 11 years ago

WebNFC (near-field communication)

Categories

(Firefox OS Graveyard :: NFC, defect)

Other
Linux
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: cjones, Assigned: dgarnerlee)

References

()

Details

(Keywords: dev-doc-needed, privacy-review-needed, Whiteboard: [tech-p1][FT:RIL])

Attachments

(9 files, 139 obsolete files)

217.99 KB, application/pdf
Details
143.24 KB, application/pdf
Details
1.45 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.92 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
44.31 KB, patch
Details | Diff | Splinter Review
5.38 KB, patch
khuey
: review+
Details | Diff | Splinter Review
10.67 KB, patch
khuey
: review+
Details | Diff | Splinter Review
7.60 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1015 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
This should be somewhat similar to WebBluetooth (bug 674737).  The goal is to allow a web app on one NFC-enabled phone to be able to discover a web app on another NFC-enabled phone.  Many details TBD.
Attached file API proposal (obsolete) —
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?
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.
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
(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. :)
@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!
(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.
(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).
Alias: webnfc
Component: General → DOM: Device Interfaces
QA Contact: general → device-interfaces
there were some interesting attacks and ideas shown at cansec that we may want to be cognizant of as we move forward with this
needs to be scheduled for a team review
Whiteboard: [secr:cutisk]
Whiteboard: [secr:cutisk] → [sec-assigned:cutisk:749325]
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.
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.
Unless something has changed drastically, this is not a blocker for V1.
No longer blocks: b2g-product-phone
Attachment #618790 - Attachment is obsolete: true
Attachment #618790 - Flags: review?(kyle)
Attachment #633560 - Flags: review?(kyle)
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
Attachment #633560 - Flags: review?(kyle)
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.
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.
- 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
Attachment #633560 - Attachment is obsolete: true
Attachment #647307 - Flags: review?(kyle)
Flags: sec-review?(curtisk)
Whiteboard: [sec-assigned:cutisk:749325]
Request for top level review for Andreas Gal.
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 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.
Attachment #678619 - Flags: review?(gal)
Attached patch Gecko NFC feature patch (obsolete) — Splinter Review
Attachment #678560 - Attachment is obsolete: true
Attached patch Gaia NFC feature patch (obsolete) — Splinter Review
Attached patch Gonk-misc NFC feature patch (obsolete) — Splinter Review
Attachment #678619 - Attachment is obsolete: true
Attachment #678619 - Flags: review?(gal)
Attachment #678633 - Flags: review+
Attachment #678633 - Flags: review+
Comment on attachment 678633 [details] [diff] [review]
NFC daemon feature patch (without external project source directories)

463kb. Yeah. You guys have fun with this.
Attachment #678633 - Flags: review?(gal)
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.
Attached file libhardware.js (obsolete) —
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.
Markus, Arno, what do you guys think?
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.
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 **** 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.
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.
Attached file bindings example (obsolete) —
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?
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.
Attachment #678622 - Attachment is obsolete: true
Attachment #678623 - Attachment is obsolete: true
Attachment #678624 - Attachment is obsolete: true
Attachment #678625 - Attachment is obsolete: true
Attachment #678633 - Attachment is obsolete: true
Attachment #678622 - Flags: review?(gal)
Attachment #678623 - Flags: review?(gal)
Attachment #678624 - Flags: review?(gal)
Attachment #678625 - Flags: review?(gal)
Attachment #678633 - Flags: review?(gal)
Attachment #647307 - Flags: review?(kyle) → review-
Attachment #683765 - Attachment is obsolete: true
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;
      }
    }
Attached patch Latest WebNFC gecko patch. (obsolete) — Splinter Review
Fixed gecko over the last gecko_06.patch. Can pass DEBUG check on new xpcom object. (not fully working yet in gaia).
Attachment #689930 - Attachment is obsolete: true
Attachment #679896 - Attachment is obsolete: true
Attachment #690026 - Attachment is obsolete: true
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.
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 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.
Attachment #695046 - Flags: review-
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
(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.
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
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
Attachment #709320 - Flags: review?(vyang)
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 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.
Attachment #709320 - Flags: review?(vyang)
Attachment #695046 - Attachment is obsolete: true
Assignee: nobody → dgarnerlee
"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 on attachment 591592 [details]
API proposal

Marking old *.idl API proposal as obsolete, as they are being updated with new patches.
Attachment #591592 - Attachment is obsolete: true
Attached patch NFC DOM Interface files (obsolete) — Splinter Review
This Gecko DOM patch contains the interface changes needed to enable NFC.
Attachment #709320 - Attachment is obsolete: true
This Gecko DOM implementation code for NFC IDL files.
Attachment 713215 [details] [diff] also contains the System/gonk and IPC interface to a nfcd daemon that talks to the nfc hardware.
Attached patch NFC makefiles (obsolete) — Splinter Review
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].
Depends on: 840780
Needed for competitive parity.
Whiteboard: [tech-p1]
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) ?
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.
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.
Attachment #711512 - Attachment is obsolete: true
Attachment #711514 - Attachment is obsolete: true
Attached patch NFC gecko makefiles (obsolete) — Splinter Review
Attachment #713222 - Attachment is obsolete: true
Attachment #713215 - Attachment is obsolete: true
Attached patch NFC DOM Interface files (obsolete) — Splinter Review
Attachment #713211 - Attachment is obsolete: true
Attached patch NFC DOM JS, and permissions. (obsolete) — Splinter Review
Attachment #713220 - Attachment is obsolete: true
Attachment #695045 - Attachment is obsolete: true
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?
Hi Vicamo,

Good point. I'll check the code origin, and reupload the patch as needed.
Removed obsolete telephony Icc{Open,Close,ExchangeAPDU} in favor of IccManager.
Attachment #730468 - Attachment is obsolete: true
Removed obsolete Icc{Open,Close,ExchangeAPDU} in telephony from last patch.
Attachment #730470 - Attachment is obsolete: true
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.
Attachment #730471 - Flags: review?(curtisk)
(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.
Flags: sec-review?(ptheriault)
Flags: sec-review?(curtisk)
Flags: needinfo?(ptheriault)
Attachment #730471 - Flags: review?(curtisk) → review?(ptheriault)
Attachment #647307 - Attachment is obsolete: true
Attachment #678896 - Attachment is obsolete: true
Attachment #679491 - Attachment is obsolete: true
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.
Attachment #730465 - Attachment is obsolete: true
Attachment #730998 - Attachment description: NFC DOM Interface files → Patch 1 (v1) - NFC DOM Interface files
Comment on attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.

Obsoleting and moving to bug 860910
Comment on attachment 730473 [details] [diff] [review]
System app update. Platform integration with NFC MozActivities.

Obsoleting and moving to bug 860910
Attachment #730473 - Attachment is obsolete: true
Comment on attachment 683781 [details] [diff] [review]
Proposed changes to have B2G system property to enable/disable nfc.

Moving to bug 860907
Attachment #683781 - Attachment is obsolete: true
Comment on attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.

Move to bug 860910
Attachment #730474 - Attachment is obsolete: true
Attachment #730998 - Attachment is obsolete: true
Attachment #730997 - Attachment is obsolete: true
Attachment #730467 - Attachment is obsolete: true
Attachment #730467 - Flags: review?(kyle)
Attachment #730471 - Attachment is obsolete: true
Attachment #730471 - Flags: review?(ptheriault)
Attached patch Patch 1 (v1) - NFC DOM IDL (obsolete) — Splinter Review
Attachment #736600 - Flags: superreview?(mrbkap)
Attachment #736600 - Flags: review?(kyle)
Attachment #736602 - Flags: superreview?(mrbkap)
Attachment #736602 - Flags: review?(kyle)
Attachment #736603 - Flags: superreview?(mrbkap)
Attachment #736603 - Flags: review?(kyle)
There's a good chance this is getting rewritten as a UnixSocket implementation, but putting it up for completeness sake for the moment.
Attachment #736607 - Flags: review?(kyle)
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.
(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.
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
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)?
Flags: needinfo?(ptheriault) → needinfo?(dgarnerlee)
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)
Attachment #736605 - Flags: review?(ptheriault) → review+
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.
(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.
Flags: needinfo?(dgarnerlee)
Remove obsolete Permissions.txt changes.
Attachment #736605 - Attachment is obsolete: true
Attachment #737939 - Flags: review+
Attachment #737939 - Flags: review+ → review?(ptheriault)
Attachment #737939 - Flags: review?(ptheriault) → review+
Attached patch Patch 1 (v2) - NFC DOM IDL (obsolete) — Splinter Review
Update jsval field to nsString
Attachment #736600 - Attachment is obsolete: true
Attachment #736600 - Flags: superreview?(mrbkap)
Attachment #736600 - Flags: review?(kyle)
Attachment #740998 - Flags: superreview?(mrbkap)
Attachment #740998 - Flags: review?(kyle)
Update jsval field to nsString.
Attachment #736603 - Attachment is obsolete: true
Attachment #736603 - Flags: superreview?(mrbkap)
Attachment #736603 - Flags: review?(kyle)
Attachment #740999 - Flags: superreview?(mrbkap)
Attachment #740999 - Flags: review?(kyle)
Update jsval field to nsString.
Attachment #736604 - Attachment is obsolete: true
Attachment #736604 - Flags: review?(kyle)
Attachment #741004 - Flags: superreview?(mrbkap)
Attachment #741004 - Flags: review?(kyle)
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.
Attachment #736607 - Flags: review?(kyle) → review-
Blocks: 866907
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?
Attachment #740998 - Flags: superreview?(mrbkap)
Attachment #740998 - Flags: review?(kyle)
Attachment #740998 - Flags: review-
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 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.
Attachment #736602 - Flags: superreview?(mrbkap)
Attachment #736602 - Flags: review?(kyle)
Attachment #736602 - Flags: review-
(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?
Implement review comments: #ifdef for 2 onndefdiscovered, onndefdisconnected, rename MozNdefRecord to NdefRecord.
Attachment #736602 - Attachment is obsolete: true
Attached patch Patch 1 (v3) - NFC DOM IDL (obsolete) — Splinter Review
Implement review comments: Comment NdefRecord fields, MozNdefRecord --> NdefRecord
Attachment #740998 - Attachment is obsolete: true
Attachment #743943 - Flags: superreview?(mrbkap)
Attachment #743943 - Flags: review?(kyle)
Implement review comments: MozNdefRecord --> NdefRecord
Attachment #740999 - Attachment is obsolete: true
Attachment #740999 - Flags: superreview?(mrbkap)
Attachment #740999 - Flags: review?(kyle)
Attachment #743944 - Flags: superreview?(mrbkap)
Attachment #743944 - Flags: review?(kyle)
Implement review comments: Rename MozNdefRecord --> NdefRecord
Attachment #736606 - Attachment is obsolete: true
Attachment #736606 - Flags: review?(kyle)
Attachment #743947 - Flags: review?(kyle)
Attachment #743918 - Flags: review?(kyle)
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.
(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.
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.
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.
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.
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.
Attachment #758311 - Flags: review+
Attachment #758311 - Attachment description: Patch 7 (v1) - NFC B2G IPC Implementation → Patch 7 (v2) - NFC B2G IPC Implementation
Patch 4 :  NFC Chrome Worker Implementation v3 - Modifies the NFC chrome worker to support UnixSocket implementation.
Attachment #758319 - Flags: superreview?(mrbkap)
Attachment #758319 - Flags: review?
Attachment #758319 - Flags: review? → review?(kyle)
Attachment #758311 - Flags: review+ → review?(kyle)
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.
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.
Here is the new bug that tracks NFC IPC changes : https://bugzilla.mozilla.org/show_bug.cgi?id=879821
Attachment #736607 - Attachment is obsolete: true
Attachment #758311 - Attachment is obsolete: true
Attachment #758311 - Flags: review?(kyle)
Attachment #741004 - Attachment is obsolete: true
Attachment #741004 - Flags: superreview?(mrbkap)
Attachment #741004 - Flags: review?(kyle)
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.
Attachment #743918 - Flags: review?(kyle)
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.
Attachment #743943 - Flags: superreview?(mrbkap)
Attachment #743943 - Flags: review?(kyle)
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.
Attachment #743944 - Flags: superreview?(mrbkap)
Attachment #743944 - Flags: review?(kyle)
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
Attachment #743947 - Flags: review?(kyle)
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.
Attachment #758319 - Flags: superreview?(mrbkap)
Attachment #758319 - Flags: review?(kyle)
Attached patch Patch 1 (v4) - NFC DOM IDL (obsolete) — Splinter Review
Added WebIDL support for NfcEvent object.
Attachment #743943 - Attachment is obsolete: true
Attachment #743918 - Attachment is obsolete: true
Attachment #743944 - Attachment is obsolete: true
Attachment #758319 - Attachment is obsolete: true
Attachment #737939 - Attachment is obsolete: true
Attachment #743947 - Attachment is obsolete: true
Attachment #768421 - Flags: review?(ptheriault)
NDEF Protocol updated with additional states.
Attachment #772296 - Attachment is obsolete: true
Update for new worker API (7/12 merge).
Attachment #768420 - Attachment is obsolete: true
"Patch 4 (v5) - NFC Chrome Worker Implementation" does not have the new NFC JSON API. That will come soon.
Attachment #776536 - Attachment description: This pdf describes about NFC architecture on FxOS and the responsibilities shared between various teams. → (v1) This pdf describes about NFC architecture on FxOS and the responsibilities shared between various teams.
Attachment #776536 - Attachment description: (v1) This pdf describes about NFC architecture on FxOS and the responsibilities shared between various teams. → (v1) This pdf describes the NFC architecture and the responsibilities shared between various teams.
(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?
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
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.
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.
(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
(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.
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
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(;).
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
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.
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
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.
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.
Flags: needinfo?(dgarnerlee)
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.
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?
Flags: needinfo?(dgarnerlee)
(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.
(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. :)
(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.
(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
Attached patch Patch 1 (v5) - NFC DOM IDL (obsolete) — Splinter Review
Attachment #768416 - Attachment is obsolete: true
Attachment #768417 - Attachment is obsolete: true
Attachment #768419 - Attachment is obsolete: true
Attachment #768421 - Attachment is obsolete: true
Attachment #768421 - Flags: review?(ptheriault)
Attachment #768423 - Attachment is obsolete: true
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
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.
(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 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
Attached patch Patch 1 (v6) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
Fixes reported crash issue in DEBUG compile (remove extra NfcEvent registration).
Attachment #780052 - Attachment is obsolete: true
Attachment #780053 - Attachment is obsolete: true
Attachment #780054 - Attachment is obsolete: true
Remove Secure Element API (now tracked in separate feature bug).
Attachment #776039 - Attachment is obsolete: true
Attachment #780055 - Attachment is obsolete: true
Attachment #780056 - Attachment is obsolete: true
(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...
Hardware: x86_64 → Other
(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).
(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.
Attached file NFC Component.pdf
> Hi Thomas, code is rebased again, and DEBUG and disabling MOZ_B2G_NFC should
> now work.

Thanks a lot. I'll try ASAP.
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.
(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
(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).
Attached patch Patch 1 (v7) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
Attachment #781128 - Attachment is obsolete: true
Attachment #781129 - Attachment is obsolete: true
Attachment #781130 - Attachment is obsolete: true
Attachment #781132 - Attachment is obsolete: true
Attachment #781133 - Attachment is obsolete: true
Attachment #781135 - Attachment is obsolete: true
Attached patch Patch 7 (v2) NFC Gonk Misc (obsolete) — Splinter Review
Gonk-misc repo system startup files and config (nfcd related).
Attachment #787980 - Flags: superreview?(jonas)
Attachment #787985 - Flags: superreview?(jonas)
Attachment #787989 - Flags: superreview?
Attachment #787994 - Attachment description: PATCH 7 (v2) NFC Gonk Misc → Patch 7 (v2) NFC Gonk Misc
Attachment #787989 - Flags: superreview? → superreview?(jonas)
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?
@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.
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 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?
(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.
Blocks: 894323
Blocks: 894689
blocking-b2g: --- → koi+
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.
Blocks: 894672, 904246, 894678, 894676, 894320, 894673, 894691
No longer blocks: b2g-nfc
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.
Attachment #787980 - Flags: superreview?(jonas)
Attachment #787985 - Flags: superreview?(jonas) → superreview+
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.
Attachment #787989 - Flags: superreview?(jonas) → superreview?
Component: DOM: Device Interfaces → NFC
Product: Core → Boot2Gecko
Version: Trunk → unspecified
(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 on attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation

Olli will help to review this patch.
Attachment #787989 - Flags: superreview? → review?(bugs)
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.
Attachment #787989 - Flags: review-
If you need help figuring out how to fix this up feel free to send email.
Attached patch Patch 1 (v8) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
Updates. Add config and ndefMakeReadOnly to DOM/nfcd.
Attachment #787980 - Attachment is obsolete: true
Attachment #787985 - Attachment is obsolete: true
Attachment #800278 - Flags: superreview?(jonas)
(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.
Attachment #787990 - Attachment is obsolete: true
Attachment #787992 - Attachment is obsolete: true
Comment on attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes

Add tighter permissions to read and write NFC calls.
Attachment #787993 - Attachment is obsolete: true
Attachment #800288 - Attachment description: atch 4 (v8) - NFC Chrome Worker Implementation → Patch 4 (v8) - NFC Chrome Worker Implementation
(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.
Attachment #800301 - Flags: review?(allstars.chh)
The last patch set for gecko is based on: 2a2ff267b86346ae80e453a814c442da82583ab1
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.
Attachment #787989 - Flags: review?(bugs) → review-
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.
Attachment #800301 - Flags: review?(allstars.chh) → review+
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);
Attachment #800301 - Attachment description: Fix debug build - remove unneeded NS_IsMainThread assertion → Fix debug build - remove unneeded NS_IsMainThread assertion r=allstars.chh
Attachment #800289 - Flags: review?(ptheriault)
Attachment #800290 - Flags: review?
Attachment #800277 - Flags: review?(jonas)
Attachment #787989 - Attachment is obsolete: true
Attachment #800290 - Flags: review? → review?(khuey)
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.
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.
Garner, please do file a followup bug for the WebIDL work and assign to yourself before landing this bug though.
(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.
Obviously security bugs needs to be fixed before we can land anything.
Attached patch Patch 7 (v3) NFC Gonk Misc (obsolete) — Splinter Review
Remove nfc.enabled system property from init patch.
Attachment #787994 - Attachment is obsolete: true
Attachment #802122 - Flags: review?(mwu)
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.
Attachment #802122 - Flags: review?(mwu)
(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).
Attached patch Patch 7 (v4) NFC Gonk Misc r=mwu (obsolete) — Splinter Review
As per conversation, we're checking Nfc in disabled initially, so the enable flag is removed. Patch updated.
Attachment #802122 - Attachment is obsolete: true
Attachment #802341 - Flags: review?(mwu)
Attachment #802341 - Flags: review?(mwu) → review+
Attachment #802341 - Attachment description: Patch 7 (v4) NFC Gonk Misc → Patch 7 (v4) NFC Gonk Misc r=mwu
Blocks: 915008
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.
Attachment #800289 - Flags: review?(ptheriault) → review+
Attached patch Patch 1 (v9) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
Attachment #800277 - Attachment is obsolete: true
Attachment #800277 - Flags: review?(jonas)
Attachment #803761 - Flags: review?(bugs)
Attachment #800278 - Attachment is obsolete: true
Attachment #800278 - Flags: superreview?(jonas)
Attachment #803763 - Flags: review?(bugs)
Review comments implemented: rooted playload field in NdefRecord, and additional type checks (String) for security.
Attachment #800284 - Attachment is obsolete: true
Attachment #803764 - Flags: superreview?
Attachment #803764 - Flags: superreview? → superreview?(bugs)
Note: this is the non-binary protocol version
Attachment #800288 - Attachment is obsolete: true
Attachment #803769 - Flags: superreview?(bugs)
MOZ_B2G_NFC now set to build, but operate only if nfc.powerlevel is 1 (enabled).
Attachment #800290 - Attachment is obsolete: true
Attachment #800290 - Flags: review?(khuey)
Attachment #803771 - Flags: review?
(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?
Attachment #803771 - Flags: review? → review?(gps)
Attachment #800289 - Flags: review+ → review?(fabrice)
Attached patch Patch 1 (v10) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
Add MOZ_B2G_NFC Compile flag around MozNfcEvent.webidl in new moz.build.
Attachment #803761 - Attachment is obsolete: true
Attachment #803761 - Flags: review?(bugs)
Attachment #804387 - Flags: superreview?
Comment on attachment 803769 [details] [diff] [review]
Patch 4 (v9) - NFC Chrome Worker Implementation

This touches Workers -> bent
Attachment #803769 - Flags: superreview?(bugs) → superreview?(bent.mozilla)
I don't have access to bug 913336, and I think I should have before I can review this stuff.
(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 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.
Attachment #804387 - Flags: superreview? → superreview-
Attachment #804387 - Attachment description: 0001-PATCH-1-7-Patch-1-v10-NFC-DOM-IDL.patch → Patch 1 (v10) NFC DOM IDL/WebIDL
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.
Attachment #803763 - Flags: review?(bugs) → review-
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
Attachment #803764 - Flags: superreview?(bugs) → superreview-
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.
Attachment #803771 - Flags: review?(gps) → feedback+
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 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.
Attachment #800289 - Flags: review?(fabrice) → feedback+
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.
(Note, after WebIDLification that JS_GetElement/JS_GetClass()->name thing can be removed, as far as I see)
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
(In reply to Olli Pettay [:smaug] from comment #245)
> #content IRC channel.

Thanks for the pointer to the IRC channel.
blocking-b2g: koi+ → 1.3?
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
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.
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.
Attached patch Patch 1 (v11) NFC DOM IDL/WebIDL (obsolete) — Splinter Review
WebIDL conversion of navigator.mozNfc (in JS), NfcEvent (C++), MozNdefRecord (C++).
Attachment #804387 - Attachment is obsolete: true
Attachment #810257 - Flags: review?(bugs)
Attachment #803763 - Attachment is obsolete: true
Attachment #810258 - Flags: review?(bugs)
nsNfc.js navigator WebIDL DOM implementation.
Attachment #803764 - Attachment is obsolete: true
Attachment #810259 - Flags: review?(bugs)
Attachment #803769 - Attachment is obsolete: true
Attachment #803769 - Flags: superreview?(bent.mozilla)
Attachment #810261 - Flags: review?(bent.mozilla)
Move nfc-manager back down to a simple permission declaration without "read".
Attachment #800289 - Attachment is obsolete: true
Attachment #810262 - Flags: review?(fabrice)
Updated makefiles for WebIDL implementation.
Attachment #803771 - Attachment is obsolete: true
Attachment #810266 - Flags: review?(gps)
Hi, the try server output of the current patch set is here:

https://tbpl.mozilla.org/?tree=Try&rev=d4bf6d92bc2a
Attachment #810262 - Flags: review?(fabrice) → review+
Attachment #810262 - Attachment description: Patch 5 (v8) - NFC Permissions Changes → Patch 5 (v8) - NFC Permissions Changes r=fabrice
blocking-b2g: 1.3? → 1.3+
Whiteboard: [tech-p1] → [tech-p1][FT:RIL]
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...);
Attachment #810258 - Flags: review?(bugs) → review+
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.
Attachment #810257 - Flags: review?(bugs) → review-
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.
Attachment #810259 - Flags: review?(bugs)
(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).
> >+   /* 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 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.
Attachment #810261 - Flags: review?(bent.mozilla) → review?(allstars.chh)
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.
Attachment #810261 - Flags: review?(allstars.chh) → review-
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.
Attachment #810266 - Flags: review?(gps)
Attachment #810266 - Flags: review-
Attachment #810266 - Flags: feedback?(gerv)
(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.
Copyright assignment to Deutsche Telekom is ok as long its licensed under MPL2 or a compatible license (it is).
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)?
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.
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).
(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.
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.
(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.
(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)?
Attached patch Patch 1 (v12) NFC DOM WebIDL (obsolete) — Splinter Review
Compile flag MOZ_B2G_NFC --> MOZ_NFC, remove NfcEvent as unused.
Attachment #810257 - Attachment is obsolete: true
Attachment #813356 - Flags: review?
Attached patch Patch 1 (v12) NFC DOM WebIDL (obsolete) — Splinter Review
Compile flag MOZ_B2G_NFC --> MOZ_NFC, remove NfcEvent as unused.
Attachment #813357 - Flags: review?
MOZ_B2G_NFC --> MOZ_NFC compile flag name change. Remove debug statement.
Attachment #810258 - Attachment is obsolete: true
Attachment #813358 - Flags: review?
MOZ_B2G_NFC --> MOZ_NFC name change. Payload change to Uint8Array will come as part of binary protocol updates in lower layers.
Attachment #810259 - Attachment is obsolete: true
Attachment #813359 - Flags: review?
Attachment #813356 - Attachment is obsolete: true
Attachment #813356 - Flags: review?
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.
Attachment #810261 - Attachment is obsolete: true
Attachment #813362 - Flags: review?(allstars.chh)
Update makefiles. Update gecko/layout/build/Makefile.in NFC to use moz.build instead.
Attachment #810266 - Attachment is obsolete: true
Attachment #810266 - Flags: feedback?(gerv)
Attachment #813364 - Flags: review?(gps)
Attachment #813357 - Flags: review? → review?(bugs)
Attachment #813358 - Flags: review? → review?(bugs)
Attachment #813359 - Flags: review? → review?(bugs)
Compile flag update to patch.
Attachment #800301 - Attachment is obsolete: true
Attachment #813378 - Flags: review?(allstars.chh)
MOZ_B2G_NFC --> MOZ_NFC name change. Payload change to Uint8Array will come as part of binary protocol updates in lower layers.
Attachment #813359 - Attachment is obsolete: true
Attachment #813359 - Flags: review?(bugs)
Attachment #813382 - Flags: review?(bugs)
Attachment #813362 - Flags: review?(allstars.chh)
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?
Attachment #813362 - Flags: review?(allstars.chh)
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"
};
And silly me. getNDEFDetails() sounds better than getDetailsNDEF()
(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
> 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 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.
Attachment #813378 - Flags: review?(allstars.chh)
Attachment #813378 - Flags: review?(gps)
Attachment #813358 - Flags: review?(bugs) → review+
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 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
Attachment #813382 - Flags: review?(bugs) → review+
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.
Attachment #813357 - Flags: review?(bugs)
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 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?
Attachment #813364 - Flags: review?(gps) → review+
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.
Attachment #813378 - Flags: review?(gps) → review+
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
Attachment #813362 - Flags: review?(allstars.chh)
(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?
(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.
We had a discussion. Ongoing development has NFCPeer and NFCTag implemented. We also have a "binary" protocol version
Flags: needinfo?(allstars.chh)
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?
Flags: needinfo?(allstars.chh)
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?
Flags: needinfo?(bugs)
Yeah, I think followups are fine.
Flags: needinfo?(bugs)
> 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;
Flags: needinfo?(allstars.chh)
(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.
(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.
Flags: needinfo?(allstars.chh)
Attached patch Patch 1 (v13) NFC DOM WebIDL (obsolete) — Splinter Review
MozNFCTag, MozNFCPeer implemented. Some differences from draft standard exist still, due to use of MozActivities.
Attachment #820050 - Flags: review?(bugs)
Attachment #813382 - Attachment is obsolete: true
Attachment #820051 - Flags: review?(bugs)
Attachment #813357 - Attachment is obsolete: true
Apply review comments. App sessions. Single cast messages to contenthelper. Binary protocol.
Attachment #813362 - Attachment is obsolete: true
Attachment #820052 - Flags: review?(allstars.chh)
Add review comment fixes. Bundle makefile changes from gecko/ipc/nfc patch.
Attachment #813364 - Attachment is obsolete: true
Attachment #820053 - Flags: review?(gps)
Move makefiles out to patch 6. IPC patch is the same.
Attachment #813378 - Attachment is obsolete: true
Attachment #820056 - Flags: review?(allstars.chh)
Attachment #813358 - Attachment description: Patch 2 (v10) - NFC DOM Boilerplate → Patch 2 (v10) - NFC DOM Boilerplate r=bugs
Move makefile changes from gecko/ipc into patchset. (update) Removed obsolete MozNfcEvent reference in Bindings.conf.
Attachment #820053 - Attachment is obsolete: true
Attachment #820053 - Flags: review?(gps)
Attachment #820086 - Flags: review?(gps)
Comment on attachment 820056 [details] [diff] [review]
0007-Fix-Debug-ipc-nfc-build.patch r=allstars.chh

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

Please update the patch title if you won't mege this to other parts.
And add r=me
Attachment #820056 - Flags: review?(allstars.chh) → review+
(In reply to Garner Lee from comment #305)
> 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.

I thought Comment 301 also said it's okay we do it in follow-ups.
Comment on attachment 820052 [details] [diff] [review]
Patch 4 (v12) - NFC Chrome Worker Implementation

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

Hi Garner
Can you address those nits I said before?
You can search through Bugzilla and you can find we do care about some extra spaces, lines, indents.
It's a huge patch, and if the patch is cleaner and simpler,
it will definitely help the reviewers to review.

Also I know I wrote some prototypes for you to reuse the worker_buf.js from nfc_worker.
Can you revise it a little bit to make it fit into Mozilla Coding Style?

I have mentioned below, the modifications for SystemWorkerManager need to go to other places now.

Please explain the new added setSessionToken function.

::: dom/system/gonk/Nfc.js
@@ +21,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var NFC = {};

let

@@ +25,5 @@
> +var NFC = {};
> +Cu.import("resource://gre/modules/nfc_consts.js", NFC);
> +
> +// set to true in nfc_consts.js to see debug messages
> +var DEBUG = NFC.DEBUG_NFC;

let

@@ +41,5 @@
> +const NFC_CID =
> +  Components.ID("{2ff24790-5e74-11e1-b86c-0800200c9a66}");
> +
> +const NFC_IPC_MSG_NAMES = [
> +  "NFC:SetSessionToken",

Why did you add this?
Can you move this to other parts?

If you keep adding new things in the same patch,
it's difficult for us to review this.

@@ +53,5 @@
> +
> +// NFC powerlevels must match config PDUs.
> +const NFC_POWER_LEVEL_DISABLED       = 0;
> +const NFC_POWER_LEVEL_LOW            = 1;
> +const NFC_POWER_LEVEL_ENABLED        = 2;

Can't we move these into nfc_consts?

@@ +59,5 @@
> +const TOPIC_MOZSETTINGS_CHANGED      = "mozsettings-changed";
> +const TOPIC_XPCOM_SHUTDOWN           = "xpcom-shutdown";
> +const SETTING_NFC_ENABLED            = "nfc.enabled";
> +
> +

nit, remove extra line

@@ +75,5 @@
> +                                   "nsISettingsService");
> +XPCOMUtils.defineLazyServiceGetter(this, "UUIDGenerator",
> +                                    "@mozilla.org/uuid-generator;1",
> +                                    "nsIUUIDGenerator");
> +XPCOMUtils.defineLazyGetter(this, "gMessageManager", function () {

Why did you need this?
Can't ppmm server your purpose?
RIL uses this because it is shared by many components, ICC, MobileConnection, Telephony, ...etc.

Why did you add 150 lines doing the same thing you did before?

@@ +248,5 @@
> +      }
> +
> +      switch (msg.name) {
> +        case "NFC:SetSessionToken":
> +          this._registerMessageTarget(this.nfc.tokenSessionMap[this.nfc._connectedSessionId], msg.target);

Please use consistent naming between
SessionId,
SessionToken,
TokenSession

@@ +303,5 @@
> +  this.powerLevel = NFC_POWER_LEVEL_DISABLED;
> +  lock.get(SETTING_NFC_ENABLED, this);
> +  this.sessionMap = [];
> +  // Maps sessionIds (that are generated from nfcd) with a unique guid
> +  this.tokenSessionMap = {};

What's the difference between a SessionMap and tokenSessionMap?

@@ +322,5 @@
> +                                         Ci.nsIObserver,
> +                                         Ci.nsISettingsServiceCallback]),
> +
> +  _connectedSessionId: null,
> +  _enabled: false,

I think I have mentioned at least twice,
please add an extra line here.

@@ +359,5 @@
> +     case "ConfigResponse":
> +        gSystemMessenger.broadcastMessage("nfc-powerlevel-change", message);
> +        break;
> +      case "ConnectResponse":
> +        // Fall through.

Merge these two lines.

@@ +430,5 @@
> +    }
> +
> +    switch (message.name) {
> +      case "NFC:GetDetailsNDEF":
> +        this.worker.postMessage({type: "getDetailsNDEF", content: message.json});

Why do we need another level 'content' ?

Can't we just 
message.type = "getDetailsNDEF";
this.worker.postMessage(message);

@@ +470,5 @@
> +        } else {
> +          this.setNFCPowerConfig(NFC_POWER_LEVEL_DISABLED);
> +          this._enabled = false;
> +        }
> +      break;

nit, indent.

@@ +515,5 @@
> +    };
> +    let outMessage = {
> +      type: "ConfigRequest",
> +      powerLevel: prop.powerLevel
> +    };

reuse the object.

::: dom/system/gonk/NfcContentHelper.js
@@ +23,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ObjectWrapper.jsm");
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +
> +var NFC = {};

let

@@ +79,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++) {
> +      var record = records[i];

let

@@ +91,5 @@
> +    return encodedRecords;
> +  },
> +
> +  // NFC interface:
> +  setSessionToken: function setSession(sessionToken) {

setSessionToken

@@ +322,5 @@
> +
> +  handleDetailsNDEFResponse: function handleDetailsNDEFResponse(message) {
> +    debug("DetailsNDEFResponse(" + JSON.stringify(message) + ")");
> +    let requester = this._requestMap[message.requestId];
> +    if (typeof requester === 'undefined') {

if (!requester) {

}

@@ +329,5 @@
> +    delete this._requestMap[message.requestId];
> +    let result = message.content;
> +    let requestId = atob(message.requestId);
> +
> +    if (message.content.status != "OK") {

Do you think it's easier if we have a boolean for this?
Also what does content in message.content.status mean?
Can't we use message directly?

like 
if (message.success)

@@ +358,5 @@
> +      }
> +      r.id = id;
> +      let payload = "";
> +      for (let i = 0; i < r.payload.length; i++) {
> +        payload += String.fromCharCode(r.payload[i]);

payload is DOMString?

@@ +375,5 @@
> +
> +  handleWriteNDEFResponse: function handleWriteNDEFResponse(message) {
> +    debug("WriteNDEFResponse(" + JSON.stringify(message) + ")");
> +    let requester = this._requestMap[message.requestId];
> +    if (typeof requester === 'undefined') {

ditto

@@ +436,5 @@
> +    delete this._requestMap[message.requestId];
> +    let result = message.content;
> +    let requestId = atob(message.requestId);
> +
> +

extra line here.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +24,5 @@
> +#include "mozilla/ipc/Nfc.h"
> +#include "Nfc.h"
> +#include <unistd.h>
> +#include <cutils/properties.h>
> +#endif

See Bug 920551, these code may need to move to ipc/nfc/Nfc.cpp

::: dom/system/gonk/nfc_consts.js
@@ +45,5 @@
> +  1:'NDEF',
> +  2:'NDEF_FORMATTABLE',
> +  3:'NFC_A',
> +  9:'MIFARE_ULTRALIGHT'
> +};

The enum here is different from Gonk protocol.
Why ?

@@ +48,5 @@
> +  9:'MIFARE_ULTRALIGHT'
> +};
> +
> +this.GECKO_NFC_ERROR_SUCCESS = "OK";
> +this.GECKO_NFC_ERROR_GENERIC_FAILURE = "FAILURE";

Can't we use a boolean for this?

What if someday there's typo said

message.result = "Ok"; // k is lower-case

Do you think how much time we will spend on it to find out this bug?

::: dom/system/gonk/nfc_worker.js
@@ +85,5 @@
> + * where JSON is sent and received from and translated into API calls.
> + * For the most part, this object is pretty boring as it simply translates
> + * between method calls and NFC JSON. Somebody's gotta do the job...
> + */
> +let Nfc = {

You have an object in Nfc.js called NFC,
and another object called Nfc in nfc_worker.js ?

@@ +160,5 @@
> +      }
> +
> +      message.requestId = message.content.requestId;
> +      message.sessionId = sessionId;
> +      message.content.records = records;

Why do we need another object 'content' here?

@@ +168,5 @@
> +
> +    Buf.newParcel(NFC_REQUEST_READ_NDEF, cb);
> +    Buf.writeInt32(message.content.sessionId);
> +    Buf.sendParcel();
> +  },

Can you remove the debug first?
I don't think they are neccesary.

@@ +312,5 @@
> +  /**
> +   * NFC Configuration
> +   */
> +  configRequest: function configRequest(message) {
> +    debug("config:"+JSON.stringify(message.content));

doing nothing here?

@@ +339,5 @@
> +    let method = this[request_type];
> +    if (typeof method == "function") {
> +      if (DEBUG) debug("Handling parcel as " + method.name);
> +      method.call(this, length);
> +    } else if (callback && typeof callback == "function") {

isn't if (typeof callback) enough?

@@ +340,5 @@
> +    if (typeof method == "function") {
> +      if (DEBUG) debug("Handling parcel as " + method.name);
> +      method.call(this, length);
> +    } else if (callback && typeof callback == "function") {
> +      callback.call(this, request_type);

Do we need to pass 'length' to callback also?

@@ +342,5 @@
> +      method.call(this, length);
> +    } else if (callback && typeof callback == "function") {
> +      callback.call(this, request_type);
> +      delete this.mCallback;
> +      this.mCallback = null;

Why do we need these two lines? (345~346)

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +13,5 @@
> +{
> +  void registerNfcCallback(in nsINfcCallback callback);
> +  void unregisterNfcCallback(in nsINfcCallback callback);
> +
> +  boolean setSessionToken(in DOMString sessionToken);

Why this?
Attachment #820052 - Flags: review?(allstars.chh) → review-
Attachment #820056 - Attachment description: 0007-Fix-Debug-ipc-nfc-build.patch → 0007-Fix-Debug-ipc-nfc-build.patch r=allstars.chh
Hi Yoshi,

DOMStrings for payload, id, type, there's work to move to Uint8Array, though JS implemented WebIDL objects seem to be missing a feature. IRC #content discussion today:

<mccr8> yeah looks like not.   in TestJSImplGen it says:   // ArrayBuffer is handled differently in callback interfaces and the example generator.
<mccr8>   // Need to figure out what should be done there.  Seems like other typed array stuff is
<mccr8>   // similarly not working in the JS implemented generator.  Probably some other issues
<mccr8>   // here as well.

The implementation have to stick to C++ for now to use Uint8Array (I'll open a JS generator bug). That change will be converted in a separate patch (to keep patch changes low for review).
Bug 929609 opened for JS implemented WebIDL Uint8Array.
Blocks: 903245
Fixes applied. Extra comments on comment reviews to follow.
Attachment #820052 - Attachment is obsolete: true
Attachment #820772 - Flags: review?(allstars.chh)
Hi Yoshi,

We went through all your previous comments and the current ones. We should have now addressed all your comments / concerns. However following are some of the points that needs extra explanation.

Following are the points that we would like to discuss / answer.

1. s/NFC/Nfc/
2. Consistent naming please.
3. Sometimes you use NFC, sometimes 'Nfc'.

<<< Consolidated all your review comments at various points on this topic >>>
 
Ans.) We have addressed our naming convention. Introduced 'NfcWorker' for nfc chrome worker obj.
      However we retained "NFC:XXXX" IPC messaging naming convention on lines of RIL (Refered to "RIL:" for RIL's IPC Message prefix)

4. What's the difference between setNfcPowerConfig and setConfig? 
Ans.) We need to add more properties to SetConfig including screen on/off states to setConfig. setNfcPowerConfig is just a wrapper over setConfig()

5. If we turn off nfc in Settings,shouldn't we pass the techLost to gaia?
Ans.) Yes, good point. Should 'nfcd' send 'tech-lost' in this case (only if connected). Also refer to comment#304 from Garner

6. Shouldn't we report some error here? If the app calls some NFC api without enabling NFC,
so the call fails silently.
Ans.) Agreed! At this moment Nfc.js simply 'throws' an error - "throw new Error("NFC is not enabled.");"

7. Why did you need this? Can't ppmm server your purpose?
RIL uses this because it is shared by many components, ICC, MobileConnection, Telephony, ...etc.
Why did you add 150 lines doing the same thing you did before?

Ans.) As mentioned earlier, this may be necessary for parent process (Nfc.js) 'NOT to broadcast' to all Contents.
Instead in the current patch Nfc.js only sends the message to registered targets (content process). Note that targets are registered through
a newly introduced DOM API 'setSessionToken(..)'. SetSessionToken passes the sessionToken that the content / application received through 'webActivity'.
Hope this clarifies!

8. Please use consistent naming between
SessionId,SessionToken,TokenSession
Ans.) Fixed naming conventions. Also note that in Nfc.js, there is still a need to maintain 'sessionId' and 'sessionTokenMap'.
SessionID is the id that 'nfcd' sends. Nfc.js creates a new unique GUID for every 'tech-discovered' notification and maps it with sessionID (int) in the map.
Also note that the sessionToken corresponding to the sessionId is cleared upon receiving 'tech-lost' notification.

9. payload is DOMString?
Ans.) Garner already answered this in a separate comment#314. WebIDL doesn't support this yet in JS mode but does in C++.

10. Do we need to pass 'length' to callback also?
Ans.) This is not needed and hence was not passed.

11. boolean setSessionToken(in DOMString sessionToken); Why needed?
Ans.) This new API was the outcome of the discussion we had with fabrice and Olli.
Since WebActivities do not yet support sending MozObjects (only dictionary type allowed), in the current design, nfc_manager (system App) sends the 
received sessionToken (alias to 'sessionId' received from nfcd) from Parent process to the selected NFC gaia application.
Application would then do the following
 i) getNfcTag(sessionToken)    // New DOM API. This internally calls setSessionToken(..) from nsNfc.js . Refer to ::gecko/dom/nfc/
    This binds the top content / target with the underlying session and the DOM returns a new object 'MozNfcTag' back to the app.
 ii) Upon getting the DOM object, Nfc application would then use read/ write and other DOM APIs.

Garner / Sid
Comment on attachment 820086 [details] [diff] [review]
Patch 6 (v11) - NFC Makefiles and Manifests

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

::: configure.in
@@ +209,4 @@
>      15)
>          GONK_INCLUDES="-I$gonkdir/frameworks/base/opengl/include -I$gonkdir/frameworks/base/native/include -I$gonkdir/frameworks/base/include -I$gonkdir/frameworks/base/services/camera -I$gonkdir/frameworks/base/include/media/stagefright -I$gonkdir/frameworks/base/include/media/stagefright/openmax -I$gonkdir/frameworks/base/media/libstagefright/rtsp -I$gonkdir/frameworks/base/media/libstagefright/include -I$gonkdir/external/dbus -I$gonkdir/external/bluetooth/bluez/lib -I$gonkdir/dalvik/libnativehelper/include/nativehelper"
>          MOZ_B2G_BT=1
> +        MOZ_NFC=1

Should we call it MOZ_B2G_NFC?
Comment on attachment 820772 [details] [diff] [review]
Patch 4 (v13) - NFC Chrome Worker Implementation

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

::: dom/system/gonk/Nfc.js
@@ +65,5 @@
> +                                   "nsISettingsService");
> +XPCOMUtils.defineLazyServiceGetter(this, "UUIDGenerator",
> +                                    "@mozilla.org/uuid-generator;1",
> +                                    "nsIUUIDGenerator");
> +XPCOMUtils.defineLazyGetter(this, "gMessageManager", function () {

I will check if it's really neccesary for you to use gMessageManager here.

@@ +271,5 @@
> +    }
> +  };
> +});
> +
> +

extra line here.

@@ +288,5 @@
> +
> +  gMessageManager.init(this);
> +  let lock = gSettingsService.createLock();
> +  lock.get("nfc.powerlevel", this);
> +  this.powerLevel = NFC.NFC_POWER_LEVEL_DISABLED;

Haven't you already done this in Nfc.prototype? line 382

@@ +291,5 @@
> +  lock.get("nfc.powerlevel", this);
> +  this.powerLevel = NFC.NFC_POWER_LEVEL_DISABLED;
> +  lock.get(NFC.SETTING_NFC_ENABLED, this);
> +  // Maps sessionId (that are generated from nfcd) with a unique guid : 'SessionToken'
> +  this.sessionTokenMap = {};

I'll leave sesseionToken until Part 1 is reviewed.
I don't know why you want to expose session now.
Also please use consistent naming, sessionId or sessiontoken.
Please check with Part 1.

@@ +326,5 @@
> +   *        A text message type.
> +   * @param message [optional]
> +   *        An optional message object to send.
> +   */
> +  send: function send(nfcMessageType, message) {

Use a better naming,
sendToWorker or some name is more easier to understand.

It might mean "send message to Content Process", or "send ndef to the other device", ...etc.

@@ +340,5 @@
> +    let message = event.data;
> +    debug("Received message: " + JSON.stringify(message));
> +    if (!this._enabled) {
> +      throw new Error("NFC is not enabled.");
> +    }

Is the timing okay here?

For example, user turns off NFC from settings,
daemon maybe send techLost,
but it seems techLost might be dropped.

@@ +345,5 @@
> +
> +    switch (message.type) {
> +      case "techDiscovered":
> +        this._connectedSessionId = message.sessionId;
> +        this.sessionTokenMap[this._connectedSessionId] = UUIDGenerator.generateUUID().toString();

What about P2P?
When receiving NDEF in P2P mode, the sessionId will be the same,
so is it we generate a new UUID ?
Or you haven't handled P2P in this patch?

@@ +389,5 @@
> +  receiveMessage: function receiveMessage(message) {
> +    debug("Received '" + JSON.stringify(message) + "' message from content process");
> +
> +    if (!this._enabled) {
> +      throw new Error("NFC is not enabled.");

throwing might not be enough.
The Gaia will never get the onerror in DOMRequest called, and it will never know what happened.

@@ +466,5 @@
> +        this._enabled = aResult;
> +        // General power setting
> +        if (this._enabled) {
> +          this.setNFCPowerConfig(NFC.NFC_POWER_LEVEL_ENABLED);
> +          this._enabled = true;

I don't understand why you set _enabled again?

@@ +469,5 @@
> +          this.setNFCPowerConfig(NFC.NFC_POWER_LEVEL_ENABLED);
> +          this._enabled = true;
> +        } else {
> +          this.setNFCPowerConfig(NFC.NFC_POWER_LEVEL_DISABLED);
> +          this._enabled = false;

here too.

@@ +518,5 @@
> +      type: "ConfigRequest",
> +      powerLevel: prop.powerLevel
> +    };
> +    this.send("configRequest", outMessage);
> +  }

Why cannot we move these into receiveMessage?
the sessionId check?

::: dom/system/gonk/NfcContentHelper.js
@@ +112,5 @@
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +    let request = Services.DOMRequest.createRequest(window);
> +    let requestId = btoa(this.getRequestId(request));
> +    this._requestMap[requestId] = {win: window};

Why cannot we just
this._requestMap[requestId] = window; 
?
And same for below.

@@ +368,5 @@
> +      return r;
> +    });
> +    let resultA = {records: records};
> +
> +    if (!resultA.records) {

Why don't you check status?
It seems to me SUCCESS is always be called.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +19,4 @@
>  
>  #include "nsINetworkManager.h"
>  #include "nsIWifi.h"
> +#ifdef MOZ_NFC

I'll wait for Part 6 reviewed by gps to see whether we should call it MOZ_B2G_NFC.

Also if you like to keep the code you have right now in SystemWorkerManager, can you file a bug to move it to Nfc.cpp?

::: dom/system/gonk/nfc_consts.js
@@ +15,5 @@
> +
> +/* Copyright © 2013, Deutsche Telekom, Inc. */
> +
> +// Set to true to debug all NFC layers
> +this.DEBUG_ALL = true;

please default to false.

@@ +54,5 @@
> +
> +// NFC powerlevels must match config PDUs.
> +this.NFC_POWER_LEVEL_DISABLED       = 0;
> +this.NFC_POWER_LEVEL_LOW            = 1;
> +this.NFC_POWER_LEVEL_ENABLED        = 2;

power level disabled/enabled seems not a correct name to me.
To me, power level sounds like a range between 0~100,
but enable/disable power level is a little confused to me.
say, if we diable power level, so the power level will run in a fixed mode and cannot be adjustable?

::: dom/system/gonk/nfc_worker.js
@@ +119,5 @@
> +        let type       = Buf.readUint8Array(typeLength);
> +        let padding    = getPaddingLen(typeLength);
> +        for (let i = 0; i < padding; i++) {
> +          Buf.readUint8();
> +        }

Try Buf.seekIncoming(padding * PARCEL_SIZE_SIZE);
And below.

@@ +246,5 @@
> +    let cb = function callback() {
> +      let error                  = Buf.readInt32();
> +      let sessionId              = Buf.readInt32();
> +      let maxSupportedLength     = Buf.readInt32();
> +      let mode                   = Buf.readInt32();

mode?
What about isReadOnly, and canBeMakeReadOnly?

@@ +331,5 @@
> +    if (typeof method == "function") {
> +      if (DEBUG) debug("Handling parcel as " + method.name);
> +      method.call(this, length);
> +    } else if (typeof callback == "function") {
> +      callback.call(this, request_type);

Also I asked before, should we also pass length to callback?

@@ +353,5 @@
> +NfcWorker[NFC_NOTIFICATION_INITIALIZED] = function NFC_NOTIFICATION_INITIALIZED (length) {
> +  let status       = Buf.readInt32();
> +  let majorVersion = Buf.readInt32();
> +  let minorVersion = Buf.readInt32();
> +  debug("NFC_NOTIFICATION_INITIALIZED status:"+status+" major:"+majorVersion+" minor:"+minorVersion);

nit, space between operators.

@@ +360,5 @@
> +NfcWorker[NFC_NOTIFICATION_TECH_DISCOVERED] = function NFC_NOTIFICATION_TECH_DISCOVERED(length) {
> +  debug("NFC_NOTIFICATION_TECH_DISCOVERED");
> +  let techs     = [];
> +  let sessionId = Buf.readInt32();
> +  let num       = Buf.readInt32();

what about ndefMsg?

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +15,5 @@
> +  void unregisterNfcCallback(in nsINfcCallback callback);
> +
> +  boolean setSessionToken(in DOMString sessionToken);
> +
> +  nsIDOMDOMRequest getDetailsNDEF(in nsIDOMWindow window, in DOMString sessionToken);

I'll wait until Part 1 is reviewed for the sessionToken.
Attachment #820772 - Flags: review?(allstars.chh) → review-
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #318)
> Comment on attachment 820086 [details] [diff] [review]
> Patch 6 (v11) - NFC Makefiles and Manifests
> > +        MOZ_NFC=1
> 
> Should we call it MOZ_B2G_NFC?

The previous review indicated other devices like laptops/desktops can have NFC capabilities too. There is a NFC reader connected to one of our dev laptops via USB for secure element development, so it was a good point.
Comment on attachment 820086 [details] [diff] [review]
Patch 6 (v11) - NFC Makefiles and Manifests

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

Just a few nits keeping you from r+.

::: configure.in
@@ +7392,5 @@
>  dnl ========================================================
> +dnl = Enable NFC Interface for B2G (Gonk usually)
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(b2g-nfc,
> +[  --enable-b2g-nfc      Set compile flags necessary for testing B2G NFC via network sockets ],

Should probably be --enable-nfc to match the define flag.

::: dom/nfc/Makefile.in
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +#
> +# Copyright © 2013 Deutsche Telekom, Inc.
> +
> +include $(topsrcdir)/dom/dom-config.mk

Is this actually needed? Please check. Consider replacing with LOCAL_INCLUDES in your moz.build file. See also bug 922566, which will kill dom-config.mk!

::: layout/build/moz.build
@@ +24,5 @@
> +    ]
> +
> +if CONFIG['MOZ_NFC']:
> +    LOCAL_INCLUDES += [
> +        TOPSRCDIR + '/dom/nfc'

I thought we made it so paths beginning with / were topsrcdir relative.
Attachment #820086 - Flags: review?(gps) → feedback+
Attached patch Patch 7 (v5) NFC Gonk Misc r=mwu (obsolete) — Splinter Review
Rename "--enable-b2g-nfc" to "--enable-b2g" per review Comment 321 from gps.
Attachment #802341 - Attachment is obsolete: true
Attachment #821391 - Flags: review?(mwu)
> Rename "--enable-b2g-nfc" to "--enable-nfc" per review Comment 321 from gps.
Fix review nits: Remove soon to be obsolete dom/dom-config.mk include in Makefile.in, remove extra TOPSRCDIR var in layout/build/moz.build.
Attachment #820086 - Attachment is obsolete: true
Attachment #821408 - Flags: review?(gps)
Attachment #820772 - Attachment is obsolete: true
Attachment #821432 - Flags: review?(allstars.chh)
<< Comments on Patch 4 (v14) >>

Hi Yoshi,

We went through your latest comments and addressed most of them. Following are the points we would like to answer / discuss

1. >>> [I will check if it's really neccesary for you to use gMessageManager here.]
Ans.) Yes please.

2. >>> [I'll leave sesseionToken until Part 1 is reviewed.
I don't know why you want to expose session now.
Also please use consistent naming, sessionId or sessiontoken.
Please check with Part 1.]
Ans.) Please refer to comment 317 , item 8. We still need both sessionId and sessionTokenMap.
      Reason why we are exposing sessions: refer to comment 317 , item 11

3. >>> [For example, user turns off NFC from settings,
daemon maybe send techLost,but it seems techLost might be dropped.]
Ans.) Its been now addressed by relying on config to properly disable Nfc Messages.

4. >>> [What about P2P?
When receiving NDEF in P2P mode, the sessionId will be the same,
so is it we generate a new UUID ?
Or you haven't handled P2P in this patch?]
Ans.) This patch should handle P2P scenarios also (At least incoming P2P:NDEF message). Even in P2P mode, 'nfcd' would issue a new 'tech-discovered' notification with a new sessionID. So therefore a new UUID (alias) should be
generated.

5. >>> [throwing might not be enough.
The Gaia will never get the onerror in DOMRequest called, and it will never know what happened.]
Ans.)  We have planned to add EventHandlers in webIDL. Refer to http://www.w3.org/2012/nfc/web-api/#event-handlers. This will come in second patch to WebIDL and Chrome (Nfc.js, ContentHelper) changes.

6. >>> [Why cannot we move these into receiveMessage? the sessionId check?]
Ans.) ConfigRequest: ConfigRequest doesn't have a sessionID. It doesn't need to have one.SessionID is only needed for applications if they intend to use MozNfcTag/MozNfcPeer DOM objects (and its APIs). Currently, settings like nfc enable/disable are handled via the nsIObserver interface.

7. >>> [power level disabled/enabled seems not a correct name to me.
Ans.) To me, power level sounds like a range between 0~100,
but enable/disable power level is a little confused to me.
say, if we disable power level, so the power level will run in a fixed mode and cannot be adjustable?]
Lets discuss this further during Taipei. Config PDUs we can split them up into relaying config values nfc enable/disable (boolean), screenon/screenoff (boolean), screenlocked/unlocked (boolean)
so we don't introduce range values to what essentially are binary device states sets.

8. >>> [Try Buf.seekIncoming(padding * PARCEL_SIZE_SIZE);]
Ans.) Hmmm...looks like this doesn't work.

9. >>> [what about ndefMsg? for NFC_NOTIFICATION_TECH_DISCOVERED]
Ans.) Well, Can we land the patch in the current state? If we change our implementation to support the latest Gonk protocol, then we may not have a good way to  test our ongoing efforts. Also 'nfcd' doesn't quite support this notification as per latest protocol revision. Please correct us if latest 'nfcd' supports it already.

Garner / Sid
Comment on attachment 821408 [details] [diff] [review]
Patch 6 (v12) - NFC Makefiles and Manifests

Review of attachment 821408 [details] [diff] [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.

Delete this file. Empty Makefile.in are not necessary.
Attachment #821408 - Flags: review?(gps) → review+
gps: Thanks. Last one! Remove empty Makefile.in (now moz.build, manifests).
Attachment #821408 - Attachment is obsolete: true
Attachment #821876 - Flags: review?(gps)
Attachment #821876 - Flags: review?(gps) → review+
Attachment #821876 - Attachment description: Patch 6 (v13) - NFC Makefiles and Manifests → Patch 6 (v13) - NFC Makefiles and Manifests r=gps
(In reply to Garner Lee from comment #320)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #318)
> > Comment on attachment 820086 [details] [diff] [review]
> > Patch 6 (v11) - NFC Makefiles and Manifests
> > > +        MOZ_NFC=1
> > 
> > Should we call it MOZ_B2G_NFC?
> 
> The previous review indicated other devices like laptops/desktops can have
> NFC capabilities too. There is a NFC reader connected to one of our dev
> laptops via USB for secure element development, so it was a good point.

In that case you need to move your implementations to dom/nfc, not dom/system/gonk,
dom/system/gonk is for B2G only.
Also see Bug 920551.
(In reply to Siddartha P from comment #326)
 
> 4. >>> [What about P2P?
> When receiving NDEF in P2P mode, the sessionId will be the same,
> so is it we generate a new UUID ?
> Or you haven't handled P2P in this patch?]
> Ans.) This patch should handle P2P scenarios also (At least incoming
> P2P:NDEF message). Even in P2P mode, 'nfcd' would issue a new
> 'tech-discovered' notification with a new sessionID. So therefore a new UUID
> (alias) should be
> generated.
Wrong, the techDiscovered when receiving NDEF in P2P mode should have the same session ID.

> 8. >>> [Try Buf.seekIncoming(padding * PARCEL_SIZE_SIZE);]
> Ans.) Hmmm...looks like this doesn't work.
> 
What Problem?

> 9. >>> [what about ndefMsg? for NFC_NOTIFICATION_TECH_DISCOVERED]
> Ans.) Well, Can we land the patch in the current state? If we change our
> implementation to support the latest Gonk protocol, then we may not have a
> good way to  test our ongoing efforts. Also 'nfcd' doesn't quite support
> this notification as per latest protocol revision. Please correct us if
> latest 'nfcd' supports it already.
> 
nfcd alreay supports that.
Comment on attachment 821432 [details] [diff] [review]
Patch 4 (v14) - NFC Chrome Worker Implementation

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

This is the r- for previous gMessageManager in Nfc.js.
You copy it from RIL,
but most things you don't need it.

::: dom/system/gonk/Nfc.js
@@ +110,5 @@
> +      }
> +      ppmm = null;
> +    },
> +
> +    _registerMessageTarget: function _registerMessageTarget(topic, target) {

topic? Or session?

@@ +129,5 @@
> +      targets.push(target);
> +      debug("Registered :" + topic + " target: " + target);
> +    },
> +
> +    _unregisterMessageTarget: function _unregisterMessageTarget(topic, target) {

Ditto.

@@ +166,5 @@
> +        debug("Unregistered " + topic + " target: " + target);
> +      }
> +    },
> +
> +    _enqueueTargetMessage: function _enqueueTargetMessage(topic, message, options) {

Why this is needed?

@@ +183,5 @@
> +
> +      messageQueue.push(msg);
> +    },
> +
> +    _sendTargetMessage: function _sendTargetMessage(topic, message, options) {

topic? But you're calling it with session.

@@ +185,5 @@
> +    },
> +
> +    _sendTargetMessage: function _sendTargetMessage(topic, message, options) {
> +      if (!this.ready) {
> +        this._enqueueTargetMessage(topic, message, options);

You will queue message during boot up?
Why?

@@ +199,5 @@
> +        target.sendAsyncMessage(message, options);
> +      }
> +    },
> +
> +    _resendQueuedTargetMessage: function _resendQueuedTargetMessage() {

No need.

@@ +256,5 @@
> +    observe: function observe(subject, topic, data) {
> +      switch (topic) {
> +        case "system-message-listener-ready":
> +          Services.obs.removeObserver(this, "system-message-listener-ready");
> +          this._resendQueuedTargetMessage();

Why do you have this?
We need to pass any possible NFC-message to Gaia during boot time?
Attachment #821432 - Flags: review?(allstars.chh) → review-
Comment on attachment 821432 [details] [diff] [review]
Patch 4 (v14) - NFC Chrome Worker Implementation

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

This is the r- for the v14.

Also please ask :gps if you'd like to put it in dom/system/gonk.
Because it will be B2G-only.
Then I would say, move the MOZ_NFC back to MOZ_B2G_NFC.

::: dom/system/gonk/Nfc.js
@@ +307,5 @@
> +                                         Ci.nsINfc,
> +                                         Ci.nsIObserver,
> +                                         Ci.nsISettingsServiceCallback]),
> +
> +  _connectedSessionId: null,

connected? 
But actually the session is valid since techDiscovered, not after connected.
Please rename it.

@@ +340,5 @@
> +
> +    switch (message.type) {
> +      case "nfcInitialized":
> +        // Silently consume it for now
> +        break;

Why do we need this in Nfc.js? If you don't want to process it right now.
Can we handle the initializedNotification only in worker ?

@@ +345,5 @@
> +      case "techDiscovered":
> +        this._connectedSessionId = message.sessionId;
> +        this.sessionTokenMap[this._connectedSessionId] = UUIDGenerator.generateUUID().toString();
> +        // Update the upper layers with a session token (alias)
> +        message.sessionId = this.sessionTokenMap[this._connectedSessionId];

I think what you want is broadcast message.sessionToken, not sessionId?

@@ +351,5 @@
> +        break;
> +      case "techLost":
> +        gMessageManager._unregisterMessageTarget(this.sessionTokenMap[this._connectedSessionId], null);
> +        // Update the upper layers with a session token (alias)
> +        message.sessionId = this.sessionTokenMap[this._connectedSessionId];

ditto.

@@ +365,5 @@
> +      case "DetailsNDEFResponse":
> +      case "ReadNDEFResponse":
> +      case "MakeReadOnlyNDEFResponse":
> +      case "WriteNDEFResponse":
> +        message.sessionId = this.sessionTokenMap[this._connectedSessionId];

Ditto

@@ +387,5 @@
> +  receiveMessage: function receiveMessage(message) {
> +    debug("Received '" + JSON.stringify(message) + "' message from content process");
> +
> +    if (!this._enabled) {
> +      throw new Error("NFC is not enabled.");

You said you will handle this ? 
When?

@@ +420,5 @@
> +       default:
> +         if (message.json.sessionToken !== this.sessionTokenMap[this._connectedSessionId]) {
> +           debug("Invalid Session Token: " + message.json.sessionToken + " Expected Session Token: " +
> +                 this.sessionTokenMap[this._connectedSessionId]);
> +           return;

Ditto, silent error here.
Please reported the error to Content.

@@ +466,5 @@
> +        if (this._enabled) {
> +          this.setNFCPowerConfig(NFC.NFC_POWER_LEVEL_ENABLED);
> +        } else {
> +          this.setNFCPowerConfig(NFC.NFC_POWER_LEVEL_DISABLED);
> +        }

this .setNFCPowerConfig(this._enabled ? NFC.NFC_POWER_LEVEL_ENABLED : NFC.NFC_POWER_LEVEL_DISABLED);

@@ +504,5 @@
> +    // Just one param for now.
> +    this.setConfig({powerLevel: powerLevel});
> +  },
> +
> +  setConfig: function setConfig(prop) {

Since the parameters of the Config are related to gonk protocol, which is related to worker.
Can we merge these two functions into one config,
and let config did the protocol thing there.
(including -1 for default)

@@ +513,5 @@
> +    let outMessage = {
> +      type: "ConfigRequest",
> +      powerLevel: prop.powerLevel
> +    };
> +    this.sendToWorker("configRequest", outMessage);

Other messages don't have 'Request'.
Can we just call it config?

::: dom/system/gonk/NfcContentHelper.js
@@ +75,5 @@
> +  }),
> +
> +  _requestMap: null,
> +
> +  // FIXME: btoa's will be unneeded when binary nfcd/gonk protocol is merged.

You already merged this, right?
Why do we need this?

@@ +92,5 @@
> +  },
> +
> +  // NFC interface:
> +  setSessionToken: function setSessionToken(sessionToken) {
> +    if (sessionToken== null) {

nit, space before ==

@@ +234,5 @@
> +      debug("Unregistered telephony callback: " + callback);
> +    }
> +  },
> +
> +  registerNfcCallback: function registerNfcCallback(callback) {

Why another function? 
I think there's only one callback here.

@@ +450,5 @@
> +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester));
> +    }
> +  },
> +
> +  _deliverCallback: function _deliverCallback(callbackType, name, args) {

Who called this?

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +655,5 @@
> +    NS_WARNING("Failed to Connect to Nfc");
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +

nit, rm extra line

::: dom/system/gonk/nfc_worker.js
@@ +147,5 @@
> +      }
> +
> +      message.type      = "ReadNDEFResponse";
> +      message.sessionId = sessionId;
> +      message.requestId = message.requestId;

What's this? 
And below.

@@ +248,5 @@
> +    let cb = function callback() {
> +      let error                  = Buf.readInt32();
> +      let sessionId              = Buf.readInt32();
> +      let isReadOnly             = Buf.readInt32();
> +      let canBeMadeReadOnly      = Buf.readInt32();

These two booleans should be only 2 bytes. You are reading...... 8 bytes.

@@ +249,5 @@
> +      let error                  = Buf.readInt32();
> +      let sessionId              = Buf.readInt32();
> +      let isReadOnly             = Buf.readInt32();
> +      let canBeMadeReadOnly      = Buf.readInt32();
> +      let maxSupportedLength     = Buf.readInt32();

Padding before maxNdefLen
Comment on attachment 779560 [details]
(v3) This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.

Gonk protocol will be defined in Bug 897312, so obsolete this.
Attachment #779560 - Attachment is obsolete: true
Comment on attachment 779560 [details]
(v3) This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.

Obsoleting. See Bug 897312 for the latest version.
Comment on attachment 820050 [details] [diff] [review]
Patch 1 (v13) NFC DOM WebIDL

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

Looks pretty good, but I want to see it again.

::: dom/webidl/MozNFCPeer.webidl
@@ +7,5 @@
> + *
> + * Copyright © 2013 Deutsche Telekom, Inc.
> + */
> +
> +[Constructor(DOMString sessionId),

We really want a chrome only constructor here, but I don't think support for that exists yet.  I'll file a bug.

@@ +9,5 @@
> + */
> +
> +[Constructor(DOMString sessionId),
> + JSImplementation="@mozilla.org/nfc/NFCPeer;1"]
> +interface MozNFCPeer : EventTarget {

I don't see any reason for this to inherit from EventTarget.  Drop that?

::: dom/webidl/MozNFCTag.webidl
@@ +24,5 @@
> +};
> +
> +[Constructor(DOMString sessionId),
> + JSImplementation="@mozilla.org/nfc/NFCTag;1"]
> +interface MozNFCTag : EventTarget {

Here too?
Attachment #820050 - Flags: review?(bugs) → review-
Comment on attachment 820051 [details] [diff] [review]
Patch 3 (v12) - NFC DOM Implementation

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

So, I think we should change this around a little to avoid the chrome only constructors I mentioned in the last review.

If we shove MozNFCPeer.js and MozNFCTag.js into nsNfc.js we can access MozNFCTag/MozNFCPeer directly from mozNfc.  Then we can use the _create method (https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Using_a__create_method_.C2.A0) to create the wrappers for the object instead of using the WebIDL Constructor, which unfortunately we currently have no way to hide from content.

::: dom/nfc/MozNdefRecord.cpp
@@ +47,5 @@
> +  , mPayload(aPayload)
> +{
> +  mWindow = aWindow;
> +  SetIsDOMBinding();
> +  MOZ_COUNT_CTOR(MozNdefRecord);

There's no need for MOZ_COUNT_CTOR/DTOR for reference counted objects.

::: dom/nfc/MozNdefRecord.h
@@ +14,5 @@
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "jsapi.h"
> +
> +#include "nsIDocument.h"

What do you need to include this for?

@@ +16,5 @@
> +#include "jsapi.h"
> +
> +#include "nsIDocument.h"
> +
> +#include "mozilla/dom/MozNdefRecordBinding.h"

You should only need to include this header in the .cpp file.
Attachment #820051 - Flags: review?(bugs) → review-
Hide public MozNFCTag and MozNFCPeer constructor.
Attachment #820050 - Attachment is obsolete: true
Attachment #823735 - Flags: review?(khuey)
nsIDocument is needed for ns(P)IDOMWindow. Constructor/Destructors macros removed. Extra "mozilla/dom/MozNdefRecordBinding.h" include removed from header file.
Attachment #820051 - Attachment is obsolete: true
Attachment #823736 - Flags: review?(khuey)
Update based on review comments.
Attachment #821432 - Attachment is obsolete: true
Attachment #823737 - Flags: review?(allstars.chh)
Minor update. Remove MozNFCTag and MozNFCPeer from public view (now inside nsNfc.js until ChromeOnly constructors are added.)
Attachment #821876 - Attachment is obsolete: true
Attachment #823738 - Flags: review?(gps)
Comment on attachment 823738 [details] [diff] [review]
Patch 6 (v14) - NFC Makefiles and Manifests r=gps, r=khuey

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

r=me on the additional changes here.
Attachment #823738 - Flags: review?(gps) → review+
Comment on attachment 823735 [details] [diff] [review]
Patch 1 (v14) NFC DOM WebIDL r=khuey

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

r=me
Attachment #823735 - Flags: review?(khuey) → review+
Attachment #823735 - Attachment description: Patch 1 (v14) NFC DOM WebIDL → Patch 1 (v14) NFC DOM WebIDL r=khuey
Comment on attachment 823736 [details] [diff] [review]
Patch 3 (v13) - NFC DOM Implementation r=khuey

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

r=me
Attachment #823736 - Flags: review?(khuey) → review+
Attachment #823738 - Attachment description: Patch 6 (v14) - NFC Makefiles and Manifests r=gps → Patch 6 (v14) - NFC Makefiles and Manifests r=gps, r=khuey
Attachment #823736 - Attachment description: Patch 3 (v13) - NFC DOM Implementation → Patch 3 (v13) - NFC DOM Implementation r=khuey
Comment on attachment 823737 [details] [diff] [review]
Patch 4 (v15) - NFC Chrome Worker Implementation

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

A few comments.

::: dom/system/gonk/NfcContentHelper.js
@@ +90,5 @@
> +    return encodedRecords;
> +  },
> +
> +  // NFC interface:
> +  setSessionToken: function setSessionToken(sessionToken) {

This function either suceeds or throws an exception, so it doesn't need a return value.  We can give this a void return value and get rid of the return statements.

@@ +231,5 @@
> +          ", result: " + JSON.stringify(result));
> +    Services.DOMRequest.fireSuccess(request, result);
> +  },
> +
> +  dispatchFireRequestSuccess: function dispatchFireRequestSuccess(requestId, result) {

It's really nit picky, but I would name this asyncFireRequestSuccess (or fireRequestSuccessAsync).

@@ +251,5 @@
> +          ", result: " + JSON.stringify(error));
> +    Services.DOMRequest.fireError(request, error);
> +  },
> +
> +  dispatchFireRequestError: function dispatchFireRequestError(requestId, error) {

Same here.

@@ +306,5 @@
> +    });
> +
> +    if (result.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, result.status);
> +    } else  {

nit: there's an extra space after else.

@@ +325,5 @@
> +    let requestId = atob(message.requestId);
> +
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, result.status);
> +    } else  {

here too.

::: dom/system/gonk/nsINfc.idl
@@ +6,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(79c96fc0-5e73-11e1-b86c-0800200c9a66)]
> +interface nsINfc : nsISupports

Is this interface actually used?

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +13,5 @@
> +{
> +  void registerNfcCallback(in nsINfcCallback callback);
> +  void unregisterNfcCallback(in nsINfcCallback callback);
> +
> +  boolean setSessionToken(in DOMString sessionToken);

This can return void as I mentioned earlier.

::: dom/system/gonk/nsISystemWorkerManager.idl
@@ +14,5 @@
>    void registerRilWorker(in unsigned long aClientId,
>                           in jsval aWorker);
> +
> +  [implicit_jscontext]
> +  void registerNfcWorker(in jsval aWorker);

The IID of this interface needs to change.
Comment on attachment 823737 [details] [diff] [review]
Patch 4 (v15) - NFC Chrome Worker Implementation

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

Please also file a follow-up bug if we need to move the impl from dom/system/gonk to dom/nfc.

::: dom/system/gonk/Nfc.js
@@ +109,5 @@
> +
> +    _registerMessageTarget: function _registerMessageTarget(sessionToken, target) {
> +      let targets = this.targetsBySessionTokens[sessionToken];
> +      if (!targets) {
> +        targets = this.targetsBySessionTokens[sessionToken] = [];

Why do you need an Array for this?

@@ +135,5 @@
> +        return;
> +      }
> +
> +      // Unregister the target for a specified sessionToken.
> +      let targets = this.targetsBySessionTokens[sessionToken];

Same as above,
why can't we just have a single 'target', instead of targets here?

@@ +243,5 @@
> +  Services.obs.addObserver(this, NFC.TOPIC_XPCOM_SHUTDOWN, false);
> +
> +  gMessageManager.init(this);
> +  let lock = gSettingsService.createLock();
> +  lock.get("nfc.powerlevel", this);

Here you call it powerlevel, with all lower cases.
Why you call it powerLevel with L is upper case below?

Can we const this?

@@ +300,5 @@
> +    }
> +
> +    let nfcMsgType = message.name + "Response";
> +    message.target.sendAsyncMessage(nfcMsgType, {
> +      type: nfcMsgType,

Why you embed message type again?

@@ +319,5 @@
> +      case "techDiscovered":
> +        this._currentSessionId = message.sessionId;
> +        // Check if the session token already exists. If exists, continue to use the same one.
> +        // If not, generate a new token.
> +        if (this.sessionTokenMap[this._currentSessionId] === undefined) {

if (!this.sessionTokenMap[this._currentSessionId]) {

@@ +325,5 @@
> +        }
> +        // Update the upper layers with a session token (alias)
> +        message.sessionToken = this.sessionTokenMap[this._currentSessionId];
> +        // Do not expose the actual session to the content
> +        message.sessionId = "";

delete message.sessionId;

@@ +333,5 @@
> +        gMessageManager._unregisterMessageTarget(this.sessionTokenMap[this._currentSessionId], null);
> +        // Update the upper layers with a session token (alias)
> +        message.sessionToken = this.sessionTokenMap[this._currentSessionId];
> +        // Do not expose the actual session to the content
> +        message.sessionId = "";

ditto

@@ +349,5 @@
> +      case "MakeReadOnlyNDEFResponse":
> +      case "WriteNDEFResponse":
> +        message.sessionToken = this.sessionTokenMap[this._currentSessionId];
> +        // Do not expose the actual session to the content
> +        message.sessionId = "";

ditto

@@ +371,5 @@
> +  receiveMessage: function receiveMessage(message) {
> +    debug("Received '" + JSON.stringify(message) + "' message from content process");
> +
> +    if (!this._enabled) {
> +      throw new Error("NFC is not enabled.");

sendNfcErrorResponse?

@@ +410,5 @@
> +           this.sendNfcErrorResponse(message);
> +           return null;
> +         }
> +
> +         debug("Received Message: " + message.name + "Session Token: " +

remove 
you already print this in line 372.

@@ +411,5 @@
> +           return null;
> +         }
> +
> +         debug("Received Message: " + message.name + "Session Token: " +
> +               this.sessionTokenMap[this._currentSessionId]);

nit, add
break;
in the end.

@@ +453,5 @@
> +        debug("'nfc.enabled' is now " + aResult);
> +        this._enabled = aResult;
> +        // General power setting
> +        this.setNFCPowerConfig(this._enabled ? NFC.NFC_POWER_LEVEL_ENABLED :
> +                                               NFC.NFC_POWER_LEVEL_DISABLED);

try setConfig.

@@ +491,5 @@
> +    // Just one param for now.
> +    this.setConfig({powerLevel: powerLevel});
> +  },
> +
> +  setConfig: function setConfig(prop) {

Try to move the gonk-protocol specific things to worker.

@@ +497,5 @@
> +    let configset = {
> +      powerLevel: prop.powerLevel
> +    };
> +    let outMessage = {
> +      type: "ConfigRequest",

config,
Use consistent naming with other requests.

::: dom/system/gonk/NfcContentHelper.js
@@ +75,5 @@
> +  }),
> +
> +  _requestMap: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {

What is this function for?
seems doing nothing here.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +662,5 @@
> +  // worker lives in Nfc.js. All we do here is hold it alive and
> +  // hook it up to the NFC thread.
> +#endif // MOZ_NFC
> +  return NS_OK;
> +}

Please file a follow-up bug to move these code into ipc/nfc/

::: dom/system/gonk/nfc_worker.js
@@ +40,5 @@
> +   * Process one parcel.
> +   */
> +  processParcel: function processParcel() {
> +    let pduType = this.readInt32();
> +    if (DEBUG) debug("No of bytes available in Parcel : " + this.readAvailable);

s/No/Number/

@@ +60,5 @@
> +     * TODO: This needs to be fixed. A map of NFC_RESPONSE_XXX and RequestID
> +     *       needs to be maintained ?? For Generic Responses (1000) ,
> +     *       we may still rely on callback approach.
> +     */
> +    this.mCallback = callback;

Remove the debug and file a follow-up bug for maintaining callback.

@@ +75,5 @@
> +  onSendParcel: function onSendParcel(parcel) {
> +    postNfcMessage(parcel);
> +  },
> +
> +  //TODO maintain callback

See above

@@ +131,5 @@
> +        }
> +
> +        let payloadLength = Buf.readInt32();
> +        //TODO using Uint8Array will make the payload become an object, not an array.
> +        let payload = [];

Is the TODO still valid?

@@ +147,5 @@
> +      }
> +
> +      message.type      = "ReadNDEFResponse";
> +      message.sessionId = sessionId;
> +      message.requestId = message.requestId;

What's this?
I've asked this before.

@@ +169,5 @@
> +      let sessionId     = Buf.readInt32();
> +
> +      message.type      = "WriteNDEFResponse";
> +      message.sessionId = sessionId;
> +      message.requestId = message.requestId;

ditto

@@ +360,5 @@
> +NfcWorker[NFC_NOTIFICATION_INITIALIZED] = function NFC_NOTIFICATION_INITIALIZED () {
> +  let status       = Buf.readInt32();
> +  let majorVersion = Buf.readInt32();
> +  let minorVersion = Buf.readInt32();
> +  debug("NFC_NOTIFICATION_INITIALIZED status:" + status + " major:" + majorVersion + " minor:" + minorVersion);

Please also define the versions used in nfc_worker,
we will know easily if we get a version mis-match.

@@ +376,5 @@
> +  debug("sessionId = " + sessionId + "  techs = "+techs);
> +  this.sendDOMMessage({type: "techDiscovered",
> +                       sessionId: sessionId,
> +                       content: { tech: techs}
> +                       });

What aboout NdefMessage?

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +11,5 @@
> +[scriptable, uuid(28c8f240-da8c-11e1-9b23-0800200c9a66)]
> +interface nsINfcContentHelper : nsISupports
> +{
> +  void registerNfcCallback(in nsINfcCallback callback);
> +  void unregisterNfcCallback(in nsINfcCallback callback);

Remove these two if you aren't going to implement them in this patch.
Attachment #823737 - Flags: review?(allstars.chh) → review-
Comment on attachment 823737 [details] [diff] [review]
Patch 4 (v15) - NFC Chrome Worker Implementation

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

::: dom/system/gonk/nfc_worker.js
@@ +155,5 @@
> +      this.sendDOMMessage(message);
> +    }
> +
> +    Buf.newParcel(NFC_REQUEST_READ_NDEF, cb);
> +    Buf.writeInt32(message.sessionId);

Where does sessionId come from anyway?
Comment on attachment 821391 [details] [diff] [review]
Patch 7 (v5) NFC Gonk Misc r=mwu

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

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

You should modify configure.in to enable NFC rather than enable it in here.

::: init.b2g.rc
@@ +17,5 @@
> +service nfcd /system/bin/nfcd
> +    class main
> +    socket nfcd stream 660 nfc nfc
> +    user nfc
> +    group nfc

I already r+'d a patch that adds nfcd to init.b2g.rc. If you still need this, rebase on that.
Attachment #821391 - Flags: review?(mwu)
Hi Yoshi, Will update with comments / answers to your earlier questions shortly
Attachment #824492 - Flags: review?(allstars.chh)
Attachment #823737 - Attachment is obsolete: true
Hi Kyle, Garner and I have now addressed your comments in v16 of the patch 4 "NFC Chrome Worker Implementation"

Yoshi, 

I have now addressed most of your latest comments. Following are some of the items , I would like to discuss.

> targets = this.targetsBySessionTokens[sessionToken] = [];
>>> Why do you need an Array for this?

We have atleast two instances of Content process accessing NFC DOM APIs in our current design
1) System App (no UI but still accesses all DOM APIs)
2) Actual NFC application , receipient of ndef-discovered message
Both applications set their tokens . Nfc.js remembers and registers the target by token. So we need to maintain atleast two values
Also its better to have an array for any future usecases so that it may provide some flexibility in terms of design. However former point
was the main reason than latter

> let targets = this.targetsBySessionTokens[sessionToken];
>>> Same as above,
>>> why can't we just have a single 'target', instead of targets here?
The same reason as above. What do you think?

> Here you call it powerlevel, with all lower cases.
>>> Why you call it powerLevel with L is upper case below?
>>> Can we const this?

I think, the rationale was this.
'powerLevel' is a outMessage to nfcWorker. So it is camelcase'd
'nfc.powerlevel' is a setting name. hence all lower.
However , I have added a constant in nfc_consts.js file

>    let nfcMsgType = message.name + "Response";
>    message.target.sendAsyncMessage(nfcMsgType, {
>       type: nfcMsgType,
>>> Why you embed message type again?

Arg1:'nfcMsgType' is the 'messageName'. Since this message is registered on the other side by ContentHelper, its listener is invoked.
Hence first arg is necessary. However 'type: nfcMsgType,' is only a meta-data. Added it only for extra clarity.
'Message Name' could be as generic as 'NFCResponse' for all NFC_XXX_RESPONSES types. Then 'type: nfcMsgType,' can act as a meta data
describing about the  message that is about to be received. In this case, its just that both happen to be same.

> throw new Error("NFC is not enabled.");
>>> sendNfcErrorResponse?

Yes , you are right. I am actullay making another incremental patch (onpeerfound, onpeerLost etc) after hopefully this patch gets landed
We will have to add EventHandlers to DOM APIs to notify result of this operation to upper layers.

>   setConfig: function setConfig(prop) {
>>> Try to move the gonk-protocol specific things to worker.

Yes , Can we make this change as part of upcoming patch. ?

> //TODO using Uint8Array will make the payload become an object, not an array.
>>>  Yes this is still valid. Ideally we would like to acheive.

> message.requestId = message.requestId;
>>> What's this?
>>> I've asked this before.
Ok. 'message'stays on the stack itself. When nfc_worker sends NFC_XXX_Requests, message has 'requestID' , 'sessionID'.
Since the worker registers for the callback, 'message' continues to be on the stack and when the callback is fired, its 'message' context
is passed. In effect it has access to both 'sessionID' , 'requestID' that it can now relay back to upper layers through NFC_XXX_Responses

> Please also define the versions used in nfc_worker,
>>> we will know easily if we get a version mis-match.
Ok. What should be the target major / minor version that worker should compare againts. ?
Can we mark this change for future updates?

> What aboout NdefMessage?
Yes, done now.

> Where does sessionId come from anyway?
Good catch. Fixed it in the latest patch. Nfc.js appends the 'message.sessionId' with the 'currentSessionId' at the time of sending the message to the worker

*Note: I will raise 3 bugs
1) To maintain callback
2) To convert nfc_worker to C++ impl.
3) File a follow-up bug to move some of SystemWorkerManager.cpp code into ipc/nfc/

TBD: (After landing the existing ChromeWorker patch, Would address the below in next patchset)
1) Compare Major / Minor version
2) Add event handlers to notify status of 'nfc.enabled'
3) Try to move the gonk-protocol specific things to worker

What do you think ?

Thanks,
Siddartha
Attached patch Patch 7 (v6) NFC Gonk Misc r=mwu (obsolete) — Splinter Review
Revert to previous patch. nfcd daemon only.
Attachment #821391 - Attachment is obsolete: true
Update makefiles to latest gecko head.
Attachment #823738 - Attachment is obsolete: true
(In reply to Siddartha P from comment #349)
 
> > targets = this.targetsBySessionTokens[sessionToken] = [];
> >>> Why do you need an Array for this?
> 
> We have atleast two instances of Content process accessing NFC DOM APIs in
> our current design
> 1) System App (no UI but still accesses all DOM APIs)
> 2) Actual NFC application , receipient of ndef-discovered message
> Both applications set their tokens . 

Will the two applications share the same token/session at the same time?
The tokens you said here are the same or different?
If the two tokens are the same:
It's reasonable both apps get the message sent from Chrome Process.

If the tokens are different:
Why do you need an Array?

> Also its better to have an array for any future usecases so that it may
> provide some flexibility in terms of design. 
Can we discuss your 'future use cases' later?

> 
> >    let nfcMsgType = message.name + "Response";
> >    message.target.sendAsyncMessage(nfcMsgType, {
> >       type: nfcMsgType,
> >>> Why you embed message type again?
> 
> Arg1:'nfcMsgType' is the 'messageName'. Since this message is registered on
> the other side by ContentHelper, its listener is invoked.
> Hence first arg is necessary. However 'type: nfcMsgType,' is only a
> meta-data. Added it only for extra clarity.
> 'Message Name' could be as generic as 'NFCResponse' for all
> NFC_XXX_RESPONSES types. Then 'type: nfcMsgType,' can act as a meta data
> describing about the  message that is about to be received. In this case,
> its just that both happen to be same.
> 
 
> >   setConfig: function setConfig(prop) {
> >>> Try to move the gonk-protocol specific things to worker.
> 
> Yes , Can we make this change as part of upcoming patch. ?
>
No, it's just moving some lines to worker.
 
> > //TODO using Uint8Array will make the payload become an object, not an array.
> >>>  Yes this is still valid. Ideally we would like to acheive.
> 
Valid? Your WebIDL has been changed to DOMString for payload.

> > message.requestId = message.requestId;
> >>> What's this?
> >>> I've asked this before.
> Ok. 'message'stays on the stack itself. When nfc_worker sends
> NFC_XXX_Requests, message has 'requestID' , 'sessionID'.
> Since the worker registers for the callback, 'message' continues to be on
> the stack and when the callback is fired, its 'message' context
> is passed. In effect it has access to both 'sessionID' , 'requestID' that it
> can now relay back to upper layers through NFC_XXX_Responses
> 
So it should be removed, right?

> > Please also define the versions used in nfc_worker,
> >>> we will know easily if we get a version mis-match.
> Ok. What should be the target major / minor version that worker should
> compare againts. ?

The latest is 1.7
> Can we mark this change for future updates?
>
You can just define in some nfc_consts, 
we can leave error handling later.
 
> > 
> *Note: I will raise 3 bugs
> 1) To maintain callback
> 2) To convert nfc_worker to C++ impl.
> 3) File a follow-up bug to move some of SystemWorkerManager.cpp code into
> ipc/nfc/
>
Good, thanks
 
> TBD: (After landing the existing ChromeWorker patch, Would address the below
> in next patchset)
> 1) Compare Major / Minor version
Do this in this bug, it's just
const NFC_MAJOR_VERSION = 1;
const NFC_MINOR_VERSION = 7;
in nfc_consts.js

> 2) Add event handlers to notify status of 'nfc.enabled'
I don't know what's this.

> 3) Try to move the gonk-protocol specific things to worker
>
The has to go to this bug,
otherwise I should review JSON protocol here.
Comment on attachment 824492 [details] [diff] [review]
Patch 4 (v16) - NFC Chrome Worker Implementation

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

This patch mostly are okay,
but I'd like to see some questions answered, for example, why using Array for registering.

Also please address some minor fixes in this patch, like the gonk-specific and the version.
thanks

::: dom/system/gonk/Nfc.js
@@ +109,5 @@
> +
> +    _registerMessageTarget: function _registerMessageTarget(sessionToken, target) {
> +      let targets = this.targetsBySessionTokens[sessionToken];
> +      if (!targets) {
> +        targets = this.targetsBySessionTokens[sessionToken] = [];

I still need to know the reason for using an array here.

@@ +369,5 @@
> +  receiveMessage: function receiveMessage(message) {
> +    debug("Received '" + JSON.stringify(message) + "' message from content process");
> +
> +    if (!this._enabled) {
> +      throw new Error("NFC is not enabled.");

sendNfcErrorResponse

@@ +481,5 @@
> +
> +  /**
> +   * NFC Config API. Properties is a set of name value pairs.
> +   */
> +  setNFCPowerConfig: function setNFCPowerConfig(powerLevel) {

move gonk-specific code to worker.

::: dom/system/gonk/NfcContentHelper.js
@@ +76,5 @@
> +  }),
> +
> +  _requestMap: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {

Why this?

::: dom/system/gonk/nfc_worker.js
@@ +127,5 @@
> +          Buf.readUint8();
> +        }
> +
> +        let payloadLength = Buf.readInt32();
> +        //TODO using Uint8Array will make the payload become an object, not an array.

check TODO here.

@@ +156,5 @@
> +      let records      = this.unMarshallNdefMessage();
> +
> +      message.type      = "ReadNDEFResponse";
> +      message.sessionId = sessionId;
> +      message.requestId = message.requestId;

remove ?

@@ +395,5 @@
> +  }
> +  this.sendDOMMessage({type: "techDiscovered",
> +                       sessionId: sessionId,
> +                       content: { tech: techs,
> +                                  ndef: ndefMsgs}

You add another level 'contetn' here?
Why?
Can't we just 
{
type:..
tech: techs,
ndef: ndefMsgs,
}
Attachment #824492 - Flags: review?(allstars.chh)
> Will the two applications share the same token/session at the same time?
> The tokens you said here are the same or different?
> If the two tokens are the same:
> It's reasonable both apps get the message sent from Chrome Process.

> If the tokens are different:
> Why do you need an Array?

Yes, these two tokens are same for System App and Nfc Application. Hence we may still need an array.

> >   setConfig: function setConfig(prop) {
> >  Try to move the gonk-protocol specific things to worker.
> 
>  No, it's just moving some lines to worker.

Not sure if I have understood your concern completely here.
I think 'setConfig' and 'setNFCPowerConfig' are ok in Nfc.js
'setConfig' is the interface that calls into worker by calling 'this.sendToWorker'.
And 'setNFCPowerConfig' is simplw wrapper on 'setConfig'.
As more 'configuration values' get added in future, we can 'setXXXConfig' that calls into 'setConfig', which in turn calls into worker.

> Valid? Your WebIDL has been changed to DOMString for payload.
Yes you are right. Removed the comment

> You can just define in some nfc_consts, 
> we can leave error handling later.
Ok, done!

>     if (!this._enabled) {
>       throw new Error("NFC is not enabled.");
> sendNfcErrorResponse

Yes, done

>       let records      = this.unMarshallNdefMessage();
> 
>       message.type      = "ReadNDEFResponse";
>       message.sessionId = sessionId;
>       message.requestId = message.requestId;

> remove ?
No, this is needed. 'requestId' is needed for all responses so that NfcContentHelper 
can fire the response to Gaia on DOM request that it received earlier

> > message.requestId = message.requestId;
> >>> What's this?
> >>> I've asked this before.
> Ok. 'message'stays on the stack itself. When nfc_worker sends
> NFC_XXX_Requests, message has 'requestID' , 'sessionID'.
> Since the worker registers for the callback, 'message' continues to be on
> the stack and when the callback is fired, its 'message' context
> is passed. In effect it has access to both 'sessionID' , 'requestID' that it
> can now relay back to upper layers through NFC_XXX_Responses
> 

>> So it should be removed, right?
>   this.sendDOMMessage({type: "techDiscovered",
>                        sessionId: sessionId,
>                        content: { tech: techs,
>                                   ndef: ndefMsgs}

Same as above explanation. nfc_worker needs to relay 'requestID' and 'sessionID' back to Nfc.js --> NfcContentHelper

> 2) Add event handlers to notify status of 'nfc.enabled'
> I don't know what's this.

We are planning on making further modifications to DOM API
- Add onPeerFound(), onPeerLost() etc..
- Add onPoweroff() , onPowerOn()   [This is as per W3c specs]

All these functionalities need EventHandlers in DOM code and also few changes in Nfc.js.
I was simply referring to this. You may ignore my comment on this one for now.

> You add another level 'contetn' here?
Done. Taken care
Attachment #824492 - Attachment is obsolete: true
Attachment #825049 - Attachment is obsolete: true
Yoshi,
- I have removed 'message.requestId' assignment in the worker code for all responses.
- Also removed 'setNFCPowerConfig' wrapper as discussed. Also now Nfc.js would only set the config if and only if the value changes
Blocks: 933136
Blocks: 933141
Comment on attachment 825113 [details] [diff] [review]
0001-PATCH-PATCH-4-7-Patch-4-v18-NFC-Chrome-Worker-Implem.patch

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

Mostly good here.
Please file bugs for follow-ups for what we have discussed before and add them into // TODO: Bug .... inside this patch.

- Maintain callback in nfc_worker.js 
- File a follow-up bug to move some of SystemWorkerManager.cpp code into ipc/nfc/
- the encodedNdefRecords in NfcContentHelper
- Revising sendNfcErrorMessage in Nfc.js with more error message.

::: dom/system/gonk/Nfc.js
@@ +128,5 @@
> +
> +    _unregisterMessageTarget: function _unregisterMessageTarget(sessionToken, target) {
> +      if (sessionToken == null) {
> +        // Unregister the target for every sessionToken when no sessionToken is specified.
> +        for (let type of this.sessionTokens) {

let session of this.sessionTokens

@@ +129,5 @@
> +    _unregisterMessageTarget: function _unregisterMessageTarget(sessionToken, target) {
> +      if (sessionToken == null) {
> +        // Unregister the target for every sessionToken when no sessionToken is specified.
> +        for (let type of this.sessionTokens) {
> +          this._unregisterMessageTarget(type, target);

s/type/session/

@@ +144,5 @@
> +      if (target == null) {
> +        debug("Unregistered all targets for the " + sessionToken + " targets: " + targets);
> +        targets = [];
> +        let list = this.sessionTokens;
> +        debug("SessionToken List : " + list);

remove

@@ +149,5 @@
> +        if (sessionToken !== null) {
> +          let index = list.indexOf(sessionToken);
> +          if (index > -1) {
> +            list.splice(index, 1);
> +            debug("Updated sessionToken list : " + list);

remove

@@ +158,5 @@
> +
> +      let index = targets.indexOf(target);
> +      if (index != -1) {
> +        targets.splice(index, 1);
> +        debug("Unregistered " + sessionToken + " target: " + target);

remove

@@ +222,5 @@
> +      }
> +    },
> +
> +    sendNfcResponseMessage: function sendNfcResponseMessage(message, data) {
> +      debug("sendNfcResponseMessage :" + message);

remove

@@ +298,5 @@
> +    }
> +
> +    let nfcMsgType = message.name + "Response";
> +    message.target.sendAsyncMessage(nfcMsgType, {
> +      type: nfcMsgType,

remove this.

@@ +301,5 @@
> +    message.target.sendAsyncMessage(nfcMsgType, {
> +      type: nfcMsgType,
> +      sessionId: message.json.sessionToken,
> +      requestId: message.json.requestId,
> +      status: NFC.GECKO_NFC_ERROR_GENERIC_FAILURE

Please file another bug for adding more information for the error message.
GENERIC_ERROR is not easy to understand what went wrong here.

@@ +395,5 @@
> +                " from a content process with no 'nfc-write' privileges.");
> +          this.sendNfcErrorResponse(message);
> +          return null;
> +        }
> +        break;

Add a default check here to return error earlier.

@@ +403,5 @@
> +    switch (message.name) {
> +       case "NFC:SetSessionToken":
> +         break;
> +       default:
> +         if (message.json.sessionToken !== this.sessionTokenMap[this._currentSessionId]) {

seems 
if (message.name !== "NFC:SetSessionToken" &&
    message.json.sessionToken !== this.sessionTokenMap[this._currentSessionId) {

}

is easier here.

@@ +487,5 @@
> +    }
> +  },
> +
> +  setConfig: function setConfig(prop) {
> +    // Add to property set. -1 if no change.

remove

@@ +490,5 @@
> +  setConfig: function setConfig(prop) {
> +    // Add to property set. -1 if no change.
> +    let configset = {
> +      powerLevel: prop.powerLevel
> +    };

Where did you use this variable?

::: dom/system/gonk/NfcContentHelper.js
@@ +76,5 @@
> +  }),
> +
> +  _requestMap: null,
> +
> +  encodeNdefRecords: function encodeNdefRecords(records) {

remove this function,
or add TODO here to show there's a bug for what you have met.

::: dom/system/gonk/nfc_worker.js
@@ +103,5 @@
> +  /**
> +   * Unmarshals a NDEF message
> +   */
> +  unMarshallNdefMessage: function unMarshallNdefMessage() {
> +      let records = [];

nit, indent, use two spaces.
Allocate 'records' only numOfRecords is > 0.

@@ +130,5 @@
> +        let payloadLength = Buf.readInt32();
> +        let payload = [];
> +        for (let i = 0; i < payloadLength; i++) {
> +          payload.push(Buf.readUint8());
> +        }

Why don't you use readUint8Array here?

@@ +148,5 @@
> +   * Read and return NDEF data, if present.
> +   */
> +  readNDEF: function readNDEF(message) {
> +    let cb = function callback() {
> +      let records = [];

remove
Attachment #825113 - Attachment is obsolete: true
Addressed all the above comments except one

>     let payloadLength = Buf.readInt32();
>     let payload = [];
>     for (let i = 0; i < payloadLength; i++) {
>       payload.push(Buf.readUint8());
>     }

>>> Why don't you use readUint8Array here?

After the recent Gonk protocol change (to pass NDEF in Tech-Discovered), the unmarshalled NDEF message from worker goes to Nfc.js and then switches process boundaries to System application through a 'broadcast message'. It looks like there is a limitation in passing Uint8Arrray object through system messages. Hence had to use what I used.

Earlier this was not needed as NDEF message is dispatched through ReadNDEFResponse(). The flow was
NfcWorker --> Nfc.js --> NfcContentHelper --> DOM --> System Application [This path seems to support uint8Array] as opposed to our current path : NfcWorker --> Nfc.js --> (System Message - broadcast) --> System Application
Yes I will raise bugs as soon as I can
- Maintain callback in nfc_worker.js 
- File a follow-up bug to move some of SystemWorkerManager.cpp code into ipc/nfc/
- the encodedNdefRecords in NfcContentHelper
- Revising sendNfcErrorMessage in Nfc.js with more error message.
Attachment #825049 - Flags: review?(allstars.chh)
Attachment #825181 - Flags: review?(allstars.chh)
Attachment #825181 - Attachment filename: 0001-PATCH-PATCH-4-7-Patch-4-v19-NFC-Chrome-Worker-Implem.patch → Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh
Attachment #825181 - Attachment description: 0001-PATCH-PATCH-4-7-Patch-4-v19-NFC-Chrome-Worker-Implem.patch → Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh
Attachment #825181 - Attachment filename: Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh → 0001-PATCH-PATCH-4-7-Patch-4-v19-NFC-Chrome-Worker-Implem.patch
Comment on attachment 825181 [details] [diff] [review]
Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh

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

Please file the bugs first and then add them into TODO,
like 

// TODO: Bug XXXX 

If you just add // TODO,
nobody will know what bug is tracking this.

::: dom/system/gonk/Nfc.js
@@ +482,5 @@
> +    let outMessage = {
> +      type: "config",
> +      powerLevel: prop.powerLevel
> +    };
> +    this.sendToWorker("config", outMessage);

Can't it just 
this.sendToWorker("config", prop)?

::: dom/system/gonk/NfcContentHelper.js
@@ +291,5 @@
> +
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, result.status);
> +    } else {
> +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester));

Why did we need this?
Attachment #825181 - Flags: review?(allstars.chh)
(In reply to Siddartha P from comment #360)
> Addressed all the above comments except one
> 
> >     let payloadLength = Buf.readInt32();
> >     let payload = [];
> >     for (let i = 0; i < payloadLength; i++) {
> >       payload.push(Buf.readUint8());
> >     }
> 
> >>> Why don't you use readUint8Array here?
> 
> After the recent Gonk protocol change (to pass NDEF in Tech-Discovered), the
> unmarshalled NDEF message from worker goes to Nfc.js and then switches
> process boundaries to System application through a 'broadcast message'. It
> looks like there is a limitation in passing Uint8Arrray object through
> system messages. Hence had to use what I used.
> 
> Earlier this was not needed as NDEF message is dispatched through
> ReadNDEFResponse(). The flow was
> NfcWorker --> Nfc.js --> NfcContentHelper --> DOM --> System Application
> [This path seems to support uint8Array] as opposed to our current path :
> NfcWorker --> Nfc.js --> (System Message - broadcast) --> System Application

Not sure what you're explaining here, 
but why you use readUint8Array for type, and id.
But for payload you use normal array?

if you have problems with TypedArray, that should be all use [], not just for payload.
Comment on attachment 825181 [details] [diff] [review]
Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +232,5 @@
> +    }
> +
> +    debug("fire request success, id: " + requestId +
> +          ", result: " + JSON.stringify(result));
> +    Services.DOMRequest.fireSuccess(request, result);

Please check RILContentHelper.js also.
If you want to post some object to Gaia, 
you need to have some __exposedProps__ property to declare the properties can be accessed.

@@ +235,5 @@
> +          ", result: " + JSON.stringify(result));
> +    Services.DOMRequest.fireSuccess(request, result);
> +  },
> +
> +  fireRequestSuccessAsync: function fireRequestSuccessAsync(requestId, result) {

Who called this?

@@ +255,5 @@
> +          ", result: " + JSON.stringify(error));
> +    Services.DOMRequest.fireError(request, error);
> +  },
> +
> +  fireRequestErrorAsync: function fireRequestErrorAsync(requestId, error) {

Who called this?
Comment on attachment 825181 [details] [diff] [review]
Patch 4 (v19) - NFC Chrome Worker Implementation r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +291,5 @@
> +
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, result.status);
> +    } else {
> +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester));

Sorry, I mean why do we need ObjectWrapper here?
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #363)
> (In reply to Siddartha P from comment #360)
> > Addressed all the above comments except one
> > 
> > >     let payloadLength = Buf.readInt32();
> > >     let payload = [];
> > >     for (let i = 0; i < payloadLength; i++) {
> > >       payload.push(Buf.readUint8());
> > >     }
> > 
> > >>> Why don't you use readUint8Array here?
> > 
> > After the recent Gonk protocol change (to pass NDEF in Tech-Discovered), the
> > unmarshalled NDEF message from worker goes to Nfc.js and then switches
> > process boundaries to System application through a 'broadcast message'. It
> > looks like there is a limitation in passing Uint8Arrray object through
> > system messages. Hence had to use what I used.
> > 
> > Earlier this was not needed as NDEF message is dispatched through
> > ReadNDEFResponse(). The flow was
> > NfcWorker --> Nfc.js --> NfcContentHelper --> DOM --> System Application
> > [This path seems to support uint8Array] as opposed to our current path :
> > NfcWorker --> Nfc.js --> (System Message - broadcast) --> System Application
> 
> Not sure what you're explaining here, 
> but why you use readUint8Array for type, and id.
> But for payload you use normal array?
> 
> if you have problems with TypedArray, that should be all use [], not just
> for payload.

Just a quick comment: I've been working on using Uint8Array for type, id, and payload. There is a bug in Gecko when passing Uint8Arrays that Kyle confirmed. The code lives in branch uint8array_cpp of our forked Gecko repo.
Comment on attachment 824518 [details] [diff] [review]
Patch 7 (v6) NFC Gonk Misc r=mwu

NFCD landed, now an obsolete patch.
Attachment #824518 - Attachment is obsolete: true
(In reply to Garner Lee from comment #367)
> Comment on attachment 824518 [details] [diff] [review]
> Patch 7 (v6) NFC Gonk Misc r=mwu
> 
> NFCD landed, now an obsolete patch.

https://github.com/mozilla-b2g/gonk-misc/commit/564d919576716f81ee85605d450b34e49ca13861
New code based on RIL ipc.
Attachment #820056 - Attachment is obsolete: true
Attachment #825704 - Flags: review?(allstars.chh)
Update makefiles (NFC ipc change).
Attachment #824520 - Attachment is obsolete: true
(In reply to Garner Lee from comment #370)
> Created attachment 825706 [details] [diff] [review]
> Patch 6 (v16) - NFC Makefiles and Manifests r=gps, r=khuey
> 
> Update makefiles (NFC ipc change).

This patch is dependent on next version of Chrome Worker patch changes (pending).
Attachment #825704 - Attachment description: Move most NFC ipc code from SystemWorkerManager to ipc/nfc. → Patch 7 (v1) Move most NFC ipc code from SystemWorkerManager to ipc/nfc.
Blocks: 933593
Blocks: 933595
Blocks: 933591
Comment on attachment 825704 [details] [diff] [review]
Patch 7 (v1) Move most NFC ipc code from SystemWorkerManager to ipc/nfc.

Moved to Bug 933635.
Attachment #825704 - Attachment is obsolete: true
Attachment #825704 - Flags: review?(allstars.chh)
Attachment #825181 - Attachment is obsolete: true
Yoshi,
Raised two bugs and added them to TODO comments, and other comments have been taken care of in v20 of ChromeWorker patch
I assume that we discussed the other main items/ points during the meeting
Comment on attachment 825780 [details] [diff] [review]
Patch 4 (v20) - NFC Chrome Worker Implementation r=allstars.chh

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

Please file bugs for follow-ups.

Thank you for this hard work.

::: dom/system/gonk/Nfc.js
@@ +393,5 @@
> +        }
> +        break;
> +      case "NFC:SetSessionToken":
> +        //Do nothing here. No need to process this message further
> +        return;

nil, return null;

@@ +429,5 @@
> +        break;
> +      default:
> +        debug("UnSupported : Message Name " + message.name);
> +        break;
> +    }

I think we need to return something from this function, maybe null?

::: dom/system/gonk/NfcContentHelper.js
@@ +80,5 @@
> +  /* TODO: This is a limitation when a DOMString is used in sequences of Moz DOM Objects.
> +   *       Strings such as 'type', 'id' 'payload' will not be acccessible to NfcWorker.
> +   *       Therefore this function exists till the bug is addressed.
> +   */
> +  encodeNdefRecords: function encodeNdefRecords(records) {

File a bug for this, please.

@@ +313,5 @@
> +
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, result.status);
> +    } else {
> +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester));

remove ObjectWrapper?

Please file another bug for this.
Attachment #825780 - Flags: review+
Blocks: 933665
Comment on attachment 825780 [details] [diff] [review]
Patch 4 (v20) - NFC Chrome Worker Implementation r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +235,5 @@
> +          ", result: " + JSON.stringify(result));
> +    Services.DOMRequest.fireSuccess(request, result);
> +  },
> +
> +  fireRequestSuccessAsync: function fireRequestSuccessAsync(requestId, result) {

Remove this, I mentioned this in v19.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #375)
> ::: dom/system/gonk/NfcContentHelper.js
> @@ +80,5 @@
> > +  /* TODO: This is a limitation when a DOMString is used in sequences of Moz DOM Objects.
> > +   *       Strings such as 'type', 'id' 'payload' will not be acccessible to NfcWorker.
> > +   *       Therefore this function exists till the bug is addressed.
> > +   */
> > +  encodeNdefRecords: function encodeNdefRecords(records) {
> 
> File a bug for this, please.

Bug 933665
> 
> @@ +313,5 @@
> > +
> > +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> > +      this.fireRequestError(requestId, result.status);
> > +    } else {
> > +      this.fireRequestSuccess(requestId, ObjectWrapper.wrap(result, requester));
> 
> remove ObjectWrapper?
> 
> Please file another bug for this.

Bug 933671
Attachment #825780 - Attachment is obsolete: true
Associated Bugzilla entries to 'TODO comments' in the code for future tracking. Other minor comments are addressed in v21.
Comment on attachment 826167 [details] [diff] [review]
Patch 4 (v21) - NFC Chrome and Worker Implementation r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +264,5 @@
> +        break;
> +    }
> +  },
> +
> +  handleReadNDEFResponse: function handleReadNDEFResponse(message) {

Hold on a sec,
Why did you add this function?
In v20 there's no this funciton.

Also during the previous review(v20), I was told by Garner we still need ObjectWrapper
So I filed Bug 933671 to discuss to remove it or not.

But it seems we actually don't need ObjectWrapper here. (as I asked in patch v19, Comment 375)

I am going to r- this patch for two reasons:

1. You add some code which is not contained in the previous patch, also this code is not asked by me, either.
2. Please explain why ObjectWrapper is still here in this patch.

Since I have r+ your previous patch. I am okay if 
- you're going to restore your v20 patch, and address my comment in Comment 375, Comment 376.
  I am okay to move the discussion for ObjectWrapper to Bug 933671.
  But still, I will check your patch again in case you still add something which isn't reviewed.

- We discuss it in this bug, which I'll close Bug 933671.
Attachment #826167 - Flags: review-
Modified NfcContentHelper.js ONLY to remove 'ObjectWrapper'.
Attachment #826167 - Attachment is obsolete: true
Yoshi,
The changes in v22 only include modification to NfcContentHelper.js.
Current handling of 'Ndef Read Response' need not have to use 'Object Wrapper'. Instead, I have applied the fix / patch suggested by Arno and Kyle.
For Non-NdefRead responses such as say Connect / Close etc.., we need not use 'Object Wrapper', as it they are simple responses (and not JS objects) fired to the upper layers.
Maybe its a good idea for us to go through this latest (v22) patch as it is more appropriate than the previous ones. Also we can close Bug 933671 as well. 
Actually v20 contains this newly added 'handleReadNDEFResponse', but anyways that is beside the point.
nfc-powerlevel-change message appeared since last permisisons update (needed settings app in Gaia).
Attachment #810262 - Attachment is obsolete: true
Attachment #827123 - Flags: review?(fabrice)
Minor. Makefiles updated 11/4, and removed redundant NFC IPC Makefiles (hg conflict patch: Bug 933635). Flagged re-review. Full patch stack now submitted to try.
Attachment #825706 - Attachment is obsolete: true
Attachment #827132 - Flags: review?(khuey)
Comment on attachment 826963 [details] [diff] [review]
Patch 4 (v22) - NFC Chrome and Worker Implementation r=allstars.chh

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

I assume you'd like to request for review here.

As I said before, this patch has some code hasn't been reviewed yet.

::: dom/system/gonk/NfcContentHelper.js
@@ +20,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ObjectWrapper.jsm");

rm

@@ +273,5 @@
> +             " message.sessionToken=" + message.sessionToken);
> +       return; // Nothing to do in this instance.
> +    }
> +    delete this._requestMap[message.requestId];
> +    let result = message.records;

s/result/records/
and below.

@@ +279,5 @@
> +
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, message.status);
> +    } else {
> +      let ndefMsg = new Array();

let ndefMsg = [];

@@ +280,5 @@
> +    if (message.status !== NFC.GECKO_NFC_ERROR_SUCCESS) {
> +      this.fireRequestError(requestId, message.status);
> +    } else {
> +      let ndefMsg = new Array();
> +      for(let i = 0; i < result.length; i++) {

nit, space after for
Attachment #826963 - Flags: review+
Try outputs:
IPC only (prerequisite): See https://bugzilla.mozilla.org/show_bug.cgi?id=933635#c6 

IPC + NFC DOM, largely green:
https://tbpl.mozilla.org/?tree=Try&rev=33dd611e4752

Will post checkin-needed of both once comments and nits fixed.
Yoshi, v23 of NfcChrome worker patch fixes only the comments that you raised on v22.
Attachment #826963 - Attachment is obsolete: true
Comment on attachment 827132 [details] [diff] [review]
Patch 6 (v17) - NFC Makefiles and Manifests r=gps, r=khuey

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

::: browser/installer/package-manifest.in
@@ +190,5 @@
>  @BINPATH@/components/dom_media.xpt
>  @BINPATH@/components/dom_network.xpt
> +#ifdef MOZ_NFC
> +@BINPATH@/components/dom_nfc.xpt
> +#endif

Why do you have this here on desktop, but none of the manifests or JS files?  That doesn't make sense.
Attached patch 0001-Sort-array-elements.patch (obsolete) — Splinter Review
Hi Garner,

When trying these patches I always get the error below. Can you merge this fix into the build-system patch? Thanks.

>>>

Traceback (most recent call last):
  File "./config.status", line 935, in <module>
    config_status(**args)
  File "/home/mozilla/Projects/mozilla/src/B2G-master-emulator-jb/gecko/build/ConfigStatus.py", line 97, in config_status
    summary = backend.consume(definitions)
  File "/home/mozilla/Projects/mozilla/src/B2G-master-emulator-jb/gecko/python/mozbuild/mozbuild/backend/base.py", line 203, in consume
    for obj in objs:
  File "/home/mozilla/Projects/mozilla/src/B2G-master-emulator-jb/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 82, in emit
    for out in output:
  File "/home/mozilla/Projects/mozilla/src/B2G-master-emulator-jb/gecko/python/mozbuild/mozbuild/frontend/reader.py", line 651, in read_mozbuild
    raise bre
mozbuild.frontend.reader.BuildReaderError: ==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/mozilla/Projects/mozilla/src/B2G-master-emulator-jb/gecko/dom/webidl/moz.build

The error was triggered on line 481 of this file:

    'MozNfc.webidl',

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['UnsortedError: An attempt was made to add an unsorted sequence to a list. The incoming list is unsorted starting at element 0. We expected "MozNdefRecord.webidl" but got "MozNFCPeer.webidl"\n']

*** Fix above errors and then restart with               "make -f client.mk build"
Attachment #827967 - Flags: feedback?(dgarnerlee)
Update webidl patch (11/4 gecko update). (makefile: swapped array sort order, boolean --> void setSessionToken).
Attachment #823735 - Attachment is obsolete: true
Attachment #828121 - Flags: review?(khuey)
Update all patches to 11/4 88bcfa3e2dc17ad641152e9da84391114b234140 gecko. Update patch 3 to match new patch 4 (updated boolean --> void setSessionToken).
Attachment #823736 - Attachment is obsolete: true
Attachment #828132 - Flags: review?(khuey)
Comment on attachment 827967 [details] [diff] [review]
0001-Sort-array-elements.patch

Fixed: Uploaded new patch 1, which also caused upload of matching patch 3, patch 4. I'll rerun the patches local and on try to re-verify new set.
Attachment #827967 - Flags: feedback?(dgarnerlee)
Attachment #827123 - Flags: review?(fabrice) → review+
Comment on attachment 827123 [details] [diff] [review]
Patch 5 (v9) - NFC Permissions Changes r=fabrice

Thanks! Adding r=
Attachment #827123 - Attachment description: Patch 5 (v9) - NFC Permissions Changes → Patch 5 (v9) - NFC Permissions Changes r=fabrice
Removed browser/installer/package-manifest.in from build file set (Comment 389).
Attachment #827132 - Attachment is obsolete: true
Attachment #827132 - Flags: review?(khuey)
Attachment #828390 - Flags: review?(khuey)
This bug is trying to implement the NFC API defined at https://wiki.mozilla.org/WebAPI/WebNFC . Parall to this W3C is also working on standardizing NFC API http://www.w3.org/2012/nfc/web-api/. I can see difference between these two API.
Is there any plan to adopt w3c version of API at later date? Please forgive my ignorance if this is already discussed.
Try server output with current patch set:
https://tbpl.mozilla.org/?tree=Try&rev=409737efa606
Keywords: checkin-needed
(In reply to Satyanarayana Nalluri from comment #396)
> This bug is trying to implement the NFC API defined at
> https://wiki.mozilla.org/WebAPI/WebNFC . Parall to this W3C is also working
> on standardizing NFC API http://www.w3.org/2012/nfc/web-api/. I can see
> difference between these two API.
> Is there any plan to adopt w3c version of API at later date? Please forgive
> my ignorance if this is already discussed.

Yes, the current apis are understood to be not quite W3C draft standard compliant yet, and are thus are prefixed with Moz until they do match. The biggest difference currently is the need to support WebActivities (MozActivities), to route NDEF messages and launch applications selectively.
Your Try push is showing B2G mochitest failures in test_interfaces.html.
https://tbpl.mozilla.org/php/getParsedLog.php?id=30249472&tree=Try
Keywords: checkin-needed
^---

Kyle, I believe we need to update the test cases to allow the new navigator DOM objects, or is there a way to determine the app/webpage/internet JS boundary? If I need to update the test cases, who should I contact?
Flags: needinfo?(khuey)
Flags: needinfo?(bugs)
Yes, the test case should be changed.  Any changes to test_interfaces.html need review from a DOM peer.  You should upload a patch that fixes the file, and get review from khuey or smaug.
(assuming you mean for them to be exposed to random webpages of course)
Flags: needinfo?(khuey)
Flags: needinfo?(bugs)
Add NFC DOM objects expected fail cases to test_interfaces.html.
It looks like we got all r+ now, and I just assign the target milestone as sprint 5. Garner, please feel free to change the target milestone if you're seeing it differently. Thanks!
Target Milestone: --- → 1.3 Sprint 5 - 11/22
mqueue-less hg try with updated test_interfaces.html, mochitest-8: https://tbpl.mozilla.org/?tree=Try&rev=e0d04c9d0181
Comment on attachment 829026 [details] [diff] [review]
0001-Add-new-allowed-NFC-DOM-interfaces-to-test_interface.patch

https://tbpl.mozilla.org/?tree=Try&rev=e0d04c9d0181

mochitest-8:
https://tbpl.mozilla.org/php/getParsedLog.php?id=30310649&tree=Try&full=1
Attachment #829026 - Flags: review?(khuey)
I have a new try server run with a cleaner hg queue (same patch set otherwise):
https://tbpl.mozilla.org/?tree=Try&rev=ff6750fde0d4
Blocks: 937430
Your patch doesn't appear to actually work ... (see the M8 failures on b2g?).
Actually, perhaps some more is needed (mochitests are completely new to me) (are you on tonight's call?). First, is the test_interfaces.html modification correct, and if all the toString methods "intermittent fails" are pointing to NFC related code?
Well, it's not clear to me why you removed MozOtaStatusEvent but other than that it is ok.

The toString things look suspiciously like that "security error that's not a security error" thing we debugged a few weeks ago ...
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #410)
> Well, it's not clear to me why you removed MozOtaStatusEvent but other than
> that it is ok.
Yeah. Adding that back soon...

> 
> The toString things look suspiciously like that "security error that's not a
> security error" thing we debugged a few weeks ago ...

I will update the gecko codebase, and try to see what's going on there. Was there a specific cause and solution in that instance? NFC isn't really in any test cases yet, though there's emulator work for that.
Well before the problem was that something didn't QI to nsIMessageListener but was being passed to add(Weak?)Listener?
It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.
Minor update. Sync to 11/11 gecko. Migrate local directories from system/gonk Makefile.in to moz.build.
Attachment #828390 - Attachment is obsolete: true
Attachment #830939 - Flags: review?(khuey)
Attachment #830939 - Flags: feedback?
Blocks: 933116
Comment on attachment 829026 [details] [diff] [review]
0001-Add-new-allowed-NFC-DOM-interfaces-to-test_interface.patch

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

Garner is going to provide a new patch here.
Attachment #829026 - Flags: review?(khuey)
Updated test_interfaces.html to not have a preference value. Running try "all" now.
Attachment #829026 - Attachment is obsolete: true
Attachment #831321 - Flags: review?
Attachment #831321 - Flags: review? → review?(khuey)
Pending full Try test output: https://tbpl.mozilla.org/?tree=Try&rev=71861ca982ab
A now working try test that passes mochitest-8: https://tbpl.mozilla.org/?tree=Try&rev=034787b14247

Will request landing after 71861ca982ab, or right now if when 034787b14247 is sufficient.
Comment on attachment 827967 [details] [diff] [review]
0001-Sort-array-elements.patch

Applied. Thanks. Obsoleting...
Attachment #827967 - Attachment is obsolete: true
Latest Try results from today's gecko branch: https://tbpl.mozilla.org/?tree=Try&rev=fa78aba70723
Keywords: checkin-needed
Thanks Kyle!
Ken, Kyle: 
The code (the other half is in nfc manager/settings UI), is disabled by default by the preference Gaia's settings.js "nfc.enabled: false" . Do we need test cases for certified apps only code?
Flags: needinfo?(khuey)
Flags: needinfo?(kchang)
(In reply to Garner Lee from comment #423)
> Ken, Kyle: 

To be clear, we have nfc-demo currently in Gaia to drive manual unit tests. It becomes potentially automation testable when NFCD, NFC DOM, and NfcManager lands, and we can flip nfc.enabled on (sessionToken enforcement for security blocks DOM usage entirely).
(In reply to Garner Lee from comment #424)
> (In reply to Garner Lee from comment #423)
> > Ken, Kyle: 
> 
> To be clear, we have nfc-demo currently in Gaia to drive manual unit tests.
I wonder if this test is what we want...:(
Here is the bug for worker test, Bug 907252.
And after we have NFC emulator, Bug 916863, we can add some marionette test for API.
Flags: needinfo?(kchang)
QA Contact: wachen
For posterity, secreview was documented here https://wiki.mozilla.org/WebAPI/Security/WebNFC
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.