[Gecko] NFC onpeerready, onpeerlost callbacks

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Garner Lee, Assigned: psiddh)

Tracking

unspecified
1.3 Sprint 5 - 11/22
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 26 obsolete attachments)

5.40 KB, patch
Details | Diff | Splinter Review
754 bytes, patch
Details | Diff | Splinter Review
8.49 KB, patch
Details | Diff | Splinter Review
22.56 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Follow up bug for initial NFC DOM.

onpeerfound, onpeerlost are required to support user stories involving pairing. See Bug 933093.

onforegrounddispatch is necessary for an NFC tag writer to receive priority access to the tag it is trying to write. In the normal case, the NFC manager would otherwise route the message to a registered application.
(Reporter)

Updated

4 years ago
Blocks: 933093
(Reporter)

Updated

4 years ago
Depends on: 674741
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 1.3?
Blocks: 860906

Updated

4 years ago
Assignee: nobody → dgarnerlee
blocking-b2g: 1.3? → 1.3+

Comment 1

4 years ago
Hi, do we have a clear view on the target milestone on this bug now? Thanks.
(Reporter)

Comment 2

4 years ago
--> Sid?

(In reply to Kevin Hu [:khu] from comment #1)
> Hi, do we have a clear view on the target milestone on this bug now? Thanks.
Assignee: dgarnerlee → psiddh

Comment 3

4 years ago
Sid, I set 11/22 as target milestoe of this bug.  But I think it is better to finish this as soon as possible, or Gaia developers don't have enough time for development....:(
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(Assignee)

Updated

4 years ago
Summary: [Gecko] NFC onpeerfound, onpeerlost, and onforegrounddispatch callbacks → [Gecko] NFC onpeerfound, onpeerlost callbacks
(Assignee)

Comment 4

4 years ago
Edited the description to onpeerfound, onpeerlost.
Since 'onforegrounddispatch' is not high priority for the P2P usecase that we are addressing, moving this to a new bug - Bug 937430
(Assignee)

Comment 5

4 years ago
Created attachment 830593 [details] [diff] [review]
0001-Bug-933136-Part-1-Add-DOM-event-handlers-peerfound-a.patch
(Assignee)

Comment 6

4 years ago
Created attachment 830594 [details] [diff] [review]
0001-Bug-933136-Part-2-Add-support-to-peerfound-and-peerl.patch
(Assignee)

Comment 7

4 years ago
Submitted initial implementation of 'onpeerfound' & 'onpeerlost'. There are two TBDs:
1) There is still limitation as far these patches are concerned. Chrome (Nfc.js) notifies all the content processes of 'onpeerfound' event.
2) Integrate with Shrinking UI

* Tested these patches for basic user scenarios

Comment 8

4 years ago
It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.
(Assignee)

Comment 9

4 years ago
Created attachment 833412 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey
Attachment #830593 - Attachment is obsolete: true
Attachment #833412 - Flags: feedback?(khuey)
(Assignee)

Comment 10

4 years ago
Created attachment 833413 [details] [diff] [review]
(v1) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome  r=allstars.chh
Attachment #830594 - Attachment is obsolete: true
Attachment #833413 - Flags: feedback?(allstars.chh)
(Assignee)

Comment 11

4 years ago
Added new DOM event handlers and implementation for peerfound and peerlost notifications.
Tested the event handlers with nfc-demo, dialer and other gaia applications for different usecases such as 'registration / unregistration' of Peer events (lost / found)
Comment on attachment 833413 [details] [diff] [review]
(v1) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome  r=allstars.chh

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

8 lines of context, please.
Attachment #833413 - Flags: feedback?(allstars.chh)
Comment on attachment 833412 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey

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

cancel the feedback because the patch doesn't have 8 lines of context.
Please reformat your patch.
Attachment #833412 - Flags: feedback?(khuey)
Comment on attachment 833412 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey

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

cancel the feedback because the patch doesn't have 8 lines of context.
Please reformat your patch.

::: dom/webidl/MozNfc.webidl
@@ +10,4 @@
>     MozNFCTag getNFCTag(DOMString sessionId);
>     MozNFCPeer getNFCPeer(DOMString sessionId);
>  
> +   void setPeerWindow(DOMString manifestUrl);

Can you add some documentation for this?
I don't know how to feedback on this if you don't write any documentation nor updating the MozillaWiki.
(Assignee)

Comment 15

4 years ago
Created attachment 8334166 [details] [diff] [review]
(v1) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome  r=allstars.chh
Attachment #833413 - Attachment is obsolete: true
Attachment #8334166 - Flags: feedback?(allstars.chh)
(Assignee)

Comment 16

4 years ago
Created attachment 8334167 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey
Attachment #833412 - Attachment is obsolete: true
Attachment #8334167 - Flags: feedback?(khuey)
(Assignee)

Comment 17

4 years ago
Created attachment 8334169 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey
Attachment #8334167 - Attachment is obsolete: true
Attachment #8334167 - Flags: feedback?(khuey)
Attachment #8334169 - Flags: feedback?(khuey)
Comment on attachment 8334169 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey

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

::: dom/webidl/MozNfc.webidl
@@ +14,5 @@
> +    * application's manifest URL that is capable of
> +    * handling peer notifications.
> +    *
> +    * Users of this API should have valid permission 'nfc-manager'.
> +    */

Seems for your code, nfc-write permission is also needed.
Comment on attachment 8334166 [details] [diff] [review]
(v1) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome  r=allstars.chh

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

