Bug 674741 (webnfc)

WebNFC (near-field communication)

RESOLVED FIXED in Firefox 28

Status

Firefox OS
NFC
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: cjones, Assigned: Garner Lee)

Tracking

(Blocks: 3 bugs, {dev-doc-needed, privacy-review-needed})

unspecified
1.3 Sprint 5 - 11/22
Other
Linux
dev-doc-needed, privacy-review-needed
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

(Whiteboard: [tech-p1][FT:RIL], URL)

Attachments

(9 attachments, 139 obsolete attachments)

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.

Comment 1

5 years ago
Created attachment 591592 [details]
API proposal

Comment 2

5 years ago
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. :)

Comment 6

5 years ago
@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
Keywords: dev-doc-needed
there were some interesting attacks and ideas shown at cansec that we may want to be cognizant of as we move forward with this
Keywords: sec-review-needed
Keywords: privacy-review-needed
needs to be scheduled for a team review
Whiteboard: [secr:cutisk]
Depends on: 749325
Whiteboard: [secr:cutisk] → [sec-assigned:cutisk:749325]

Comment 11

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

See https://wiki.mozilla.org/WebAPI/WebNFC for API draft
Attachment #618790 - Flags: review?(kyle)
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.
Blocks: 715784
Unless something has changed drastically, this is not a blocker for V1.

Updated

5 years ago
No longer blocks: 715784

Comment 15

5 years ago
Created attachment 633560 [details] [diff] [review]
Rebased first draft implementation
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.

Comment 19

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

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

- Not implemented feedback:
    o Left NfcCallback as member class as it doesn't really make sense to separate it and RIL does this as well ;)
    o Left NfcData as char* cause we treat it like that on the daemon side and base64 encode content anyways
Attachment #633560 - Attachment is obsolete: true
Attachment #647307 - Flags: review?(kyle)
Flags: sec-review?(curtisk)
Whiteboard: [sec-assigned:cutisk:749325]
(Assignee)

Comment 20

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

Comment 21

5 years ago
Request for top level review for Andreas Gal.
(Assignee)

Comment 22

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

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

Comment 24

5 years ago
Created attachment 678622 [details] [diff] [review]
Gecko NFC feature patch
Attachment #678560 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 678623 [details] [diff] [review]
Gaia NFC feature patch
(Assignee)

Comment 26

5 years ago
Created attachment 678624 [details] [diff] [review]
Gonk-misc NFC feature patch
(Assignee)

Comment 27

5 years ago
Created attachment 678625 [details] [diff] [review]
B2G (source root) NFC feature patch
Attachment #678622 - Flags: review?(gal)
Attachment #678619 - Attachment is obsolete: true
Attachment #678619 - Flags: review?(gal)
Attachment #678623 - Flags: review?(gal)
Attachment #678624 - Flags: review?(gal)
Attachment #678625 - Flags: review?(gal)
(Assignee)

Comment 28

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

Updated

5 years ago
Attachment #678633 - Flags: review+
(Assignee)

Updated

5 years ago
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)

Comment 30

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

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

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

Here is what I propose instead:

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

Comment 31

5 years ago
Created attachment 678896 [details]
libhardware.js

Comment 32

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

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

Comment 33

5 years ago
Markus, Arno, what do you guys think?

Comment 34

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

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

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

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

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

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

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

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

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

Comment 35

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

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

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

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

Comment 36

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

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

Comment 37

5 years ago
Created attachment 679491 [details]
bindings example

Comment 38

5 years ago
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?
(Assignee)

Comment 39

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

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

Comment 40

5 years ago
Created attachment 683765 [details] [diff] [review]
Proposed changes to have android system property to enable/disable nfc.
(Assignee)

Comment 41

5 years ago
Created attachment 683781 [details] [diff] [review]
Proposed changes to have B2G system property to enable/disable nfc.
Attachment #683765 - Attachment is obsolete: true
(Assignee)

Comment 42

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

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

In debug only code block:

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

        return NS_ERROR_NOT_INITIALIZED;
      }
    }
(Assignee)

Comment 43

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

Fixed gecko over the last gecko_06.patch. Can pass DEBUG check on new xpcom object. (not fully working yet in gaia).
Attachment #689930 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
Created attachment 695045 [details] [diff] [review]
NFC Gaia merge git.mozilla.org e421765cf2f739dbff0051f2f6b0c770e1e9cd51
(Assignee)

