991970 - [B2G][NFC] Support W3C compliant ontagfound, ontaglost

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Garner Lee]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=963533#c5

Investigate and create W3C compliant "ontagfound"/"ontaglost" Currently, they are "nfc-ndef-discovered" and "nfc-tag-discovered" and "nfc-tech-lost" activities.

A new system message ("Activity picker" that instead invokes the on<event> callback) and support for getting NFC interface objects in the argument list is needed for this to happen.

Comment 1

[Garner Lee]

Related to: 963533

Comment 2

[Garner Lee]

Attachment: WIP: Gecko DOM changes for events.

Comment 3

[Garner Lee]

Attachment: 0001-Bug-991970-ontagfound-ontaglost-changes.-Can-t-pass-.patch

WIP Gaia change: ontagfound with unimplemented ndefmessage.

Comment 4

[Garner Lee]

Perhaps a obvious note: the manifest.webapp specifies the accepted activities, but instead of the registered callback for 'activity', ontagfound/ontaglost will be called instead.

Ignoring implementation, the proposed change for compliance is going to be somewhat unnatural, when it works as designed.

Feedback is welcome.

Comment 5

[Garner Lee]

Attachment: WIP: Gecko DOM changes for events (v1.1)

Comment 6

[Garner Lee]

Attachment: WIP: Gaia changes. Non-queued events (v1.1)

Comment 7

[Garner Lee]

Attachment: NFC-Demo updates using new API

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Why does this depend on Bug 1075198?
In ontagfound the parameter should be type of MozNFCTag, not Uint8Array.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Garner

This bug has been filed for a while and no recently update, almost 4 months.

Like onpeerfound, we will handle 'how to dispatch event to foregound' to Bug 1082440.
So in this bug you can just focus on the 'dispatch ontagfound with a MozNFCTagEvent'.

This bug is blocking our plan (Bug 1042851) now also it blocks our following work on MozNFCTag object.

Given it already takes 4 months,
Please update the status here and provide ETA to finish this.
If you don't have any progress, then I'll take this bug because of priority.

Thanks

Comment 10

[Garner Lee]

I'll take another look. The main issue that remains unsolved is the whole "declare a system message/activity handler" in manifest, and then declare another event handler to handle the tag data/event object. It'll work (as in the outdated WIP), just not really good solution. On the other hand, it can solved in a separate bug, and example code supplied until the original callback is simply redundant.