::: dom/system/gonk/Nfc.js
@@ +52,5 @@
>  ];
>  
> +const NFC_IPC_PEER_MSG_NAMES = [
> +  "NFC:RegisterPeerEvent",
> +  "NFC:UnRegisterPeerEvent",

UnregisterPeerEvent

@@ +191,5 @@
> +    _registerPeerTarget: function _registerPeerTarget(msg) {
> +      let appInfo = msg.json;
> +      let targets = this.peerTargetsQueue;
> +
> +      for(let index = 0; index < targets.length; index++) {

nit, space after for.

@@ +210,5 @@
> +      }
> +
> +      // Target not found! Add to the target queue
> +
> +      /**

Use consistent comment type. 
//

@@ +214,5 @@
> +      /**
> +       * Registered PeerInfo target consists of 4 fields
> +       * target : Target to notify the right content for peer notifications
> +       * appId  : The Application that registered for the peerfound / peerlost events
> +       * event  : Possible values are :- 1 , 2 OR 3

remove the '-' char, it looks like '- 1', minus 1.

If you're going to using bitwise operation, use 0x01, 0x02

@@ +216,5 @@
> +       * target : Target to notify the right content for peer notifications
> +       * appId  : The Application that registered for the peerfound / peerlost events
> +       * event  : Possible values are :- 1 , 2 OR 3
> +       *          NFC_PEER_EVENT_FOUND , NFC_PEER_EVENT_LOST OR
> +       *          (NFC_PEER_EVENT_FOUND | NFC_PEER_EVENT_LOST)

When will (NFC_PEER_EVENT_FOUND | NFC_PEER_EVENT_LOST) happen?

And if it does,
how do you maintain the callback map in NfcContentHelper?

@@ +223,5 @@
> +       */
> +      let peerInfo = { target : msg.target,
> +                       appId  : appInfo.appId,
> +                       event  : appInfo.event,
> +                       active : false };

Seems 'active' is used to indicate to notify peerlost or not.
But this boolean is only enabled when peerfound, 
it looks strange to me here,
so each app needs to set onpeerfound, and onpeerlost at the same time?

Can't the app just listens to onpeerlost?
Or should we handle it here, not in NfcContentHelper?

@@ +248,5 @@
> +             return;
> +          }
> +        }
> +      }
> +      if (matchFound && (index < targets.length)) {

Why index < targets.length is needed?

If it's not needed, consider moving declaration of 'index' into for-loop

@@ +300,5 @@
> +          this._unregisterPeerTarget(msg);
> +          return null;
> +        case "NFC:PeerWindow":
> +          // ONLY privileged Content can send this event
> +          if (!msg.target.assertPermission("nfc-manager")) {

PeerWindow needs a nfc-write and a nfc-manager permissions,
can we move the permission check into one place?

@@ +311,5 @@
> +           * that right target associated with this application id can be
> +           * notified of 'PeerFound'.
> +           */
> +          this.notifyPeerFound(msg.json.appId);
> +        break;

The switch here is a mess,
sometimes you use return null, sometimes you use break;

@@ +335,5 @@
>      },
> +
> +    notifyPeerFound: function notifyPeerFound(appId) {
> +      let targets = this.peerTargetsQueue;
> +      for(let index = 0; index < targets.length; index++) {

nit, space after for.

@@ +351,5 @@
> +    },
> +
> +    notifyPeerLost: function notifyPeerLost() {
> +      let targets = this.peerTargetsQueue;
> +      for(let index = 0; i < targets.length; index++) {

ditto

::: dom/system/gonk/NfcContentHelper.js
@@ +226,5 @@
> +    cpmm.sendAsyncMessage("NFC:RegisterPeerEvent", {
> +      appId: appId,
> +      event: event
> +    });
> +    return;

return; ?
Are you missing something here or this is a useless code?

@@ +244,5 @@
> +    cpmm.sendAsyncMessage("NFC:UnRegisterPeerEvent", {
> +      appId: appId,
> +      event: event
> +    });
> +    return;

ditto

@@ +254,5 @@
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +    cpmm.sendAsyncMessage("NFC:PeerWindow", {appId: appId});
> +    return;
> +  },

ditto

::: dom/system/gonk/nfc_consts.js
@@ +47,5 @@
>  this.NFC_TECHS = {
>    0:'NDEF',
>    1:'NDEF_WRITEABLE',
>    2:'NDEF_FORMATABLE',
> +  3:'P2P'

Why modify const?

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +6,5 @@
>  #include "nsIDOMDOMRequest.idl"
>  
>  interface nsIVariant;
>  
> +[scriptable, function,uuid(271f48b0-c884-4f0b-a348-e29824c95168)]

space after ,

@@ +20,5 @@
> +   * @param sessionToken
> +   *        SessionToken received from Chrome
> +   */
> +   void peerNotification(in unsigned long event,
> +                         [optional] in DOMString sessionToken);

Why is this optional?

@@ +27,1 @@
>  [scriptable, uuid(28c8f240-da8c-11e1-9b23-0800200c9a66)]

uuid change.

@@ +38,5 @@
>    nsIDOMDOMRequest makeReadOnlyNDEF(in nsIDOMWindow window, in DOMString sessionToken);
>  
>    nsIDOMDOMRequest connect(in nsIDOMWindow window, in unsigned long techType, in DOMString sessionToken);
>    nsIDOMDOMRequest close(in nsIDOMWindow window, in DOMString sessionToken);
> + /**

Add a extra line

@@ +55,5 @@
> +  *       Callback that is used to notify upper layers whenever PeerEvents happen.
> +  */
> +  void registerTargetForPeerEvent(in nsIDOMWindow window,
> +                                  in unsigned long appId,
> +                                  in unsigned long event,

octet?

@@ +69,5 @@
> +  *
> +  * @param event
> +  *       Event to be unregistered. Either NFC_EVENT_PEER_FOUND or NFC_EVENT_PEER_LOST
> +  */
> +  void unRegisterTargetForPeerEvent(in nsIDOMWindow window,

unregisterTargetForPeerEvent

@@ +73,5 @@
> +  void unRegisterTargetForPeerEvent(in nsIDOMWindow window,
> +                                    in unsigned long appId,
> +                                    in unsigned long event);
> + /**
> +  * Updates the top most window's application id with the Chrome

Updates?
or set?
s/Chrome/Chrome Process/

@@ +79,5 @@
> +  * @param window
> +  *        Current window
> +  *
> +  * @param appId
> +  *        Application ID to be updated with chrome

ditto

@@ +81,5 @@
> +  *
> +  * @param appId
> +  *        Application ID to be updated with chrome
> +  */
> +  void setPeerWindow(in nsIDOMWindow window, in unsigned long appId);

You call it setPeer'Window'
However what you provide here is 'appId'

Please rename you function name.
Attachment #8334166 - Flags: feedback?(allstars.chh) → feedback-
Comment on attachment 8334169 [details] [diff] [review]
(v1) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey

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

::: dom/nfc/nsNfc.js
@@ +23,5 @@
> +                                   "appsService",
> +                                   "@mozilla.org/AppsService;1",
> +                                   "nsIAppsService");
> +
> +let gMozNfc = null;

This does not work.  There is a single global gMozNfc (well, per-process).  If you have two different windows in an app use Nfc this will have the value of the second mozNfc object, and you will only dispatch events to that object.

::: dom/webidl/MozNFCPeer.webidl
@@ +10,5 @@
>  
>  [JSImplementation="@mozilla.org/nfc/NFCPeer;1"]
>  interface MozNFCPeer {
>    DOMRequest sendNDEF(sequence<MozNdefRecord> records);
> +  DOMRequest sendFile(Blob blob);

You should probably just call this 'sendBlob' (or maybe we should just call all of these 'send' and let the WebIDL overloading stuff handle it).

::: dom/webidl/MozNfc.webidl
@@ +15,5 @@
> +    * handling peer notifications.
> +    *
> +    * Users of this API should have valid permission 'nfc-manager'.
> +    */
> +   void setPeerWindow(DOMString manifestUrl);

setPeerWindow should be [ChromeOnly], right?
Attachment #8334169 - Flags: feedback?(khuey) → feedback-
One thing about the onpeerfound,

W3C NFC says "onpeerfound MUST be fired whenever a new NFCPeer is detected by the NFC manager."

But actually Gecko is firing this event when the User touches the Shrinking UI, not when a NFCPeer is detected.

So do we need to rename the callback?

Also the naming setPeerWindow isn't clear, actually it sets the manifest of the app, not the Window.

Comment 22

4 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> One thing about the onpeerfound,
> 
> W3C NFC says "onpeerfound MUST be fired whenever a new NFCPeer is detected
> by the NFC manager."
> 
> But actually Gecko is firing this event when the User touches the Shrinking
> UI, not when a NFCPeer is detected.
> 
> So do we need to rename the callback?

I think it is important from a security perspective to call this callback only when the user has approved it (this is the whole point of having the "shrinking UI"). You make a valid point regarding the semantics as defined by the W3C. How about renaming it "onpeerready"?
(Assignee)

Comment 23

4 years ago
Yes, I think 'onpeerready' is more appropriate.
(Assignee)

Comment 24

4 years ago
Created attachment 8335850 [details] [diff] [review]
System Manager Patch : Integration with Shrink UI and usage of 'validateAppManifestUrl(...)'
(Assignee)

Comment 25

4 years ago
Created attachment 8335858 [details] [diff] [review]
(v2) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey
Attachment #8334169 - Attachment is obsolete: true
Attachment #8335858 - Flags: review?(khuey)
(Assignee)

Comment 26

4 years ago
Created attachment 8335860 [details] [diff] [review]
(v2) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8334166 - Attachment is obsolete: true
Attachment #8335860 - Flags: review?(allstars.chh)
(Assignee)

Comment 27

4 years ago
Kyle, 
Uploaded version 2 of the DOM patch.

Changes between v1 & v2 :

1) Renamed the DOM API from 'setPeerWindow' to 'validateManifestUrl' since the role of this API is to pass the application's URL to Chrome process.
   Chrome process checks to see if the App manifest is registered with it or not

2) Renamed 'onpeerfound' to 'onpeerready'.
3) Added two event listeners .
   a) To send/relay hardware related (Power specific) events to chrome process. This change is necessary for the basic nfc usecases to work. 
      Upon turning NFC ON from settings the nfc_manager dispatches the 'hardware event' with value '1' that is relayed to Chrome process to turn ON NFC chip
   b) To send/relay userResponse on P2P UI (Shrink UI) to the Chrome process so that chrome can notify 'onpeerready' event
4) Fixed your previous patch comments

Resposes to prev comments

> +let gMozNfc = null;
>>> This does not work.  There is a single global gMozNfc (well, per-process).  If you have two different windows in an app use Nfc this will have the value of the second mozNfc object, and you will only >>> dispatch events to that object.

[Ok. Fixed]

> +  DOMRequest sendFile(Blob blob);
>>> You should probably just call this 'sendBlob' (or maybe we should just call all of these 'send' and let the WebIDL overloading stuff handle it.

[Followed W3C spec for NfcPeer. They have defined the interface 'sendFile()' , hence the name]


> +   void setPeerWindow(DOMString manifestUrl);
>>> setPeerWindow should be [ChromeOnly], right?

[This is used by SystemManager (gaia). This is now renamed to 'validateManifestUrl' in v2 of the patch.]
(Assignee)

Comment 28

4 years ago
Yoshi,
Uploaded version 2 of the Chrome patch.

Changes between v1 & v2 :

1) Renamed 'setPeerWindow' to 'validateManifestUrl'. Note that 'validateManifestUrl' permission is checked in Chrome process. ONLY nfc_manager can use this API.
2) Renamed 'peerfound event' to 'peerready event'.
3) Added support to new hardware state values. This is necessary for smooth functioning of Nfc stack. Hence making this change as part of this patch
4) Addressed the integration logic of P2P UI / Shrink UI.
   a) When P2P tech discovered is sent by 'nfcd' to Nfc.js (Chrome process), it dispatches System event to nfc_manager (gaia)
   b) nfc_manager then calls the API , 'validateManifestUrl' with top application's manifest URL. This is passed to Chrome process with its equivalent AppID
   c) Chrome process then checks if it is a valid / registered AppID that can receive 'onpeerready' event. If so, it sends a SUCCESS response, otherwise 'FAILURE'
   d) Upon receiving Success by nfc_manager, it shrinks the UI / shows the P2P UI. 
   e) If User aknowledges on the UI, the response is sent back to Chrome process through a custom event and then to chrome process "NFC:NotifyP2PUserResponse".
   f) Finally chrome, upon further validating the appID (just in case) checks the response . If true (userResponse), then 'onpeerready' of the application is notified
   g) IF user on P2P UI (Step#d ) does aknowledge, then nothing happens as Chrome process only process the IPC message ""NFC:NotifyP2PUserResponse" if the response is 
      'true'
5) Fixed your previous patch comments
(Assignee)

Comment 29

4 years ago
Responses to your previous comments (Others have been taken care of)

> +       * target : Target to notify the right content for peer notifications
> +       * appId  : The Application that registered for the peerfound / peerlost events
> +       * event  : Possible values are :- 1 , 2 OR 3
> +       *          NFC_PEER_EVENT_FOUND , NFC_PEER_EVENT_LOST OR
> +       *          (NFC_PEER_EVENT_FOUND | NFC_PEER_EVENT_LOST)

>>> When will (NFC_PEER_EVENT_FOUND | NFC_PEER_EVENT_LOST) happen?
>>> And if it does,
>>> how do you maintain the callback map in NfcContentHelper?

As per earlier comment# 28, point 4.f, 'NFC_PEER_EVENT_READY' is sent by Chrome process when user has acknowledged on P2P / Shrink UI.
Before notifying this event, the application has to be registered for 'onpeerready' event
 
NfcContentHelper maintains two callbacks atmost (per application / content process)
peerEventsCallbackMap[0x01] and peerEventsCallbackMap[0x02]
0x01 : To notify DOM code for onpeerready
0x02 : To notify DOM code for onpeerlost

However in Nfc.js, a object containing 4 fields
[Target, appId, event (in this case, 0x01 | 0x02 = 0x03) and active] is maintained.


@@ +223,5 @@
> +       */
> +      let peerInfo = { target : msg.target,
> +                       appId  : appInfo.appId,
> +                       event  : appInfo.event,
> +                       active : false };


>>> Seems 'active' is used to indicate to notify peerlost or not.
>>> But this boolean is only enabled when peerfound, 
>>> it looks strange to me here,
>>> so each app needs to set onpeerfound, and onpeerlost at the same time?
>>> Can't the app just listens to onpeerlost?
>>> Or should we handle it here, not in NfcContentHelper?

'active' is used to indicate that ONLY the current target is active (that is about to receive 'onpeerready') among other registered targets.
When notifying 'onpeerready' , one appropriate target will be set 'active'. Since only this target can receive 'onpeerlost' now, hence the flag is used.