Comment 45

5 years ago
Created attachment 695046 [details] [diff] [review]
NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474 (release)
Attachment #679896 - Attachment is obsolete: true
Attachment #690026 - Attachment is obsolete: true
(Assignee)

Comment 46

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

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

Comment 47

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

Comment 49

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

Comment 50

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

Yes, new try submit below has it removed.

Comment 51

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

Comment 52

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

Comment 53

4 years ago
Created attachment 709320 [details] [diff] [review]
Latest gecko code, Mercurial modified version of the same patch.
(Assignee)

Updated

4 years ago
Attachment #709320 - Flags: review?(vyang)
(Assignee)

Comment 54

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

Updated

4 years ago
Attachment #695046 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → dgarnerlee
(Assignee)

Comment 56

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

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

Comment 57

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

Comment 58

4 years ago
Comment on attachment 591592 [details]
API proposal

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

Updated

4 years ago
Attachment #591592 - Attachment is obsolete: true
(Assignee)

Comment 59

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

This Gecko DOM patch contains the interface changes needed to enable NFC.
Attachment #709320 - Attachment is obsolete: true
(Assignee)

Comment 60

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

This Gecko DOM implementation code for NFC IDL files.
(Assignee)

Comment 61

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

Comment 62

4 years ago
Created attachment 713220 [details] [diff] [review]
NFC system/gonk and permission patch implemenation.
(Assignee)

Comment 63

4 years ago
Created attachment 713222 [details] [diff] [review]
NFC makefiles

Makefiles for nfc. There is additionally a system property that enables the compiled nfc feature at runtime (setprop nfc.enabled) in the previous attachment 683781 [details] [diff] [review].
Blocks: 844910
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) ?
(Assignee)

Comment 66

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

Comment 67

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

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

Comment 68

4 years ago
Created attachment 730467 [details] [diff] [review]
NFC gecko makefiles
Attachment #713222 - Attachment is obsolete: true
(Assignee)

Comment 69

4 years ago
Created attachment 730468 [details] [diff] [review]
NFC DOM cpp/c/h implementation files.
Attachment #713215 - Attachment is obsolete: true
(Assignee)

Comment 70

4 years ago
Created attachment 730470 [details] [diff] [review]
NFC DOM Interface files
Attachment #713211 - Attachment is obsolete: true
(Assignee)

Comment 71

4 years ago
Created attachment 730471 [details] [diff] [review]
NFC DOM JS, and permissions.
Attachment #713220 - Attachment is obsolete: true
(Assignee)

Comment 72

4 years ago
Created attachment 730473 [details] [diff] [review]
System app update. Platform integration with NFC MozActivities.
Attachment #695045 - Attachment is obsolete: true
(Assignee)

Comment 73

4 years ago
Created attachment 730474 [details] [diff] [review]
NFC Demo. Also accepts ndef-discovered activities.
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?
(Assignee)

Comment 75

4 years ago
Hi Vicamo,

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

Comment 76

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

Removed obsolete telephony Icc{Open,Close,ExchangeAPDU} in favor of IccManager.
Attachment #730468 - Attachment is obsolete: true
(Assignee)

Comment 77

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

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

Updated

4 years ago
Attachment #730470 - Attachment is obsolete: true
(Assignee)

Comment 78

4 years ago
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
Blocks: 860906
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 #730467 - Flags: review?(kyle)
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)
Created attachment 736600 [details] [diff] [review]
Patch 1 (v1) - NFC DOM IDL
Attachment #736600 - Flags: superreview?(mrbkap)
Attachment #736600 - Flags: review?(kyle)
Created attachment 736602 [details] [diff] [review]
Patch 2 (v1) - NFC DOM Boilerplate
Attachment #736602 - Flags: superreview?(mrbkap)
Attachment #736602 - Flags: review?(kyle)
Created attachment 736603 [details] [diff] [review]
Patch 3 (v1) - NFC DOM Implementation
Attachment #736603 - Flags: superreview?(mrbkap)
Attachment #736603 - Flags: review?(kyle)
Created attachment 736604 [details] [diff] [review]
Patch 4 (v1) - NFC Chrome Worker Implementation
Attachment #736604 - Flags: review?(kyle)
Created attachment 736605 [details] [diff] [review]
Patch 5 (v1) - NFC Permissions Changes
Attachment #736605 - Flags: review?(ptheriault)
Created attachment 736606 [details] [diff] [review]
Patch 6 (v1) - NFC Makefiles and Manifests
Attachment #736606 - Flags: review?(kyle)
Created attachment 736607 [details] [diff] [review]
Patch 7 (v1) - NFC B2G IPC Implementation

