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

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dgarnerlee, Assigned: allstars.chh)

Tracking

({dev-doc-needed})

unspecified
2.1 S8 (7Nov)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [p=3])

Attachments

(3 attachments, 10 obsolete attachments)

4.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
22.29 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
3.46 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

5 years ago
Blocks: 948721
(Reporter)

Comment 1

5 years ago
Related to: 963533
Depends on: 963533
blocking-b2g: --- → backlog
(Reporter)

Updated

5 years ago
Assignee: nobody → dgarnerlee
Blocks: 963531
(Reporter)

Comment 2

5 years ago
(Reporter)

Comment 3

5 years ago
WIP Gaia change: ontagfound with unimplemented ndefmessage.
(Reporter)

Comment 4

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

Comment 5

5 years ago
Attachment #8422713 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
Attachment #8422742 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
No longer blocks: 963531
(Reporter)

Updated

5 years ago
Depends on: 1075198
Why does this depend on Bug 1075198?
In ontagfound the parameter should be type of MozNFCTag, not Uint8Array.
No longer depends on: 963533
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
Flags: needinfo?(dgarnerlee)
(Reporter)

Comment 10

5 years ago
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
Flags: needinfo?(dgarnerlee)
(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.
Assignee: dgarnerlee → allstars.chh
No longer depends on: 964183, 1075198
Attachment #8446326 - Attachment is obsolete: true
Attachment #8446328 - Attachment is obsolete: true
Attachment #8446330 - Attachment is obsolete: true
Attachment #8509320 - Attachment is obsolete: true
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.
Attachment #8510161 - Flags: review?(dlee)
I have problems running marionette-webapi now, so I'd complete the test case in Bug 1087928.
Attachment #8510161 - Flags: review?(dlee)
Attachment #8510161 - Attachment is obsolete: true
Attachment #8510227 - Flags: review?(dlee)
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
Attachment #8510227 - Flags: review?(dlee)
Attachment #8510227 - Attachment is obsolete: true
Attachment #8511832 - Flags: review?(dlee)
Attachment #8511832 - Flags: review?(dlee) → review+
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
Attachment #8511832 - Flags: review?(bugs)
Comment on attachment 8510160 [details] [diff] [review]
Part 1: indent to 2 spaces.

fixed indent to 2 spaces.
Attachment #8510160 - Flags: review?(bugs)
Attachment #8510160 - Flags: review?(bugs) → review+
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'.
Attachment #8511832 - Flags: review?(bugs) → review+
s/Chrome/parent/g
Attachment #8511832 - Attachment is obsolete: true
Attachment #8512417 - Flags: review+
Attachment #8512452 - Flags: review?(bugs) → review+
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S8 (7Nov)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.