>  this.NFC_TECHS = {
>    0:'NDEF',
>    1:'NDEF_WRITEABLE',
>    2:'NDEF_FORMATABLE',
> +  3:'P2P'
>>> Why modify const?

I think this is as per the latest nfcd-gonk specification.

> +        case "NFC:PeerWindow":
> +          // ONLY privileged Content can send this event
> +          if (!msg.target.assertPermission("nfc-manager")) {
>>> PeerWindow needs a nfc-write and a nfc-manager permissions,
>>> can we move the permission check into one place?

Actually 'nfc-manager' is a different than 'nfc-read' and 'nfc-write'.
Moreover 'nfc-write' permissions means that application can do I/O with NFC hardware
Since 'validateManifesetUrl' is an Api that just reads and is special privileged API (that other application should not use),
therefore only 'nfc-manager' is ok. What do you think?
(Assignee)

Comment 30

4 years ago
Created attachment 8335872 [details] [diff] [review]
(v2) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8335860 - Attachment is obsolete: true
Attachment #8335860 - Flags: review?(allstars.chh)
(Assignee)

Updated

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

Comment 31

4 years ago
Also note that 'onpeerlost' will not be called by Chrome process if the application has not registered for 'onpeerready'.
(In reply to Siddartha P from comment #28)
> Yoshi,
> 3) Added support to new hardware state values. This is necessary for smooth
> functioning of Nfc stack. Hence making this change as part of this patch

Is this related to onpeerready?

If no, please remove it from this patch.
Summary: [Gecko] NFC onpeerfound, onpeerlost callbacks → [Gecko] NFC onpeerready, onpeerlost callbacks
(Assignee)

Comment 33

4 years ago
Created attachment 8335882 [details] [diff] [review]
(v2) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

Stripped out 'Hardware state changes' from the patch as per the request
Attachment #8335872 - Attachment is obsolete: true
Attachment #8335872 - Flags: review?(allstars.chh)
Attachment #8335882 - Flags: review?(allstars.chh)
(In reply to Siddartha P from comment #29)
> >  this.NFC_TECHS = {
> >    0:'NDEF',
> >    1:'NDEF_WRITEABLE',
> >    2:'NDEF_FORMATABLE',
> > +  3:'P2P'
> >>> Why modify const?
> 
> I think this is as per the latest nfcd-gonk specification.
> 
Why does nfcd-gonk spec relate to this bug?

> > +        case "NFC:PeerWindow":
> > +          // ONLY privileged Content can send this event
> > +          if (!msg.target.assertPermission("nfc-manager")) {
> >>> PeerWindow needs a nfc-write and a nfc-manager permissions,
> >>> can we move the permission check into one place?
> 
> Actually 'nfc-manager' is a different than 'nfc-read' and 'nfc-write'.
> Moreover 'nfc-write' permissions means that application can do I/O with NFC
> hardware

I know.

> Since 'validateManifesetUrl' is an Api that just reads and is special
> privileged API (that other application should not use),
> therefore only 'nfc-manager' is ok. What do you think?

Please check your Nfc.js line 282, 
I believe the ""NFC:ValidateAppID" is in NFC_IPC_PEER_MSG_NAMES
and line 282 does check nfc-write permission in line 283.
Comment on attachment 8335882 [details] [diff] [review]
(v2) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

::: dom/system/gonk/Nfc.js
@@ +85,5 @@
>      targetsBySessionTokens: {},
>      sessionTokens: [],
>  
> +    // Registered Queue of PeerTargets
> +    peerTargetsQueue: [],

Why not using a dict? {}

@@ +188,5 @@
>          target.sendAsyncMessage(message, options);
>        }
>      },
>  
> +    _registerPeerTarget: function _registerPeerTarget(msg) {

Why prefix with _?
Isn't this function called from Content through IPC?
Also the function name doesn't match the IPC message name.

@@ +192,5 @@
> +    _registerPeerTarget: function _registerPeerTarget(msg) {
> +      let appInfo = msg.json;
> +      let targets = this.peerTargetsQueue;
> +
> +      for (let index = 0; index < targets.length; index++) {

If using a dictionary, we don't have do a for-loop

@@ +197,5 @@
> +        if (targets[index].appId === appInfo.appId) {
> +          /**
> +           * If the application Id matches and one of the events
> +           * (NFC_PEER_EVENT_READY / NFC_PEER_EVENT_LOST) is already registered
> +           */

Consistent comment style. //

@@ +201,5 @@
> +           */
> +          if (targets[index].event & appInfo.event) {
> +            // Already registered target for this event. Do Nothing!
> +            debug("Already registered this target and event ! AppID: " +
> +                             appInfo.appId + " Event: "+ appInfo.event);

Why a if clause but doing nothing?

@@ +309,5 @@
> +          if (!msg.target.assertPermission("nfc-manager")) {
> +            debug("NFC message " + message.name +
> +                  " from a content process with no 'nfc-manager' privileges.");
> +            break;
> +          }

Mentioned here before.

::: dom/system/gonk/NfcContentHelper.js
@@ +63,5 @@
>    this._requestMap = [];
> +  /**
> +   * Maintains an array of PeerEvent related callbacks, mainly
> +   * one for 'peerReady' and another for 'peerLost'.
> +   */

nit, Using // as comment style.

@@ +228,5 @@
> +      event: event
> +    });
> +  },
> +
> +  unRegisterTargetForPeerEvent: function unRegisterTargetForPeerEvent(window,

unregister
I think I mentioned this before.
Comment on attachment 8335882 [details] [diff] [review]
(v2) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

The patch title is a mess.

::: dom/system/gonk/Nfc.js
@@ +54,5 @@
> +const NFC_IPC_PEER_MSG_NAMES = [
> +  "NFC:RegisterPeerEvent",
> +  "NFC:UnregisterPeerEvent",
> +  "NFC:ValidateAppID",
> +  "NFC:NotifyP2PUserResponse"

What's this?
I didn't find anyone called this.

@@ +211,5 @@
> +          return;
> +        }
> +      }
> +
> +      // Target not found! Add to the target queue

not registered yet

@@ +316,5 @@
> +           * should have registered for NFC_PEER_EVENT_READY).
> +           *
> +           * Notify the content process immediately of the status
> +           */
> +           var isRegistered = this.validateAppID(msg.json.appId);

let

@@ +317,5 @@
> +           *
> +           * Notify the content process immediately of the status
> +           */
> +           var isRegistered = this.validateAppID(msg.json.appId);
> +           var status = (isRegistered === true) ? NFC.GECKO_NFC_ERROR_SUCCESS :

remove '=== true'

@@ +355,5 @@
>      sendNfcResponseMessage: function sendNfcResponseMessage(message, data) {
>        this._sendTargetMessage(this.nfc.sessionTokenMap[this.nfc._currentSessionId], message, data);
>      },
> +
> +    validateAppID: function validateAppID(appId) {

If the function will return true or false, it should be named "isXXX"

@@ +370,5 @@
> +      }
> +      return false;
> +    },
> +
> +    notifyPeerReady: function notifyPeerReady(appId) {

I cannot comment on this function and the function below for I don't know what's the notifyP2PUserResponse

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +84,5 @@
> +  *        Application ID to be updated with Chrome process
> +  *
> +  * Returns SUCCESS if registered, else ERROR
> +  */
> +  nsIDOMDOMRequest validateAppManifestUrl(in nsIDOMWindow window, in unsigned long appId);

I didn't find any information for the usage or sample code, or your github.

So I suppose it works like

// now a P2P NFC is discovered 
var request = mozNfc.validateAppManifestUrl(manifest);
request.onsuccess = function() {
  if (request.result) { // request.result should be a boolean
    // launch Shrinking UI
  }
};

If the above is correct, I am wondering if the naming is correct ?

It seems it doesn't do any *validation* on the Chrome Process. It's asking if this manifest is able to process Peer Event.
Attachment #8335882 - Flags: review?(allstars.chh) → review-
Comment on attachment 8335858 [details] [diff] [review]
(v2) Part 1 : Add-DOM-event-handlers-peerfound, peerlost r=khuey

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

::: dom/nfc/nsNfc.js
@@ +212,5 @@
> +    this.__DOM_IMPL__.setEventHandler("onpeerready", handler);
> +    let appId = this._window.document.nodePrincipal.appId;
> +
> +    if (handler === null) {
> +      this._nfcContentHelper.unRegisterTargetForPeerEvent(this._window, appId,

it should be 'unregister...' according to the IDL in Part2.
(Assignee)

Comment 38

4 years ago
Created attachment 8337327 [details] [diff] [review]
(v3) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey
Attachment #8335858 - Attachment is obsolete: true
Attachment #8335858 - Flags: review?(khuey)
Attachment #8337327 - Flags: review?(khuey)
(Assignee)

Comment 39

4 years ago
Created attachment 8337328 [details] [diff] [review]
(v3) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8335882 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 40

4 years ago
Yoshi,
I think I have addressed all your comments in v3 of Chrome patch for 'onpeerready' and 'onpeerlost'.
Differences between v2 & v3 are:
1) Addressed your comments (major one being moving 'peerTargetsQueue' to a dictionary object')
2) Remove 'active' field from 'targetInfo / peerInfo' and introduced 'currentPeerAppId' that keeps an account of currently validated manifestURL passed by nfc_manager. This is valid till the session is lost (tech-lost)
3) Renamed 'validateManifestURL' to 'checkP2PRegistration' all across the stack
4) Added 'notifyP2PUserResponse' interface in nsINfcContentHelper.idl to notify user response on P2P UI from nfc dom code.
(Assignee)

Comment 41

4 years ago
Kyle,
I have uploaded another version (v3) of the Nfc DOM patch.

Differences between v2 & v3 are:
1) Addressed your comments on earlier patch
2) Renamed 'validateManifestURL' (in v1: 'setAppManifestUrl') to 'checkP2PRegistration' in the MozNfc.idl
   Please note that this API is used by nfc_manager. Chrome process upon receiving this message checks for valid 'nfc_manager' permissions.
3) Added event listener for 'nfc-p2p-user-response' to listen on user feedback on P2P UI and relay the information to chrome process through a newly added interface in nsINfcContentHelper.idl to notify user response.
Note: 'sendFile' in MozNFCPeer.webidl naming convention was used on the lines of W3C
(Assignee)

Comment 42

4 years ago
Created attachment 8337329 [details] [diff] [review]
testP2PUI.patch

Incremental patch to earlier posted System-manager-patch to demonstrate how 'checkP2PRegistration' API is called.
Comment on attachment 8337328 [details] [diff] [review]
(v3) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