There's a good chance this is getting rewritten as a UnixSocket implementation, but putting it up for completeness sake for the moment.
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.
(Assignee)

Comment 93

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

Comment 98

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

Comment 99

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

Remove obsolete Permissions.txt changes.
Attachment #736605 - Attachment is obsolete: true
Attachment #737939 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #737939 - Flags: review+ → review?(ptheriault)
Attachment #737939 - Flags: review?(ptheriault) → review+
(Assignee)

Comment 100

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

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)
(Assignee)

Comment 101

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

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)
(Assignee)

Comment 102

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

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-
(Assignee)

Updated

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

Comment 107

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

Comment 108

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

Implement review comments: #ifdef for 2 onndefdiscovered, onndefdisconnected, rename MozNdefRecord to NdefRecord.
Attachment #736602 - Attachment is obsolete: true
(Assignee)

Comment 109

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

Implement review comments: Comment NdefRecord fields, MozNdefRecord --> NdefRecord
Attachment #740998 - Attachment is obsolete: true
Attachment #743943 - Flags: superreview?(mrbkap)
Attachment #743943 - Flags: review?(kyle)
(Assignee)

Comment 110

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

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)
(Assignee)

Comment 111

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

Implement review comments: Rename MozNdefRecord --> NdefRecord
Attachment #736606 - Attachment is obsolete: true
Attachment #736606 - Flags: review?(kyle)
Attachment #743947 - Flags: review?(kyle)
(Assignee)

Updated

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

Comment 116

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

Comment 118

4 years ago
Created attachment 758311 [details] [diff] [review]
Patch 7 (v2) - NFC B2G IPC Implementation
Attachment #758311 - Flags: review+

Updated

4 years ago
Attachment #758311 - Attachment description: Patch 7 (v1) - NFC B2G IPC Implementation → Patch 7 (v2) - NFC B2G IPC Implementation

Comment 119

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

Patch 4 :  NFC Chrome Worker Implementation v3 - Modifies the NFC chrome worker to support UnixSocket implementation.
Attachment #758319 - Flags: superreview?(mrbkap)
Attachment #758319 - Flags: review?

Updated

4 years ago
Attachment #758319 - Flags: review? → review?(kyle)

Updated

4 years ago
Attachment #758311 - Flags: review+ → review?(kyle)

Comment 120

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

Comment 122

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

Comment 128

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

Added WebIDL support for NfcEvent object.
Attachment #743943 - Attachment is obsolete: true
(Assignee)

Comment 129

4 years ago
Created attachment 768417 [details] [diff] [review]
Patch 2 (v3) - NFC DOM Boilerplate
Attachment #743918 - Attachment is obsolete: true
(Assignee)

Comment 130

4 years ago
Created attachment 768419 [details] [diff] [review]
Patch 3 (v4) - NFC DOM Implementation
Attachment #743944 - Attachment is obsolete: true
(Assignee)

Comment 131

4 years ago
Created attachment 768420 [details] [diff] [review]
Patch 4 (v4) - NFC Chrome Worker Implementation
Attachment #758319 - Attachment is obsolete: true
(Assignee)

Comment 132

4 years ago
Created attachment 768421 [details] [diff] [review]
Patch 5 (v3) - NFC Permissions Changes
Attachment #737939 - Attachment is obsolete: true
(Assignee)

Comment 133

4 years ago
Created attachment 768423 [details] [diff] [review]
Patch 6 (v3) - NFC Makefiles and Manifests
Attachment #743947 - Attachment is obsolete: true
(Assignee)

Comment 134

4 years ago
Created attachment 772296 [details]
This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.
(Assignee)

Updated

4 years ago
Attachment #768421 - Flags: review?(ptheriault)
(Assignee)

Comment 135

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

NDEF Protocol updated with additional states.
Attachment #772296 - Attachment is obsolete: true
(Assignee)