For the event handler, it should also contain data as a field (Uint8Array, which isn't supported by the event object code generator), unless we want the data to arrive separately in the message handler callback. I workaround is to just directly implement the event ourselves. This seems to be discouraged in new code.



(In reply to Yoshi Huang[:allstars.chh] from comment #9)
> Hi Garner
> 
> This bug has been filed for a while and no recently update, almost 4 months.
> 
> Like onpeerfound, we will handle 'how to dispatch event to foregound' to Bug
> 1082440.
> So in this bug you can just focus on the 'dispatch ontagfound with a
> MozNFCTagEvent'.
> 
> This bug is blocking our plan (Bug 1042851) now also it blocks our following
> work on MozNFCTag object.
> 
> Given it already takes 4 months,
> Please update the status here and provide ETA to finish this.
> If you don't have any progress, then I'll take this bug because of priority.
> 
> Thanks

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Garner Lee from comment #10)
> On the other hand, it can
> solved in a separate bug, 
File anothe bug to address your concern.
 
> For the event handler, it should also contain data as a field (Uint8Array,
> which isn't supported by the event object code generator), unless we want
> the data to arrive separately in the message handler callback. I workaround
> is to just directly implement the event ourselves. This seems to be
> discouraged in new code.

I think you mean to contain the MozNDEFRecord, not just the payload(Uint8Array)
I will try if I can add MozNDEFRecord in thie bug,
So in the ontagfound callback it will have evt.tag for the NFCTag object and evt.ndef for the NDEF.

Also I still didn't see new WIP patch on this, I'll take this one.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Part 1: indent to 2 spaces.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Part 2: implement ontagfound/lost

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: indent to 2 spaces.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: implement ontagfound/lost

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8510161 [details] [diff] [review]
Part 2: implement ontagfound/lost

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

::: dom/nfc/nsINfcContentHelper.idl
@@ +41,5 @@
>     *
>     * @param sessionToken
>     *        SessionToken received from Chrome process
>     */
> +  void notifyPeerReady(in DOMString sessionToken);

Here and Below I just changed the indent from 3 spaces to 2 spaces.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I have problems running marionette-webapi now, so I'd complete the test case in Bug 1087928.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: implement ontagfound/lost

Comment 19

[Dimi Lee [:dimi]]

Comment on attachment 8510227 [details] [diff] [review]
Part 2: implement ontagfound/lost

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

::: dom/nfc/gonk/Nfc.js
@@ +562,5 @@
>          }
>  
>          SessionHelper.unregisterSession(message.sessionId);
>          // Do not expose the actual session to the content
>          delete message.sessionId;

If we do not want to expose sessionID to content, maybe we should move this before onPeerLost/onTagLost

::: dom/nfc/nsINfcContentHelper.idl
@@ +72,5 @@
>    nsIDOMDOMRequest connect(in nsIDOMWindow window, in unsigned long techType, in DOMString sessionToken);
>    nsIDOMDOMRequest close(in nsIDOMWindow window, in DOMString sessionToken);
>  
> +  /**
> +   * Initiate Send file operation

Nits. send

::: dom/nfc/nsNfc.js
@@ +200,5 @@
> +
> +  set ontaglost(handler) {
> +    this.__DOM_IMPL__.setEventHandler("ontaglost", handler);
> +  },
> +

I would suggest another coding style because we will have many NFC events (taglost,tagfound,peerfound,peerready,peerlost).

init: function init(aWindow) {
  this.defineEventHandlerGetterSetter("onpeerfound");
  this.defineEventHandlerGetterSetter("onpeerready");  
  this.defineEventHandlerGetterSetter("onpeerlost");
  this.defineEventHandlerGetterSetter("ontagfound");
  this.defineEventHandlerGetterSetter("ontaglost");
},

defineEventHandlerGetterSetter: function(name) {
  Object.defineProperty(this, name, {
    get: function() {
      return this.__DOM_IMPL__.getEventHandler(name);
    },
    set: function(handler) {
      this.__DOM_IMPL__.setEventHandler(name, handler);
    }
  });
},

::: dom/webidl/MozNFC.webidl
@@ +75,5 @@
> +
> +  /**
> +   * Ths event will be fired when a NFCTag is detected.
> +   */
> +  attribute EventHandler ontagfound;

ontagfound will get MozNFCTagEvent with tag data, should we add
[CheckPermissions="nfc-read"] here ?

@@ +80,5 @@
> +
> +  /**
> +   * This event will be fired if the tag detected in ontagfound has been removed.
> +   */
> +  attribute EventHandler ontaglost;

I don't understand why onpeerlost require nfc-write permission.
So I am a little bit confused if we also need permission check in ontaglost EventHandler

::: dom/webidl/MozNFCTagEvent.webidl
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor(DOMString type, optional MozNFCTagEventInit eventInitDict),
> + Func="Navigator::HasNFCSupport", AvailableIn="CertifiedApps"]

require nfc-write & nfc-read ?

@@ +24,5 @@
> +{
> +  MozNFCTag? tag = null;
> +
> +  sequence<MozNDEFRecord>? ndefRecords = [];
> +};

MozNFCTagEvent should be added to test_all_synthetic_events.html

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: implement ontagfound/lost v2.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8511832 [details] [diff] [review]
Part 2: implement ontagfound/lost v2.

Add r? to smaug for requesting review for:
1. WebIDL changed MozNFC.webidl, and MozNFCTagEvent.webidl.

2. IDL changed in nsINfcContentHelper.idl: the techList in nsINfcTagEvent actually is an array of NFCTechType defined in dom/webidl/MozNFCTag.webidl, I use nsIVariant to represent this.

3. modifying test case in dom/events/test/test_all_synthetic_events.html since I added a MozNFCTagEvent.

Thanks

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8510160 [details] [diff] [review]
Part 1: indent to 2 spaces.

fixed indent to 2 spaces.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8511832 [details] [diff] [review]
Part 2: implement ontagfound/lost v2.

I wouldn't talk about Chrome process, but either chrome process or parent process. On IRC people said they use 'parent process'.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: implement ontagfound/lost v3

s/Chrome/parent/g

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: add permission check

Add permission check based on the comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1003268#c38

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0aa93b680d6b

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c74213e2f68f
https://hg.mozilla.org/integration/b2g-inbound/rev/66002b73a68b
https://hg.mozilla.org/integration/b2g-inbound/rev/627bfb5807ea

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c74213e2f68f
https://hg.mozilla.org/mozilla-central/rev/66002b73a68b
https://hg.mozilla.org/mozilla-central/rev/627bfb5807ea