::: dom/system/gonk/Nfc.js
@@ +101,5 @@
>        for (let msgname of NFC_IPC_MSG_NAMES) {
>          ppmm.addMessageListener(msgname, this);
>        }
> +
> +      for each (let msgname in NFC_IPC_PEER_MSG_NAMES) {

Where is this NFC_IPC_PEER_MSG_NAMES defined?

@@ +182,5 @@
>          target.sendAsyncMessage(message, options);
>        }
>      },
>  
> +    _registerPeerTarget: function _registerPeerTarget(msg) {

Repeat again from Comment 35:

@@ +188,5 @@
>          target.sendAsyncMessage(message, options);
>        }
>      },
>  
> +    _registerPeerTarget: function _registerPeerTarget(msg) {

Why prefix with _?
Isn't this function called from Content through IPC?
Also the function name doesn't match the IPC message name.

-------------------------------------------------------------
If you think they are called through IPC hence should be a private method, then do the same thing for isRegisteredP2PTarget below.

@@ +189,5 @@
> +      let targetInfo = targets[appInfo.appId];
> +      // If the application Id is already registered
> +      if (targetInfo) {
> +        // If the event is not registered
> +        if (targetInfo.event !== appInfo.event) {

nit, if (A && B)

@@ +202,5 @@
> +      // Registered targetInfo target consists of 2 fields (values)
> +      // target : Target to notify the right content for peer notifications
> +      // event  : Possible values are : 0x01 , 0x02 OR 0x03
> +      //          NFC_PEER_EVENT_READY , NFC_PEER_EVENT_LOST OR
> +      //          (NFC_PEER_EVENT_READY | NFC_PEER_EVENT_LOST)

The comment is not correct here.
(NFC_PEER_EVENT_READY | NFC_PEER_EVENT_LOST) should be processed in line 195, 

targetInfo.event |= appInfo.event; 

not here.

In Part 1 you have setter onpeerready and setter onpeerlost,
there's no API could do/register two events at the same time.
Or if it can, then seems unregisterTarget become unneccesary.

@@ +205,5 @@
> +      //          NFC_PEER_EVENT_READY , NFC_PEER_EVENT_LOST OR
> +      //          (NFC_PEER_EVENT_READY | NFC_PEER_EVENT_LOST)
> +      targetInfo = { target : msg.target,
> +                     event  : appInfo.event };
> +      targets[appInfo.appId] = targetInfo;

targets[appInfo.appId] = {...};

I'd say don't reuse the targetInfo here.
When people are looking at this funciton,
if he missed the 'return' statement inside the if clause in line 198.
The whole function looks weird.

@@ +220,5 @@
> +          targets.splice(appInfo.appId, 1);
> +        }
> +        else {
> +          // Otherwise, update the event field ONLY, by removing the event flag
> +          targetInfo.event &= ~appInfo.event;

There should be some check for the events,
if user or app uses a non-existing event type, say 0x0,
then the targetInfo wil never be deleted.

@@ +229,5 @@
> +    isRegisteredP2PTarget: function isRegisteredP2PTarget(appId) {
> +      let targetInfo = this.peerTargetsQueue[appId];
> +      // Check if it is a registered target and capable of receiving
> +      // NFC_PEER_EVENT_READY
> +      return (targetInfo && (targetInfo.event & NFC.NFC_PEER_EVENT_READY));

Nice,
I hope you understand why using map now,
it makes the code much simpler.

However if the app is not registered, the targetInfo will be undefined, then this function will return 'undefined', instead of false, although undefined is considered as falsy value.

The better way is to return boolean

return (targetInfo != null) &&
       (.. & .. !== 0);

@@ +237,5 @@
> +      let targetInfo = this.peerTargetsQueue[appId];
> +      // Check if the application id is registered and capable of receiving
> +      // 'NFC_PEER_EVENT_READY' AND check if the appId to be notified is
> +      // the currentPeerAppId
> +      if (targetInfo && (targetInfo.event & NFC.NFC_PEER_EVENT_READY) &&

calling isRegisteredP2PTarget instead?

@@ +251,5 @@
> +    notifyPeerLost: function notifyPeerLost() {
> +      let targetInfo = this.peerTargetsQueue[this.currentPeerAppId];
> +      // Check if the current application id is registered and capable of receiving
> +      // 'NFC_PEER_EVENT_LOST'
> +      if (targetInfo && (targetInfo.event & NFC.NFC_PEER_EVENT_LOST)) {

ditto

@@ +281,5 @@
>            return null;
>          }
> +      } else if (NFC_IPC_PEER_MSG_NAMES.indexOf(msg.name) != -1) {
> +        if (!msg.target.assertPermission("nfc-write")) {
> +          if (DEBUG) {

If you want to protect the debug function with DEBUG, do it for all, or remove this.

@@ +306,5 @@
> +          this._unregisterPeerTarget(msg);
> +          break;
> +        case "NFC:CheckP2PRegistration":
> +          // ONLY privileged Content can send this event
> +          if (!msg.target.assertPermission("nfc-manager")) {

Comment 19,
can we move the permission check into one place?

@@ +320,5 @@
> +           this.currentPeerAppId = (isRegistered) ? msg.json.appId : null;
> +           let status = (isRegistered) ? NFC.GECKO_NFC_ERROR_SUCCESS :
> +                                         NFC.GECKO_NFC_ERROR_GENERIC_FAILURE;
> +           msg.target.sendAsyncMessage(msg.name + "Response", {status: status,
> +                                               requestId: msg.json.requestId});

nit, alignment on object.

@@ +333,5 @@
> +          // Notify the 'NFC_PEER_EVENT_READY' in case, user has acknowledged
> +          if (msg.json.userResponse === true) {
> +            this.notifyPeerReady(msg.json.appId);
> +          }
> +          // Ignore:User has NOT acknowledged!

Same comment with the IDL if user doesn't accept establishment for the P2P link.

Add break; here

::: dom/system/gonk/NfcContentHelper.js
@@ +236,5 @@
> +    }
> +    let index = this.peerEventsCallbackMap.indexOf(event);
> +    if (index != -1) {
> +      this.peerEventsCallbackMap.splice(index, 1);
> +    }

I am not quite sure what you're trying to do here.

Are you really trying to remove the READY or LOST from the peerEventsCallbackMap?

Then if two apps have called 'registerTargetForPeerEvent' before, now one of them called unregisterTargetForPeerEvent, then the registration for the other app will be deleted too. 
But why?

Or you're trying to delete to callback function from peerEventCallbackMap[READY or LOST]?

@@ +254,5 @@
> +    let requestId = btoa(this.getRequestId(request));
> +    this._requestMap[requestId] = window;
> +
> +    cpmm.sendAsyncMessage("NFC:CheckP2PRegistration", {appId: appId,
> +                                                requestId: requestId});

nit, alignment on the object.

@@ +265,5 @@
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +
> +    cpmm.sendAsyncMessage("NFC:NotifyP2PUserResponse", {appId: appId,
> +                                                 userResponse: response});

ditto

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +82,5 @@
> +  *
> +  * @param appId
> +  *        Application ID to be updated with Chrome process
> +  *
> +  * Returns SUCCESS if registered, else ERROR

returns DOMRequest,
if the appId is registered, onsuccess will be called, ...


One strage thing here if the app is not registered, onerror will be called, instead of the request.result being false.

@@ +99,5 @@
> +  * @param userResponse
> +  *        boolean flag indicating user feedback on P2P UI
> +  *
> +  */
> +  void notifyP2PUserResponse(in nsIDOMWindow window, in unsigned long appId, in boolean userResponse);

Does userResponse mean 'user accepts/denies to establish the P2P link'?

Should we tell gecko the denial case?
Previous meeting you said app won't perceive the user denied it.

If we don't have to notify the denial, remove the userResponse parameter.

If we do, 
s/userResponse/isConfirmed/
or isAccepted since it's a boolean.

Also that goes for the function name,
should it be
notifyUserAcceptedP2P?

You call it userResponse,
when the value is false, I would think user doesn't do any response on that, so a timeout is triggered.
But I don't think that's your use case.
Attachment #8337328 - Flags: review?(allstars.chh) → review-
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #43)
> ::: dom/system/gonk/NfcContentHelper.js
> @@ +236,5 @@
> > +    }
> > +    let index = this.peerEventsCallbackMap.indexOf(event);
> > +    if (index != -1) {
> > +      this.peerEventsCallbackMap.splice(index, 1);
> > +    }
> 
> I am not quite sure what you're trying to do here.
> 
> Are you really trying to remove the READY or LOST from the
> peerEventsCallbackMap?
> 
> Then if two apps have called 'registerTargetForPeerEvent' before, now one of
> them called unregisterTargetForPeerEvent, then the registration for the
> other app will be deleted too. 
> But why?
> 
> Or you're trying to delete to callback function from
> peerEventCallbackMap[READY or LOST]?
> 

My previous comment here should be incorrect. You do want to delete the READY or the LOST from peerEventsCallbackMap.

Please ignore my previous comment here.
Comment on attachment 8337328 [details] [diff] [review]
(v3) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

::: dom/system/gonk/Nfc.js
@@ +225,5 @@
> +        }
> +      }
> +    },
> +
> +    isRegisteredP2PTarget: function isRegisteredP2PTarget(appId) {

Seems this function could be revised a little bit so it can be re-used by notifyPeerReady/Lost

@@ +238,5 @@
> +      // Check if the application id is registered and capable of receiving
> +      // 'NFC_PEER_EVENT_READY' AND check if the appId to be notified is
> +      // the currentPeerAppId
> +      if (targetInfo && (targetInfo.event & NFC.NFC_PEER_EVENT_READY) &&
> +         (appId === this.currentPeerAppId)) {

Under what circumstance will appId differ from this.currentPeerAppId ?

@@ +246,5 @@
> +      }
> +      debug("Application ID : " + appId + " is not a registered target for PeerReady notification");
> +    },
> +
> +

nit, extra line here.

@@ +247,5 @@
> +      debug("Application ID : " + appId + " is not a registered target for PeerReady notification");
> +    },
> +
> +
> +    notifyPeerLost: function notifyPeerLost() {

If the appId in notifyPeerReady is unnecessary, it seems 
we could merge notifyPeerReady/Lost into one function.

::: dom/system/gonk/NfcContentHelper.js
@@ +63,5 @@
>    this._requestMap = [];
> +
> +  // Maintains an array of PeerEvent related callbacks, mainly
> +  // one for 'peerReady' and another for 'peerLost'.
> +  this.peerEventsCallbackMap = [];

The difference between [] and {} is the length property,
if we don't have to use the length, {} seems easier.
Comment on attachment 8337327 [details] [diff] [review]
(v3) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

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

::: dom/nfc/nsNfc.js
@@ +129,5 @@
> +  },
> +
> +  sendFile: function sendFile(blob) {
> +    debug("sendFile is not currently implemented.");
> +    return null;

Consider move it to another bug.
Adding a dummy implementation here is not a good idea.

@@ +158,5 @@
>      debug("mozNfc init called");
>      this._window = aWindow;
> +    var self = this;
> +    this._cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"]
> +                   .getService(Ci.nsISyncMessageSender);

no use right now.
Hi, Sidd
Try to update the title of this bug and the patches.
Right now I can see not only onpeerready/lost are implemented, but also two more APIs are introduced.

Try to narrow the problem down, split into different bugs or parts.
Making the patches larger and larger won't help the developement nor the review process.
(Assignee)

Comment 48

4 years ago
Hi Yoshi,

Thanks for your comments.
For the patch title , I had updated currently this way:
"Bug 933136 - Part 2: Add support to notify 'onpeerready'
 and 'onpeerlost' events from Chrome process

Added two P2P IPC messages 'CheckP2PRegistration' and 'NotifyP2PUserResponse'
to integrate with content process for supporting basic P2P usecases."

I think I have addressed most of your latest comments now.
Regd. the breaking down of patches, I think it would be better if this entire changes are accepted as one change from the usecase perspective. 'NotifyPeerReady' & "NotifyPeerLost' are the final actions taken by Nfc.js . Therefore everything before this needs to be in place for atleast basic/main functionality/feature to work.

So I request you to consider these changes as one change. However I understand your concerns as far as review is concerned. Please note that even testing changes are becoming a huge maintenance challenge for me due to fragmentation of our Nfc patches all over the stack.
(In reply to Siddartha P from comment #48)
> the breaking down of patches, I think it would be better if this
> entire changes are accepted as one change from the usecase perspective.
> 'NotifyPeerReady' & "NotifyPeerLost' are the final actions taken by Nfc.js .
> Therefore everything before this needs to be in place for atleast basic/main
> functionality/feature to work.
> 
> So I request you to consider these changes as one change. However I
> understand your concerns as far as review is concerned. 

Not only review, but how to help you to oraganise/maintain these patches.
This isn't my personal comment, but a common practice in Mozilla.
Please spend some minutes to check other bugs on Buzilla, 
or check some patches that are *NOT* reviewed by me,
when I first submitted patches to mozilla-central, I was also asked to break those patches.

I am not saying you MUST seperate onpeerready/lost, and those two APIs into different parts, I am saying "is it possible to break this into different parts"?
Right now I see two sets of functionalities,
one is called by the nfc app (setting onpeerready/lost)
and the other set is called by nfc_manager.

And the 2nd set, API called by nfc_manager, are NOT still there yet when you first filed this bug. So apparently we found we are still lacking of two APIs when we implement it. But given that we still need those two callbacks we needed in the first place, I'd say it would be simpler if we can continue the 'onpeerready/lost' progress, and start another thread to figure the two needed API calls.

Also when we split into small parts, when people look into bug, they know what's the related work to land this, how many progess we have made so far, ...etc.
Right now there are two patches, one is r- and one is still r?.
People looking into this still have no idea how many remaining work to land this patch.
If there were
Part 2a - onpeerready/lost. r+
PArt 2b - isP2PRregistered, notifyUseResponse. r-
Then we know it's already 50% done, and the remaining has been reviewed at least once, seems remaining work is not huge.

Please do understand that I am *NOT* trying to add more efforts to you, but try to reduce it.
If you think I am doing opposite way to you,
sorry about that.
However I do spend a lot time for reviewing your patch.
But if you think it doesn't worth it,
feel free to find another reviewer to review this. :)

It's great now you already removed the hardware state and the NFC_A consts out of this bug.
So in the future when we discuss where we fixed the Hardware state problem, we could simply say "Bug XXXX, handling Nfc Hardware state", instead of this bug, "NFC-onpeerready".

And if those patches aren't small enough, we will meet some problems when uplifting patches to other branches when we develop in the future.

For example, in the future when we are at 1.4, but somehow we found the bug for the constants for the NFC_A, we have a fix for 1.4, also will have to uplift it into 1.3 branch, but at that time we will figure out we also put this bug(933136) to 1.3 as well,
which we create some unnecessary dependencies here.

> Please note that
> even testing changes are becoming a huge maintenance challenge for me due to
> fragmentation of our Nfc patches all over the stack.

I think that's problem you didn't maintain the patches well,
I am guessing you push all you commits into one branch, and those commits are even not self-contained. for example this commit contains fix for issue A and B, and another commit contains some minor fix for the issue B again, and so on,
so you have problems to divide your patches.

Try to push the related code into same branch, for example, Part 1 code into part1_branch.

Please also check Bugzilla again, many bugs come with more that 5 patches, one example is the Uint8Array problem found by Arno, Bug 933681, which has 8 parts.But seems none saying that there are *too many* patches to maintain.
(Assignee)

Comment 50

4 years ago
Created attachment 8337612 [details] [diff] [review]
(v4) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey
Attachment #8337327 - Attachment is obsolete: true
Attachment #8337327 - Flags: review?(khuey)
Attachment #8337612 - Flags: review?(khuey)
(Assignee)

Comment 51

4 years ago
Created attachment 8337616 [details] [diff] [review]
(v4) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8337328 - Attachment is obsolete: true
Attachment #8337616 - Flags: review?(allstars.chh)
(Assignee)

Comment 52

4 years ago
Yoshi, I will address your latest comment of breaking down the patches. First I will answer your comments thus far,

V4 has been uploaded now.

>> Consider move it to another bug.
>> Adding a dummy implementation here is not a good idea.

Agreed, removed these changes from nsNfc.js

>   let targetInfo = targets[appInfo.appId];
>   // If the application Id is already registered
>   if (targetInfo) {
>     // If the event is not registered
>     if (targetInfo.event !== appInfo.event) {

>>> nit, if (A && B)

We need 4 conditions here.
If (App is already registered) {  --> Condition 1

   if (Event is not registered) { --> Condition 2
   }
   // else part - Event and AppID are registered --> Condition 3

   return;
 }
 // Condition 4
 New Traget - Add

>> targets[appInfo.appId] = {...};
>> I'd say don't reuse the targetInfo here.
Yes, Agreed

>> There should be some check for the events,
 Yes, Agreed. Implemented in v4

>> The better way is to return boolean
>> return (targetInfo != null) &&
>>       (.. & .. !== 0);
  Yes, Agreed. Implemented in v4

>> calling isRegisteredP2PTarget instead?
  Yes for 'notifyPeerReady', I have now used implemented it in v4
  *but, we may not need this in 'notifyPeerLost'. Since 'currentPeerAppId' gives us the already registred appID andit also needs to be checked against 'PeerLost' registration. Iff we have 'peerLost'
 should we notify

>> can we move the permission check into one place?
  Yes, Agreed. Implemented in v4

>> One strage thing here if the app is not registered, onerror will be called, instead of the 
>> request.result being false.
  Actually in NfcContentHelper, we do return status for 'NFC:CheckP2PRegistrationResponse' from handleResponse(...)

>> notifyUserAcceptedP2P?
  Yes, Agreed. Implemented in v4

>> Under what circumstance will appId differ from this.currentPeerAppId ?
  There are couple of usecases. For instance, P2P UI is shown to the user . (Already 'CheckP2PRegistration' with top manifest URL is sent down to Chrome).
  But lets just say, an Incoming call or some other user action kicks out the top application, then it is better for Chrome to have extra check to counter against such usecases. Chrome should not notify wrong application .

>> If the appId in notifyPeerReady is unnecessary, it seems 
>> we could merge notifyPeerReady/Lost into one function.
   Maybe we can. But since these respective event hadlers should be checked for 'PEER_READY' and 'PEER_LOST' respectively, maybe it is better to have them separate. What do you think?

>> The difference between [] and {} is the length property,
>> if we don't have to use the length, {} seems easier.
  I quickly tried this out. It gave me an exception at the usage of 
  'let index = this.peerEventsCallbackMap.indexOf(event);'

I think I have fixed all other comments that you have asked for.
(Assignee)

Comment 53

4 years ago
Yoshi , Here is my current branch setup even to test a basic onpeerready (leave alone other edge cases)
1) We have an ongoing master where we add fixes and few other branches where we add our features into
2) However we have landed initial DOM + Chrome impl ( WebNfc). Now I have to base my patches of this bug and not our 'master branch' or our 'master branch + features' for your review.
3) We also need to strip out Hardware changes (Which I agree we should not make it part of these patches)
   But the complexity is, we are on a certain 'nfcd' that functions in a stable way for H/W state changes. Now reverting back to 'powerconfig' I had to figure out a commit of 'nfcd' that works fine
   Our master & master + features have migrated to H/w state changes already. I need to manually strip them out of my patch that I would give it for your review. Note that in order to test my changes, I need a valid working nfcd. (We go back in time in order to pick stable working commits, which is reasonable)
4) Now there is no Uint8 array merged up until yesterday, I guess. So I will not be able to test even basic 'write' operation from different applications (Note that my patches for you IS not based on 'uint8 array'- Which is what you want ideally). I need to hand-pick changes prior to uint8 array to make a basic Nfc I/O
5) This is only the chrome process side of things. I need to repeat this flip-flop for Dom Nfc and nfc_manager. (note that We cannot merge my changes to nfc_manager to our master as nfc_manager patch is pretty close to landing). Also Shrinking UI has been given to us recently to integrate.
6) We always accommodated integration of P2P UI with 'onpeerready' / 'onpeerlost' and was always part of design. It was not an after thought at all.