Comment 136

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

Update for new worker API (7/12 merge).
Attachment #768420 - Attachment is obsolete: true
(Assignee)

Comment 137

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

Comment 138

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

Updated

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

Updated

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

Comment 141

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

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

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

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

> - The session id is generated randomly by nfcd?

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

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

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

Comment 142

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

Comment 144

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

Comment 148

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

Comment 150

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

Comment 153

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

Comment 154

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

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

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

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

Done.

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

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

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

Done.

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

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

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

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

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

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

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

Good point. We added this.

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

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

Comment 155

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

Comment 156

4 years ago
Created attachment 779560 [details]
(v3) This document describes the NFC API interaction between gecko and the actual NFC library and hardware implementation.
Attachment #775053 - Attachment is obsolete: true
(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
(Assignee)

Comment 159

4 years ago
Created attachment 780052 [details] [diff] [review]
Patch 1 (v5) - NFC DOM IDL
Attachment #768416 - Attachment is obsolete: true
(Assignee)

Comment 160

4 years ago
Created attachment 780053 [details] [diff] [review]
Patch 2 (v4) - NFC DOM Boilerplate
Attachment #768417 - Attachment is obsolete: true
(Assignee)

Comment 161

4 years ago
Created attachment 780054 [details] [diff] [review]
Patch 3 (v5) - NFC DOM Implementation
Attachment #768419 - Attachment is obsolete: true
(Assignee)

Comment 162

4 years ago
Created attachment 780055 [details] [diff] [review]
Patch 5 (v4) - NFC Permissions Changes
Attachment #768421 - Attachment is obsolete: true
Attachment #768421 - Flags: review?(ptheriault)
(Assignee)

Comment 163

4 years ago
Created attachment 780056 [details] [diff] [review]
Patch 6 (v4) - NFC Makefiles and Manifests
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
(Assignee)

Comment 165

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

Comment 168

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

Fixes reported crash issue in DEBUG compile (remove extra NfcEvent registration).
Attachment #780052 - Attachment is obsolete: true
(Assignee)

Comment 169

4 years ago
Created attachment 781129 [details] [diff] [review]
Patch 2 (v5) - NFC DOM Boilerplate
Attachment #780053 - Attachment is obsolete: true
(Assignee)

Comment 170

4 years ago
Created attachment 781130 [details] [diff] [review]
Patch 3 (v6) - NFC DOM Implementation
Attachment #780054 - Attachment is obsolete: true
(Assignee)

Comment 171

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

Remove Secure Element API (now tracked in separate feature bug).
Attachment #776039 - Attachment is obsolete: true
(Assignee)

Comment 172

4 years ago
Created attachment 781133 [details] [diff] [review]
Patch 5 (v5) - NFC Permissions Changes
Attachment #780055 - Attachment is obsolete: true
(Assignee)

Comment 173

4 years ago
Created attachment 781135 [details] [diff] [review]
Patch 6 (v5) - NFC Makefiles and Manifests
Attachment #780056 - Attachment is obsolete: true
(Assignee)

Comment 174

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

Comment 175

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

Comment 176

4 years ago
(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.
Created attachment 781461 [details]
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

Comment 181

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

Comment 182

4 years ago
Created attachment 787980 [details] [diff] [review]
Patch 1 (v7) NFC DOM IDL/WebIDL
Attachment #781128 - Attachment is obsolete: true
(Assignee)

Comment 183

4 years ago
Created attachment 787985 [details] [diff] [review]
Patch 2 (v6) - NFC DOM Boilerplate
Attachment #781129 - Attachment is obsolete: true
(Assignee)

Comment 184

4 years ago
Created attachment 787989 [details] [diff] [review]
Patch 3 (v7) - NFC DOM Implementation
Attachment #781130 - Attachment is obsolete: true
(Assignee)

Comment 185

4 years ago
Created attachment 787990 [details] [diff] [review]
Patch 4 (v7) - NFC Chrome Worker Implementation
Attachment #781132 - Attachment is obsolete: true
(Assignee)

Comment 186

4 years ago
Created attachment 787992 [details] [diff] [review]
Patch 5 (v6) - NFC Permissions Changes
Attachment #781133 - Attachment is obsolete: true
(Assignee)

Comment 187

4 years ago
Created attachment 787993 [details] [diff] [review]
Patch 6 (v6) - NFC Makefiles and Manifests
Attachment #781135 - Attachment is obsolete: true
(Assignee)

Comment 188

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

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

Updated

4 years ago
Attachment #787980 - Flags: superreview?(jonas)
(Assignee)

Updated

4 years ago
Attachment #787985 - Flags: superreview?(jonas)
(Assignee)

Updated

4 years ago
Attachment #787989 - Flags: superreview?
(Assignee)

Updated

4 years ago
Attachment #787994 - Attachment description: PATCH 7 (v2) NFC Gonk Misc → Patch 7 (v2) NFC Gonk Misc
(Assignee)

Updated

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

Comment 190

4 years ago
@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.
No longer depends on: 749325
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?
(Assignee)

Comment 193

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

Updated

4 years ago
Blocks: 894323

Updated

4 years ago
Blocks: 894689

Updated

4 years ago
blocking-b2g: --- → koi+

Comment 194

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

Updated

4 years ago
Blocks: 894672, 904246, 894678, 894676, 894320, 894673, 894691
No longer blocks: 860906
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?

Updated

4 years ago
Component: DOM: Device Interfaces → NFC
Product: Core → Boot2Gecko
Version: Trunk → unspecified
(Assignee)

Comment 197

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

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

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

Comment 198

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

Comment 201

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

Updates. Add config and ndefMakeReadOnly to DOM/nfcd.
Attachment #787980 - Attachment is obsolete: true
(Assignee)

Comment 202

4 years ago
Created attachment 800278 [details] [diff] [review]
Patch 2 (v7) - NFC DOM Boilerplate
Attachment #787985 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #800278 - Flags: superreview?(jonas)
(Assignee)

Comment 203

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

Comment 204

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

Comment 205

4 years ago
Created attachment 800288 [details] [diff] [review]
Patch 4 (v8) - NFC Chrome Worker Implementation
Attachment #787990 - Attachment is obsolete: true
(Assignee)

Comment 206

4 years ago
Created attachment 800289 [details] [diff] [review]
Patch 5 (v7) - NFC Permissions Changes
Attachment #787992 - Attachment is obsolete: true
(Assignee)

Comment 207

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

Add tighter permissions to read and write NFC calls.
(Assignee)

Comment 208

4 years ago
Created attachment 800290 [details] [diff] [review]
Patch 6 (v7) - NFC Makefiles and Manifests
Attachment #787993 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #800288 - Attachment description: atch 4 (v8) - NFC Chrome Worker Implementation → Patch 4 (v8) - NFC Chrome Worker Implementation
(Assignee)

Comment 209

4 years ago
Created attachment 800301 [details] [diff] [review]
Fix debug build - remove unneeded NS_IsMainThread assertion r=allstars.chh
(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.
(Assignee)

Updated

4 years ago
Attachment #800301 - Flags: review?(allstars.chh)
(Assignee)

Comment 211

4 years ago
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);
Depends on: 913336
Depends on: 913340
(Assignee)

Updated

4 years ago
Attachment #800301 - Attachment description: Fix debug build - remove unneeded NS_IsMainThread assertion → Fix debug build - remove unneeded NS_IsMainThread assertion r=allstars.chh
(Assignee)

Updated

4 years ago
Attachment #800289 - Flags: review?(ptheriault)
(Assignee)

Updated

4 years ago
Attachment #800290 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #800277 - Flags: review?(jonas)
(Assignee)

Updated

4 years ago
Attachment #787989 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 218

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

Comment 222

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

Remove nfc.enabled system property from init patch.
Attachment #787994 - Attachment is obsolete: true
Attachment #802122 - Flags: review?(mwu)

Comment 223

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

Updated

4 years ago
Attachment #802122 - Flags: review?(mwu)
(Assignee)

Comment 224

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

Comment 225

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

As per conversation, we're checking Nfc in disabled initially, so the enable flag is removed. Patch updated.
Attachment #802122 - Attachment is obsolete: true
Attachment #802341 - Flags: review?(mwu)

Updated

4 years ago
Attachment #802341 - Flags: review?(mwu) → review+
(Assignee)

Updated

4 years ago
Attachment #802341 - Attachment description: Patch 7 (v4) NFC Gonk Misc → Patch 7 (v4) NFC Gonk Misc r=mwu
(Assignee)

Updated

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

Comment 227

4 years ago
Created attachment 803761 [details] [diff] [review]
Patch 1 (v9) NFC DOM IDL/WebIDL
Attachment #800277 - Attachment is obsolete: true
Attachment #800277 - Flags: review?(jonas)
Attachment #803761 - Flags: review?(bugs)
(Assignee)

Comment 228

4 years ago
Created attachment 803763 [details] [diff] [review]
Patch 2 (v8) - NFC DOM Boilerplate
Attachment #800278 - Attachment is obsolete: true
Attachment #800278 - Flags: superreview?(jonas)
Attachment #803763 - Flags: review?(bugs)
(Assignee)

Comment 229

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

Review comments implemented: rooted playload field in NdefRecord, and additional type checks (String) for security.
Attachment #800284 - Attachment is obsolete: true
Attachment #803764 - Flags: superreview?
(Assignee)

Updated

4 years ago
Attachment #803764 - Flags: superreview? → superreview?(bugs)
(Assignee)

Comment 230

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

Note: this is the non-binary protocol version
Attachment #800288 - Attachment is obsolete: true
Attachment #803769 - Flags: superreview?(bugs)
(Assignee)

Comment 231

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

MOZ_B2G_NFC now set to build, but operate only if nfc.powerlevel is 1 (enabled).
Attachment #800290 - Attachment is obsolete: true
Attachment #800290 - Flags: review?(khuey)
Attachment #803771 - Flags: review?
(Assignee)

Comment 232

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

Updated

4 years ago
Attachment #803771 - Flags: review? → review?(gps)
(Assignee)

Updated

4 years ago
Attachment #800289 - Flags: review+ → review?(fabrice)
(Assignee)

Comment 233

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

Add MOZ_B2G_NFC Compile flag around MozNfcEvent.webidl in new moz.build.
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-
(Assignee)

Updated

4 years ago
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+
Blocks: 916863
(Assignee)

Comment 241

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

Comment 246

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

Comment 248

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

Comment 249

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

Comment 250

4 years ago
Created attachment 810257 [details] [diff] [review]
Patch 1 (v11) NFC DOM IDL/WebIDL

WebIDL conversion of navigator.mozNfc (in JS), NfcEvent (C++), MozNdefRecord (C++).
Attachment #804387 - Attachment is obsolete: true
Attachment #810257 - Flags: review?(bugs)
(Assignee)

Comment 251

4 years ago
Created attachment 810258 [details] [diff] [review]
Patch 2 (v9) - NFC DOM Boilerplate
Attachment #803763 - Attachment is obsolete: true
Attachment #810258 - Flags: review?(bugs)
(Assignee)

Comment 252

4 years ago
Created attachment 810259 [details] [diff] [review]
Patch 3 (v10) - NFC DOM Implementation

nsNfc.js navigator WebIDL DOM implementation.
Attachment #803764 - Attachment is obsolete: true
Attachment #810259 - Flags: review?(bugs)
(Assignee)

Comment 253

4 years ago
Created attachment 810261 [details] [diff] [review]
Patch 4 (v10) - NFC Chrome Worker Implementation
Attachment #803769 - Attachment is obsolete: true
Attachment #803769 - Flags: superreview?(bent.mozilla)
Attachment #810261 - Flags: review?(bent.mozilla)
(Assignee)

Comment 254

4 years ago
Created attachment 810262 [details] [diff] [review]
Patch 5 (v8) - NFC Permissions Changes r=fabrice

Move nfc-manager back down to a simple permission declaration without "read".
Attachment #800289 - Attachment is obsolete: true
Attachment #810262 - Flags: review?(fabrice)
(Assignee)

Comment 255

4 years ago
Created attachment 810266 [details] [diff] [review]
Patch 6 (v9) - NFC Makefiles and Manifests

Updated makefiles for WebIDL implementation.
Attachment #803771 - Attachment is obsolete: true
Attachment #810266 - Flags: review?(gps)
(Assignee)

Comment 256

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

Updated

4 years ago
Attachment #810262 - Attachment description: Patch 5 (v8) - NFC Permissions Changes → Patch 5 (v8) - NFC Permissions Changes r=fabrice

Updated

4 years ago
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;
>+};

Hm