However that said, Since you are reviewer, I would leave the decision to you to break the patches further. But I do feel that we have covered most of the usecases for P2P UI , and I have taken care of most of your comments too. Since we are close to a final patch, hence I was asking you to consider it as one patch. Again if you think otherwise, its your call.
Comment on attachment 8337616 [details] [diff] [review]
(v4) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

Mostly looks okay,
Cancelling r? for there are 2 questions to be answered.

1. Why is the check for appId is needed in notifyPeerReady.
2. Should we remove the extra bool from notifyUserAcceptedP2P?

Thanks

::: dom/system/gonk/Nfc.js
@@ +54,5 @@
> +const NFC_IPC_PEER_MSG_NAMES = [
> +  "NFC:RegisterPeerTarget",
> +  "NFC:UnregisterPeerTarget",
> +  "NFC:CheckP2PRegistration",
> +  "NFC:notifyUserAcceptedP2P"

should use Capital N with NotifyUserAcceptedP2P

@@ +85,5 @@
>      targetsBySessionTokens: {},
>      sessionTokens: [],
>  
> +    // Manage registered Peer Targets
> +    peerTargetsQueue: [],

I didn't find out this last time,
it should be {}, and call peerTargetsMap

@@ +192,5 @@
>  
> +    registerPeerTarget: function registerPeerTarget(msg) {
> +      let appInfo = msg.json;
> +      // Sanity check on PeerEvent
> +      if(!this.isValidPeerEvent(appInfo.event)) {

nit, space after if.

@@ +206,5 @@
> +          targetInfo.event |= appInfo.event;
> +        }
> +        // Otherwise event is already registered, return!
> +        return;
> +      }

> We need 4 conditions here.
> If (App is already registered) {  --> Condition 1
> 
>    if (Event is not registered) { --> Condition 2
>    }
>    // else part - Event and AppID are registered --> Condition 3
> 
>    return;
>  }
>  // Condition 4
>  New Traget - Add

From your code we only have to handle condition 1 and condition 2 are all true, we don't have to enumerate 4 conditions here.

However if you'd like to make your code look more consistent with unregisterPeerTarget, it's fine for me.

@@ +220,5 @@
> +
> +    unregisterPeerTarget: function unregisterPeerTarget(msg) {
> +      let appInfo = msg.json;
> +      // Sanity check on PeerEvent
> +      if(!this.isValidPeerEvent(appInfo.event)) {

ditto

@@ +229,5 @@
> +      if (targetInfo) {
> +        // Application Id registered and the event exactly matches.
> +        if (targetInfo.event === appInfo.event) {
> +          // Remove the target from the list of registered targets
> +          targets.splice(appInfo.appId, 1);

If peerTargetsQueue were a map {},
then it would be 
delete targets[appInfo.appId];

@@ +250,5 @@
> +    notifyPeerReady: function notifyPeerReady(appId) {
> +      let targetInfo = this.peerTargetsQueue[appId];
> +      // Check if the application id is a registeredP2PTarget
> +      // AND check if the appId to be notified is the currentPeerAppId
> +      if ((this.isRegisteredP2PTarget(appId)) && (appId === this.currentPeerAppId)) {

> >> Under what circumstance will appId differ from this.currentPeerAppId ?
>   There are couple of usecases. For instance, P2P UI is shown to the user .
> (Already 'CheckP2PRegistration' with top manifest URL is sent down to
> Chrome).
>   But lets just say, an Incoming call or some other user action kicks out
> the top application, then it is better for Chrome to have extra check to
> counter against such usecases. Chrome should not notify wrong application .
> 

But this notifyPeerReady is called by notifyUserAcceptedP2P, which means "User has accepted the P2P link by clicking on the Shrinking UI in nfc_manager", so why will app (nfc_manager) still call notifyUserAccepted with other app(like Caller) ?

@@ +278,5 @@
> +
> +    isValidPeerEvent: function isValidPeerEvent(event) {
> +      if ((event === NFC.NFC_PEER_EVENT_READY) ||
> +          (event === NFC.NFC_PEER_EVENT_LOST)  ||
> +          (event === (NFC.NFC_PEER_EVENT_READY | NFC.NFC_PEER_EVENT_LOST))) {

return (event & (NFC.NFC_PEER_EVENT_READY | NFC.NFC_PEER_EVENT_LOST)) !== 0;

::: dom/system/gonk/NfcContentHelper.js
@@ +235,5 @@
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +    let index = this.peerEventsCallbackMap.indexOf(event);
> +    if (index != -1) {
> +      this.peerEventsCallbackMap.splice(index, 1);

indexOf and splice are methods from Array,
if you're using {} for peerEventsCallbackMap,
then it should be simply

delete this.peerEventsCallbackMap[event];

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +101,5 @@
> +  *
> +  *        If the user has indicated to not accept establishment of P2P link, Chrome Process
> +  *        will simply ignore this event
> +  */
> +  void notifyUserAcceptedP2P(in nsIDOMWindow window, in unsigned long appId, in boolean action);

If we're going to ingore the denial,
seems remove the action parameter is easier,
i.e.
only call this API when user accepts the P2P link.
Attachment #8337616 - Flags: review?(allstars.chh)
(Assignee)

Comment 55

4 years ago
Created attachment 8338067 [details] [diff] [review]
(v5) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey
Attachment #8337612 - Attachment is obsolete: true
Attachment #8337612 - Flags: review?(khuey)
(Assignee)

Updated

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

Comment 56

4 years ago
Created attachment 8338069 [details] [diff] [review]
(v5) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8337616 - Attachment is obsolete: true
Attachment #8338069 - Flags: review?(allstars.chh)
(Assignee)

Comment 57

4 years ago
Kyle, Uploaded latest version of v5 for DOM implementation of event handlers 'onpeerready' 'onpeerlost
The last review happened on v1 of the same patch .
Difference between v5 & v1 :
1) Removed 'sendFile()' API from MozNFCPeer.webidl. This will be added as part of NFC-BT patch in a separate bug
2) Renamed the newly added api in MozNfc.webidl to 'checkP2PRegistration()'. Renamed 'onpeerlost' to 'onpeerready'
3) Addressed your comments
4) Added an event listener "nfc-p2p-user-accept" in Nfc dom code to integrate P2P UI with 'onpeerready' and 'onpeerlost' usecases
(Assignee)

Comment 58

4 years ago
Yoshi, Uploaded v5 of the same patch addressing most of your comments

>> should use Capital N with NotifyUserAcceptedP2P
Done

>> it should be {}, and call peerTargetsMap
Done

>>However if you'd like to make yourcode look more consistent with unregisterPeerTarget,it's fine for me
Ok. Thanks

>> delete targets[appInfo.appId];
Yes agreed, Done.

>> return (event & (NFC.NFC_PEER_EVENT_READY | NFC.NFC_PEER_EVENT_LOST)) !== 0;
This may not completely validate for other usecases such as say, if an event with 'val' 6 or 7 being sent, I think it will still return true. Maybe better to check for explicit values of '0x01' , '0x02' & '0x03'.

>> delete this.peerEventsCallbackMap[event];
Agreed. Done

> +  void notifyUserAcceptedP2P(in nsIDOMWindow window, in unsigned long appId, in boolean action);
>> If we're going to ingore the denial,
>> seems remove the action parameter is easier,
>> i.e.
>> only call this API when user accepts the P2P link

Agreed. Done

Combining the following two questions

>> But this notifyPeerReady is called by notifyUserAcceptedP2P, which means "User has accepted the P2P >> link by clicking on the Shrinking UI in nfc_manager", so why will app (nfc_manager) still call
>> notifyUserAccepted with other app(like Caller) ?
                      
                      AND 

>> 1. Why is the check for appId is needed in notifyPeerReady.

a) When a Nfc enabled FxOs phone is brought closer, system manager (gaia through Chrome process) receives 'P2P-Discovered' notification. 

b) Now nfc_manager.js (system application) retrieves the top manifest Url and passes it down to Chrome process using 'checkP2PRegistration()'. If 'Yay' then onsuccess in nfc_manager is fired. Only then 'P2P UI' / 'Shrink UI' would be shown. Otherwise, no notification of any kind is informed to user.

c) Now lets say, the top app is shrunk. At this point chrome already has the 'appId' of the top-most application. ('this.currentPeerAppId' is Nfc.js)

d) User may accept the P2P (or) Deny it at this point. Lets say after 5 seconds he decides to accept on P2P UI (With the latest change now, we don't even notify Chrome process if user denies. Nfc_manager cancels the P2P UI)

e) When 'notifyUserAcceptedP2P' ('appId' is sent along again by nfc_manager) is sent to the Chrome process, maybe, it is necessary to check the top most appId with 'this.currentPeerAppId' again as 5 seconds have elapsed (Anything could have happened on UI in these 5 secs). Chrome process should not notify 'PeerReady' event just based on 'appId'. Additional check '(appId === this.currentPeerAppId)' ensures that the already registered appId is to whom the 'PeerReady' is being sent.

f) Also 'this.currentPeerAppId' serves another purpose at the time of notifying 'peerLost' event.
Since there is no appId information at the time of sending 'peerLost', 'this.currentPeerAppId' fills the slot nicely. (In earlier patches, targetInfo's 'active' field used to serve the purpose)

    This check ensures that the top-most visible application has to be a registered active target registered on either 'PeerReady' or 'PeerLost' atleast to receive the notification. 

>> 2. Should we remove the extra bool from notifyUserAcceptedP2P?
Agreed. Done
(In reply to Siddartha P from comment #58)
> e) When 'notifyUserAcceptedP2P' ('appId' is sent along again by nfc_manager)
> is sent to the Chrome process, maybe, it is necessary to check the top most
> appId with 'this.currentPeerAppId' again as 5 seconds have elapsed (Anything
> could have happened on UI in these 5 secs). Chrome process should not notify
> 'PeerReady' event just based on 'appId'. Additional check '(appId ===
> this.currentPeerAppId)' ensures that the already registered appId is to whom
> the 'PeerReady' is being sent.
> 
Can you provide some example with how notifyUserAcceptedP2P is called?

00:00 - a NFC app set onpeerready, and another device is in proximity
00:01 - Shrinking UI calls checkP2PRegistration, and this will return true on in onsuccess
00:02 - Shrinking UI is launched.
00:07 - User accepts the P2P link, shrinking-sent event will be notified to nfc_manager, and it will call notifyUserAcceptedP2P with appId from the NFC app at 00:00

When will another app like Caller come into play?

You mean it comes during 00:02 ~ 00:06, and Shrinking UI doesn't know the Caller is going to the foreground now?
So it calls notifyUserAcceptedP2P with Caller's appId?
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #59)
> (In reply to Siddartha P from comment #58)
> > e) When 'notifyUserAcceptedP2P' ('appId' is sent along again by nfc_manager)
> > is sent to the Chrome process, maybe, it is necessary to check the top most
> > appId with 'this.currentPeerAppId' again as 5 seconds have elapsed (Anything
> > could have happened on UI in these 5 secs). Chrome process should not notify
> > 'PeerReady' event just based on 'appId'. Additional check '(appId ===
> > this.currentPeerAppId)' ensures that the already registered appId is to whom
> > the 'PeerReady' is being sent.
> > 
> Can you provide some example with how notifyUserAcceptedP2P is called?
> 
> 00:00 - a NFC app set onpeerready, and another device is in proximity
> 00:01 - Shrinking UI calls checkP2PRegistration, and this will return true
> on in onsuccess
> 00:02 - Shrinking UI is launched.
> 00:07 - User accepts the P2P link, shrinking-sent event will be notified to
> nfc_manager, and it will call notifyUserAcceptedP2P with appId from the NFC
> app at 00:00
> 
> When will another app like Caller come into play?
> 
> You mean it comes during 00:02 ~ 00:06, and Shrinking UI doesn't know the
> Caller is going to the foreground now?
> So it calls notifyUserAcceptedP2P with Caller's appId?

Just discussed with Alive,
He said this should be handled by ShrinkUI itself but he hasn't tested that, maybe we should file a new bug to discuss this special case.

So the appId check is unnecessary here, please remove it for now.
And as I said before, try to merge to notifyPeerReady/Lost functions.
(Assignee)

Comment 61

4 years ago
Created attachment 8338278 [details] [diff] [review]
(v6) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

v6 version of the patch is uploaded. [Removed AppId check in 'notifyPeerReady' as per Yoshi's comments]
Attachment #8338069 - Attachment is obsolete: true
Attachment #8338069 - Flags: review?(allstars.chh)
Attachment #8338278 - Flags: review?(allstars.chh)
Comment on attachment 8338278 [details] [diff] [review]
(v6) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

Cancelling r? to see if we can reduce some functions from Nfc.js.

::: dom/system/gonk/Nfc.js
@@ +108,5 @@
>        for (let msgname of NFC_IPC_MSG_NAMES) {
>          ppmm.addMessageListener(msgname, this);
>        }
> +
> +      for each (let msgname in NFC_IPC_PEER_MSG_NAMES) {

Sorry I didn't spot it until now.
For array, please use for of

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

@@ +119,5 @@
>        for (let msgname of NFC_IPC_MSG_NAMES) {
>          ppmm.removeMessageListener(msgname, this);
>        }
> +
> +      for each (let msgname in NFC_IPC_PEER_MSG_NAMES) {

ditto

@@ +244,5 @@
> +      // Check if it is a registered target and capable of receiving
> +      // NFC_PEER_EVENT_READY
> +      return ((targetInfo != null) &&
> +              (targetInfo.event & NFC.NFC_PEER_EVENT_READY !== 0));
> +    },

How do you think if we add another event argument for this?

isRegisteredP2PTarget: function isRegisteredP2PTarget(appId, event) {
  let targetInfo = this.peerTargetsMap[appId];
  return ((targetInfo != null) &&
          (targetInfo.event & event !== 0));
}

@@ +259,5 @@
> +      }
> +      debug("Application ID : " + appId + " is not a registered target for PeerReady notification");
> +    },
> +
> +    notifyPeerLost: function notifyPeerLost() {

Also add another argument appId here

notifyPeerLost: function notifyPeerLost(appId) {
...
},

So right now notifyPeerReady and notifyPeerLost are similar now, except the event is different,

we can simplify it to

notifyPeerEvent: notifyPeerEvent(appId, event) {

};

@@ +275,5 @@
> +      debug("No active registered target for PeerLost notification");
> +    },
> +
> +    isValidPeerEvent: function isValidPeerEvent(event) {
> +      if ((event === NFC.NFC_PEER_EVENT_READY) ||

return (...);

@@ +280,5 @@
> +          (event === NFC.NFC_PEER_EVENT_LOST)  ||
> +          (event === (NFC.NFC_PEER_EVENT_READY | NFC.NFC_PEER_EVENT_LOST))) {
> +            // Valid values : 0x01, 0x02 Or 0x03
> +            return true;
> +          }

nit ,indent.

@@ +491,3 @@
>          gSystemMessenger.broadcastMessage("nfc-manager-tech-lost", message);
> +        // Notify 'PeerLost' to appropriate registered target, if any
> +        gMessageManager.notifyPeerLost();

gMessageManager.notifyPeerLost(this.currentPeerAppId);
Attachment #8338278 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8338067 - Flags: superreview?(bugs)
Comment on attachment 8338067 [details] [diff] [review]
(v5) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

>   init: function init(aWindow) {
>     debug("mozNfc init called");
>     this._window = aWindow;
>+    var self = this;
>+
>+    this._window.addEventListener("nfc-p2p-user-accept", function (event) {
What is this? What if the script running on the page adds events listener to the window
before this code is run and calls stopImmediatePropagation(), then this handler won't be called at all.


>+  set onpeerready(handler) {
>+    this.__DOM_IMPL__.setEventHandler("onpeerready", handler);
>+    let appId = this._window.document.nodePrincipal.appId;
>+
>+    if (handler === null) {
>+      this._nfcContentHelper.unregisterTargetForPeerEvent(this._window, appId,
>+                                                        NFC_PEER_EVENT_READY);
>+    } else {
>+      var self = this;
>+      this._nfcContentHelper.registerTargetForPeerEvent(this._window, appId,
>+        NFC_PEER_EVENT_READY, function(evt, sessionToken) {
>+          self.session = sessionToken;
>+          self.firePeerEvent(evt, sessionToken, handler);
>+      });
>+    }
So setting onpeerready = function() {} has a side effect, but
.addEventListener("peerready", function() {}) doesn't have?
That is unexpected, and I believe MessagePort is the only other place in the web platform where 
there onfoo handler has such side effect (and I see that behavior a bug in MessagePort).

I'd prefer having some kind of start() method, or could registerTargetForPeerEvent be called (asynchronously perhaps) when mozNFC is called?

>+  set onpeerlost(handler) {
>+    this.__DOM_IMPL__.setEventHandler("onpeerlost", handler);
>+    let appId = this._window.document.nodePrincipal.appId;
>+
>+    if (handler === null) {
>+      this._nfcContentHelper.unregisterTargetForPeerEvent(this._window, appId,
>+                                                         NFC_PEER_EVENT_LOST);
>+    } else {
>+      var self = this;
>+      this._nfcContentHelper.registerTargetForPeerEvent(this._window, appId,
>+        NFC_PEER_EVENT_LOST, function(evt, sessionToken) {
>+          self.session = sessionToken;
>+          self.firePeerEvent(evt, sessionToken, handler);
>+      });
>+    }
ditto

>+    * Returns success if given maifestUrl
manifestUrl
Attachment #8338067 - Flags: superreview?(bugs) → superreview-
(Reporter)

Updated

4 years ago
Depends on: 860910
(Assignee)

Updated

4 years ago
Depends on: 943613
(Assignee)

Comment 64

4 years ago
Created attachment 8338952 [details] [diff] [review]
(v7) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

v7 of Chrome patch - Taking care of latest Yoshi's comments
Attachment #8338278 - Attachment is obsolete: true
Attachment #8338952 - Flags: review?(allstars.chh)
(Assignee)

Comment 65

4 years ago
> How do you think if we add another event argument for this?
> isRegisteredP2PTarget: function isRegisteredP2PTarget(appId, event) 
Done

> So right now notifyPeerReady and notifyPeerLost are similar now, except the event is different,
> we can simplify it to
Nice! Done

Other comments have been taken care too
Comment on attachment 8338952 [details] [diff] [review]
(v7) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

::: dom/system/gonk/Nfc.js
@@ +473,3 @@
>          gSystemMessenger.broadcastMessage("nfc-manager-tech-lost", message);
> +        // Notify 'PeerLost' to appropriate registered target, if any
> +        gMessageManager.notifyPeerEvent(msg.json.appId, NFC.NFC_PEER_EVENT_LOST);

Where does this appId come from? worker?
Attachment #8338952 - Flags: review?(allstars.chh) → review-
(Assignee)

Comment 67

4 years ago
Created attachment 8339105 [details] [diff] [review]
(v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8338952 - Attachment is obsolete: true
Attachment #8339105 - Flags: review?(allstars.chh)
Comment on attachment 8338952 [details] [diff] [review]
(v7) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

with another defect I found.

::: dom/system/gonk/Nfc.js
@@ +241,5 @@
> +
> +    isRegisteredP2PTarget: function isRegisteredP2PTarget(appId, event) {
> +      let targetInfo = this.peerTargetsMap[appId];
> +      // Check if it is a registered target for the 'event'
> +      return ((this.isValidPeerEvent(event)) && (targetInfo != null) &&

checking for event is unnecessary.

1. isRegisteredP2PTarget is called by notifyPeerEvent, which in turned called by NfcContentHelper, nsNfc.js.

2. The function name says "this function will check if the appId is registered with the event", doesn't say the "will check the event is valid or not".
Try to make your function simpler, do it one thing and do it well.

If you still think you'd need to check the event, do it in notifyPeerEvent, not inside this function.
Attachment #8338952 - Attachment is obsolete: false
(Assignee)

Comment 69

4 years ago
Yoshi, I think it is better to check for the validity of the event even in isRegisteredP2PTarget(..)
It is a simple sanity check and nothing more than that.

In notifyPeerEvent, we have another sanity check for the event as they are two different
(Assignee)

Updated

4 years ago
Attachment #8338952 - Attachment is obsolete: true
(In reply to Siddartha P from comment #69)
> Yoshi, I think it is better to check for the validity of the event even in
> isRegisteredP2PTarget(..)

So isRegisteredP2PTarget will return false when
1. the event is invalid.
2. the target is not registed.

The function name doesn't match what it tried to do here.
As the name told me "check if the target is registered with that event", not "is the event valid".

You could 
1. Make the function more complicated, more powerful to do more things, and people reading your code will spend more time to figure out what it's doing and check if it is doing correctly.

2. make it easier to let everyone understand easily.

I am open if you want to add more functionalities to your function, in that case, please update the naming first, and I will restart the review all over again.

Or you could make it easier I could just review what you've updated since last version.

Please understand that I have to review the whole patch again if you try to add something in the last minute.
Comment on attachment 8339105 [details] [diff] [review]
(v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

Cancelling r? for the improper check the events.

::: dom/system/gonk/Nfc.js
@@ +194,5 @@
> +      let appInfo = msg.json;
> +      // Sanity check on PeerEvent
> +      if (!this.isValidPeerEvent(appInfo.event)) {
> +        return;
> +      }

Since it's just an integer check, cannot we do this in content process ?

@@ +222,5 @@
> +      let appInfo = msg.json;
> +      // Sanity check on PeerEvent
> +      if (!this.isValidPeerEvent(appInfo.event)) {
> +        return;
> +      }

ditto

@@ +245,5 @@
> +      return ((this.isValidPeerEvent(event)) && (targetInfo != null) &&
> +                                     (targetInfo.event & event !== 0));
> +    },
> +
> +    notifyPeerEvent: function notifyPeerEvent(appId, event) {

So the event check is inside isRegisteredP2PTarget, not inside notifyPeerEvent, the event argument comes in this function, why doesn't this function have to check it first, but the sub-function has to?

@@ +325,5 @@
> +        case "NFC:CheckP2PRegistration":
> +           // Check if the application id is a valid registered target.
> +           // (It should have registered for NFC_PEER_EVENT_READY).
> +           let isRegistered = this.isRegisteredP2PTarget(msg.json.appId,
> +                                              NFC.NFC_PEER_EVENT_READY);

nit, alignment

::: dom/system/gonk/NfcContentHelper.js
@@ +327,5 @@
> +      case "NFC:PeerEvent":
> +        let callback = this.peerEventsCallbackMap[message.json.event];
> +        if (callback) {
> +           callback.peerNotification(message.json.event,
> +                             message.json.sessionToken);

nit, alignment
Attachment #8339105 - Flags: review?(allstars.chh)
(Assignee)

Comment 72

4 years ago
Created attachment 8339158 [details] [diff] [review]
(v6) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

In v6 of the DOM patch, added two new Chrome Only interfaces as suggested and addressed other comments
Attachment #8338067 - Attachment is obsolete: true
Attachment #8338067 - Flags: review?(khuey)
Attachment #8339158 - Flags: superreview?(bugs)
Attachment #8339158 - Flags: review?(khuey)
(Assignee)

Comment 73

4 years ago
Created attachment 8339216 [details] [diff] [review]
(v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

Uploaded latest version . As discussed over IRC, the validity checks on event are retained in Chrome process only. Thx
Attachment #8339105 - Attachment is obsolete: true
Attachment #8339216 - Flags: review?(allstars.chh)
Comment on attachment 8339216 [details] [diff] [review]
(v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

I don't see my previous comments has been addressed.


@@ +325,5 @@
> +        case "NFC:CheckP2PRegistration":
> +           // Check if the application id is a valid registered target.
> +           // (It should have registered for NFC_PEER_EVENT_READY).
> +           let isRegistered = this.isRegisteredP2PTarget(msg.json.appId,
> +                                              NFC.NFC_PEER_EVENT_READY);

nit, alignment

::: dom/system/gonk/NfcContentHelper.js
@@ +327,5 @@
> +      case "NFC:PeerEvent":
> +        let callback = this.peerEventsCallbackMap[message.json.event];
> +        if (callback) {
> +           callback.peerNotification(message.json.event,
> +                             message.json.sessionToken);

nit, alignment
Attachment #8339216 - Flags: review?(allstars.chh)
Comment on attachment 8339216 [details] [diff] [review]
(v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh

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

except the previous comments are not addressed,
others are okay to me.
Attachment #8339216 - Flags: review+
(Assignee)

Comment 76

4 years ago
Created attachment 8339395 [details] [diff] [review]
(v6b) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

Renamed the interfaces to 'eventListenerWasAdded' and 'eventListenerWasRemoved'as per the latest patch
Attachment #8339158 - Attachment is obsolete: true
Attachment #8339158 - Flags: superreview?(bugs)
Attachment #8339158 - Flags: review?(khuey)
Attachment #8339395 - Flags: superreview?(bugs)
Attachment #8339395 - Flags: review?(khuey)
Comment on attachment 8339395 [details] [diff] [review]
(v6b) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

And FYI to kheuy that
+   [ChromeOnly]
+   void eventListenerWasAdded(DOMString aType);
+   [ChromeOnly]
+   void eventListenerWasRemoved(DOMString aType);
is the way js implemented webidl can get notified these days when a listener is added or removed.
Attachment #8339395 - Flags: superreview?(bugs) → superreview+
NFC is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
Comment on attachment 8339395 [details] [diff] [review]
(v6b) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey

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

We're getting close.  I only found one thing to complain about.

::: dom/nfc/nsNfc.js
@@ +216,5 @@
> +  eventListenerWasAdded: function(evt) {
> +    let event = (evt === 'peerready') ? NFC_PEER_EVENT_READY :
> +                                        NFC_PEER_EVENT_LOST;
> +    this.registerTarget(event);
> +  },

See below.

@@ +222,5 @@
> +  eventListenerWasRemoved: function(evt) {
> +    let event = (evt === 'peerready') ? NFC_PEER_EVENT_READY :
> +                                        NFC_PEER_EVENT_LOST;
> +    this.unregisterTarget(event);
> +  },

eventListenerWasAdded/Removed can get called with any argument, not just peerready/peerlost.

If I do

window.navigator.mozNfc.removeEventListener("foopy")

eventListenerWasRemoved is going to get called with the argument "foopy", and you're going to unregisterTarget(NFC_PEER_EVENT_LOST).
Attachment #8339395 - Flags: review?(khuey) → review-
(Assignee)

Comment 80

4 years ago
Created attachment 8341402 [details] [diff] [review]
(v7) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug
Attachment #8339395 - Attachment is obsolete: true
Attachment #8341402 - Flags: superreview?(bugs)
Attachment #8341402 - Flags: review?(khuey)
Comment on attachment 8341402 [details] [diff] [review]
(v7) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug

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

Consider add/removeEventListener("toString");
Attachment #8341402 - Flags: superreview?(bugs)
Attachment #8341402 - Flags: review?(khuey)
Attachment #8341402 - Flags: review-
(Assignee)

Comment 82

4 years ago
Created attachment 8341428 [details] [diff] [review]
(v8) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug
Attachment #8341402 - Attachment is obsolete: true
Attachment #8341428 - Flags: superreview?(bugs)
Attachment #8341428 - Flags: review?(khuey)
Comment on attachment 8341428 [details] [diff] [review]
(v8) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug

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

Olli doesn't need to review this again.

::: dom/nfc/nsNfc.js
@@ +214,5 @@
> +
> +  eventListenerWasAdded: function(evt) {
> +    if (this.getEventType(evt) !== -1) {
> +      this.registerTarget(this.getEventType(evt));
> +    }

I would do:

let eventType = this.getEventType(evt);
if (eventType == -1)
  return;
this.registerTarget(eventType);

but this is nit-picking.
Attachment #8341428 - Flags: superreview?(bugs)
Attachment #8341428 - Flags: review?(khuey)
Attachment #8341428 - Flags: review+
(Assignee)

Comment 84

4 years ago
Created attachment 8341440 [details] [diff] [review]
(v8b) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug

Fixed nits as per latest comments
Attachment #8341428 - Attachment is obsolete: true
Sidd met a problem in running try-server,
filed Bug 945610 for this.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #74)
> Comment on attachment 8339216 [details] [diff] [review]
> (v8) Part 2 : Add support to notify PeerLost and PeerFound notifications in
> Chrome r=allstars.chh
> 
> Review of attachment 8339216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see my previous comments has been addressed.
> 
> 
> @@ +325,5 @@
> > +        case "NFC:CheckP2PRegistration":
> > +           // Check if the application id is a valid registered target.
> > +           // (It should have registered for NFC_PEER_EVENT_READY).
> > +           let isRegistered = this.isRegisteredP2PTarget(msg.json.appId,
> > +                                              NFC.NFC_PEER_EVENT_READY);
> 
> nit, alignment
> 
> ::: dom/system/gonk/NfcContentHelper.js
> @@ +327,5 @@
> > +      case "NFC:PeerEvent":
> > +        let callback = this.peerEventsCallbackMap[message.json.event];
> > +        if (callback) {
> > +           callback.peerNotification(message.json.event,
> > +                             message.json.sessionToken);
> 
> nit, alignment

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #71)
> @@ +325,5 @@
> > +        case "NFC:CheckP2PRegistration":
> > +           // Check if the application id is a valid registered target.
> > +           // (It should have registered for NFC_PEER_EVENT_READY).
> > +           let isRegistered = this.isRegisteredP2PTarget(msg.json.appId,
> > +                                              NFC.NFC_PEER_EVENT_READY);
> 
> nit, alignment
> 
> ::: dom/system/gonk/NfcContentHelper.js
> @@ +327,5 @@
> > +      case "NFC:PeerEvent":
> > +        let callback = this.peerEventsCallbackMap[message.json.event];
> > +        if (callback) {
> > +           callback.peerNotification(message.json.event,
> > +                             message.json.sessionToken);
> 
> nit, alignment

Hi Sidd
Checking your patch again I still found the nits are not addressed yet.
I think this is the 3rd time I ask for this.

Please specify why you didn't address those nits.
Otherwise I will cancel the r+ first to check is there any other comment you didn't address.

Thanks
Flags: needinfo?(psiddh)
Comment on attachment 8341440 [details] [diff] [review]
(v8b) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug

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

::: dom/webidl/MozNfc.webidl
@@ +16,5 @@
> +    *
> +    * Returns success if given manifestUrl is registered for 'onpeerready',
> +    * otherwise error
> +    *
> +    * Users of this API should have valid permission 'nfc-manager'.

From Comment 34,
this API uses not only nfc-manager but also nfc-write permission.
(Assignee)

Comment 88

4 years ago
Hi Yoshi,

Between these versions , I did take care of your comments at the specified points. (only partly) 
https://bug933136.bugzilla.mozilla.org/attachment.cgi?id=8339216  (v8)

https://bug933136.bugzilla.mozilla.org/attachment.cgi?id=8338952 (v7)

I only fixed the alignment part and thought I have addressed your comments on nits completely.
However I also had to take care of left-justification as you have been pointing out. Sorry for over-looking this part.
Flags: needinfo?(psiddh)
(Assignee)

Comment 89

4 years ago
Created attachment 8341966 [details] [diff] [review]
(v8c) Part 1 : Add DOM event handlers 'onpeerready' 'onpeerlost'  r=khuey, sr=smaug
Attachment #8341440 - Attachment is obsolete: true
(Assignee)

Comment 90

4 years ago
Created attachment 8341968 [details] [diff] [review]
(v8b) Part 2 : Add support to notify PeerLost and PeerFound notifications in Chrome r=allstars.chh
Attachment #8339216 - Attachment is obsolete: true
(Assignee)

Comment 91

4 years ago
Latest tryResults of the patches : https://tbpl.mozilla.org/?tree=Try&rev=2ca8af7875c5
(Assignee)

Comment 92

4 years ago
There are couple of exceptions and may not be related to the current patches.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/404b5d26f2de
https://hg.mozilla.org/integration/b2g-inbound/rev/2a7e01f13840
Keywords: checkin-needed
Just draw a simple diagram for the API introduces in this bug,
feel free to corret it if anything is wrong.
https://hg.mozilla.org/mozilla-central/rev/404b5d26f2de
https://hg.mozilla.org/mozilla-central/rev/2a7e01f13840
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
NFC isn't a committed feature, so clearing nom.
blocking-b2g: 1.3? → ---
Blocks: 1034474
You need to log in before you can comment on or make changes to this bug.