Closed Bug 937430 Opened 8 years ago Closed 7 years ago

Add support onforegrounddispatch callback to NFC DOM

Categories

(Firefox OS Graveyard :: NFC, defect)

defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: psiddh, Assigned: psiddh)

References

Details

Attachments

(6 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.11 (KHTML, like Gecko) Ubuntu/12.04 Chromium/20.0.1132.47 Chrome/20.0.1132.47 Safari/536.11

Steps to reproduce:

Follow up bug for initial 933136. Separating onforegrounddispatch to other event handlers 'onpeerfound' & 'onpeerlost'
Depends on: webnfc
Assignee: nobody → psiddh
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Fabrice has some idea to modify SystemMessage to route to only foreground app.
With this we could make app to have nfc-manage permission and send the system message to contact app only before sending to system app.

If we couldn't have this and since we cannot access window in nfc.js, we may need to implement solution#1 :/
Flags: needinfo?(fabrice)
So, my idea is that we should introduce a new call and not rely on sendMessage() or broadcastMessage().

It would be something like : sendToForegroundApp(type, message, extra) and will send the message to either the foreground app if it listens for this message type, and fallback to the system app if it listens for this message.

That means that we need to know which app is the foreground app - and this knowledge is currently only available to gaia. So we need gaia to send back to gecko the manifest url of the foreground app. Gecko itself already knows the manifest url of the system app.

Alive, can you work on the gaia side if that option is ok for you?
Flags: needinfo?(fabrice) → needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #2)
> Fabrice has some idea to modify SystemMessage to route to only foreground
> app.
> With this we could make app to have nfc-manage permission and send the
> system message to contact app only before sending to system app.
> 
> If we couldn't have this and since we cannot access window in nfc.js, we may
> need to implement solution#1 :/

I'd also suggest a specific "nfc-forgeround-dispatch" (or similar) permission if we go that route, since nfc-manager credentialed API(s) are important to protect from normal apps. Apps without it the new permission, can't get foreground dispatch.
(In reply to Fabrice Desré [:fabrice] from comment #3)
> So, my idea is that we should introduce a new call and not rely on
> sendMessage() or broadcastMessage().
> 
> It would be something like : sendToForegroundApp(type, message, extra) and
> will send the message to either the foreground app if it listens for this
> message type, and fallback to the system app if it listens for this message.
> 
> That means that we need to know which app is the foreground app - and this
> knowledge is currently only available to gaia. So we need gaia to send back
> to gecko the manifest url of the foreground app. Gecko itself already knows
> the manifest url of the system app.
> 
> Alive, can you work on the gaia side if that option is ok for you?

Sure.
Flags: needinfo?(alive)
Hi Fabrice , 

Couple of questions please

> It would be something like : sendToForegroundApp(type, message, extra) and will send the message to either the 
> foreground app if it listens for this message type, and fallback to the system app if it listens for this message.

A) Could you please tell us if sendToForegroundApp() when called from Chrome returns some kind of a return value.
Say, returns true if the foreground app listens for this message, else false. The reason for such a condition is that when a nfc event is dispatched by 'nfcd' (gonk) to Nfc Chrome process, the dispatch order should be something like this.
  1. Is there any foreground app eligible to process the event
      YES : Dispatch to the foreground app
      NO  : Dispatch it to system app so that it display a picker (MozActivity) from which user chooses the application to launch 
 As you can see, there needs to be a simple 'if' like condition

B) Is it possible to enforce some kind of the permission rule like, only the foreground app with 'nfc-write' permissions in its manifest file can get this event.

C) Is it possible for Nfc applications to dynamically de-register from the 'message type' ?

> That means that we need to know which app is the foreground app - and this
> knowledge is currently only available to gaia. So we need gaia to send back
> to gecko the manifest url of the foreground app. Gecko itself already knows
> the manifest url of the system app.

Currently we use pass the top applications' manifest URL for 'onpeerready' Nfc DOM event handler. When Chrome process tries to notifies / fire 'onpeerready' it fires to the appropriate content (Not a broadcast) based on 'appID' that the content process registered with Chrome.

Do you think , by introducing another Nfc DOM event handler 'onforegrounddispatch' would be useful ?
Flags: needinfo?(fabrice)
If we add this mechanism to system messages, it needs to not be nfc-specific, while fitting nfc use cases.

so,
A) My plan is to have the logic inside sendToForegroundApp() to dispatch either to the current foreground app or to the system app - if they listen for the correct message of course.

B) Yes, we can use permissions like for the other messages types.

C) Applications can already unregister themselves from a message type by setting a null message handler: navigator.mozSetMessageHandler('nfc-something', null).
Flags: needinfo?(fabrice)
Blocks: 969136
Uploading this gaia patch only for feedback. Will issue a 'git pull' request soon.
Attachment #8384066 - Attachment description: 0004-Bug-937430-Part-4-Use-sendMessageToForegroundApp-API.patch → (v1) Part 4: Nfc Parent process to use new interface to notify foreground application. r=allstars.chh
Attachment #8384063 - Flags: feedback?(fabrice.desre)
Attachment #8384064 - Flags: feedback?(fabrice.desre)
Attachment #8384065 - Flags: feedback?(fabrice.desre)
Attachment #8384066 - Flags: feedback?(allstars.chh)
Attachment #8384067 - Flags: feedback?(alive)
Uploaded all the patches required for supporting 'onforegrouddispatch' and added respective reviewers for their initial feedback and review of implementation.

Alive, I am uploading a gaia patch only for your initial feedback. I will follow it up with 'git pull' request.

Fabrice, I added a third argument for 'sendMessageToForeground' so that users of this API can have a slightly better control when dispatching the system message.
Note that this argument is optional.

1) API can be used to send a message to foreground app. If no app is present in foreground, then dispatch the same message type to 'system app'
  Ex:- sendMessageToForegroundApp('xyz-msg', message); // 'xyz-msg' will be dispatched to system app, provided there is no foreground app handling it

2) sendMessageToForegroundApp('xyz-msg', message, null);  // If there is no foreground app handling 'xyz-msg', then it will NOT be delivered to system app

3) sendMessageToForegroundApp('xyz-msg', message, 'my-new-xyz-msg'); // If there is no foreground app handling 'xyz-msg', then 'my-new-xyz-msg' will be delivered to system app

Do you think this will be useful or an overkill ? what do you suggest?
As usual, Nfc Gaia applications interested to receive 'foreground event' can do the following:

window.navigator.mozSetMessageHandler('onforegrounddispatch', function (msg) {
  var nfcdom = window.navigator.mozNfc;
  var nfcTag = nfcdom.getNFCTag(msg.sessionToken);
  var records = new Array();
  var empty = new Uint8Array(0);
  var emptyRec = [new MozNDEFRecord(nfc.tnf_well_known,
                                    nfc.rtd_text,
                                    empty, 'dummy text')];
  records.push(emptyRec);
  var req = nfcTag.writeNDEF(records);
  req.onsuccess = (function() {
    debug('writeNDEF successfully');
  });
  req.onerror = (function() {
    debug('writeNDEF FAILED');
  });
});

In the application's manifest, add the relevant system message 

  "messages": [
     { "onforegrounddispatch": "/nfc.html" }
   ]
Attachment #8384063 - Attachment is obsolete: true
Attachment #8384063 - Flags: feedback?(fabrice.desre)
Attachment #8384191 - Flags: feedback?(fabrice.desre)
Attachment #8384067 - Flags: feedback?(alive) → feedback+
Comment on attachment 8384064 [details] [diff] [review]
(v1) Part 2: Receive foreground application's  manifestUrl in shell.js. r=fabrice

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

::: b2g/chrome/content/shell.js
@@ +756,5 @@
>            JSON.stringify({ nfcHardwareState: detail.nfcHardwareState }));
>          break;
> +      case 'update-foregroundapp-manifest-url':
> +        Services.obs.notifyObservers(null, 'update-foregroundapp-manifest-url',
> +          JSON.stringify({ manifestUrl: detail.url }));

no need to stringify a js object there, you can you send the manifest url.
Attachment #8384064 - Flags: feedback?(fabrice.desre) → feedback+
Sidd, have you thought again for the onpeerready case? 
onpeerready also needs the app is in the foreground however mostly the logic is in nfc_manager (Gaia)

However this bug uses system message and mozContent event to pass the information, and mostly it is done in the Gecko part(SystemMessageManager). Also in this bug it also covers the P2P case,
1. If a device P2P is in proximity
2. If we get a NDEF from a P2P device
And these two cases will be notified to Gaia through mozActivity,

What will happen if the foreground app has registered onpeerready callback and the onforeground callback?
Comment on attachment 8384191 [details] [diff] [review]
(v1) Part 1: Add new interface 'sendMessageToForegroundApp' in system-message-internal. r=fabrice

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

::: dom/messages/SystemMessageInternal.js
@@ +305,5 @@
> +        found = true;
> +        break;
> +      }
> +    }
> +

Until this point this is very similar to broadcastMessage. I think we could make broadcastMessage (or a broadcastInternal method) do that and return a boolean to tell whether or not we found pages to dispatch the message.

@@ +310,5 @@
> +    // Foreground App not registered to receive 'aType',
> +    // dispatch the type 'aFallbackType' to system application (privileged) if specified
> +    if (!found && (aFallbackType !== null)) {
> +        debug("aFallbackType   " + JSON.stringify(aFallbackType));
> +        let privilegedContent = 'app://system.gaiamobile.org';

this should not be hardcoded. You can get the manifest url from Services.prefs.getCharPref('browser.manifestURL') and the main page from Services.prefs.getCharPref('browser.homescreenURL') (see https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#210)

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +44,5 @@
> +   *                      When there is no registered foreground application to receive the message of 'type',
> +   *                      following three conditions arise depending on whether this parameter is specified or not.
> +   *                      - If 'fallbackType'is not specified, then message of given 'type' is dispatched to the system application.
> +   *                      - If 'fallbackType' is explicitly specified then it is dispatched to the system application.
> +   *                      - If 'fallbackType'is specified as null, then no message is dispatched to privileged content

since it's not the last parameter it's hard to distinguish between "not specified" (is this 'undefined') and "specified as null" since both are falsy.
Attachment #8384191 - Flags: feedback?(fabrice.desre)
Comment on attachment 8384065 [details] [diff] [review]
(v1) Part 3: Add new system message 'onforegrounddispatch'. r=fabrice

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

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +113,5 @@
>    "nfc-powerlevel-change": {
>      "settings": ["read", "write"]
>    },
>    "wifip2p-pairing-request": { },
> +  "onforegrounddispatch": {

that's a very generic name. We need something more nfc specific here.
Attachment #8384065 - Flags: feedback?(fabrice.desre) → feedback-
Hi Yoshi,
Good point raised. My bad. I have not updated the bug with the latest patch that I tested the usecases of both Passive Tag & P2P scenarios. Please provide your feedback on the latest patch (v1a).

If it is a 'P2P' tech, then 'onpeerready' will be called (If the Nfc application has registered for it). If it is a regular Passive Tag, then the application's callback 'onforegrounddispatch' will be called. (Again if the application has registered for it). Multiple Nfc Applications can have both callbacks registered at the same time. 'onpeerready' & 'onpeerfound' are DOM calls while 'onforegrounddispatch' is system message callback.
What do you think ?

In fact, We can remove 'broadcastMessage' completely from Nfc.js and instead use, as following:-
   gSystemMessenger.sendMessage("nfc-manager-tech-discovered",
                                 message,
                                 Services.io.newURI(pageURL, null, null),
                                 Services.io.newURI(manifestURL, null, null));

I will upload that patch later (in a separate bug) as it may not be appropriate to overload this bug with it. Please note that I have tested this patch too for both P2P & Passive tag scenarios.
Attachment #8384066 - Attachment is obsolete: true
Attachment #8384066 - Flags: feedback?(allstars.chh)
Attachment #8384990 - Flags: feedback?(allstars.chh)
(In reply to Siddartha P from comment #20)
> 
> If it is a 'P2P' tech, then 'onpeerready' will be called (If the Nfc
> application has registered for it). If it is a regular Passive Tag, then the
> application's callback 'onforegrounddispatch' will be called. (Again if the
> application has registered for it). Multiple Nfc Applications can have both
> callbacks registered at the same time. 'onpeerready' & 'onpeerfound' are DOM
> calls 

What's onpeerfound?

> while 'onforegrounddispatch' is system message callback.

System message is used to notify the app, even the app is *not* launched yet.

But now the app is in the foreground, why do you insist on using a system message?
Why don't we use a regular DOM callback, like ontagfound from W3C NFC API?
Why can't we reuse how onpeerready is done?

Before you introduce any new API, please step back to look Bug 948721.
I think we still have some flaws in our original WebNFC API.
The onforegrounddispatch still has the same problem in Bug 963533 and Bug 964183.

Unless your new proposal could also fix them, otherwise we should focus on *automation tests* and refine the orginal APIs first.

BTW, you can try to explain these API to the developer next to you, (maybe Garner or others), please try to explain what's the usage and difference between
- onpeerready
- nfc-manager-tech-discovered system message and nfc-ndef-discover MozActivity.
- and the one you're trying to do now : onforegrounddispatch

I have been asked by several Gaia/Gecko developers about this question. :(

If you're going to add another system message(onforegrounddispatch) to do this, I would say it makes this NFC API more difficult to use, and it isn't a good thing we'd like to see.

I'll propose we make this API simpler.
> What's onpeerfound?
I meant 'onpeerlost' that is used in conjunction with 'onpeerready'

> System message is used to notify the app, even the app is *not* launched yet.
I think, this is true for APIs 'broadcastMessage' and 'sendMessage'. However if you use the new API, 'sendMessageToForegroundApp' it will *not* launch the app even if we send this system message using the API, because this API internally checks whether 'target page' is launched or not.(If not launched, it will not deliver it to the app). This API will only deliver the message if and only if it is foreground (top most visible) application. Note that this API will *not* deliver the message even if the application is launched but not in the foreground (say it is in background).

> But now the app is in the foreground, why do you insist on using a system message?
> Why don't we use a regular DOM callback, like ontagfound from W3C NFC API?
> Why can't we reuse how onpeerready is done?
Agreed. We can do that. However I felt it was worth an alternative proposal for discussion.

> The onforegrounddispatch still has the same problem in Bug 963533 and Bug 964183.
This new API / system message will definitely address Bug 964183. 

> Unless your new proposal could also fix them, otherwise we should focus on *automation tests* and 
> refine the orginal APIs first.
Ok. Agreed.

Ok. Let me try. I am not suggesting that is is simple for gaia developers. After this, I will also update the wiki to make it more clearer.

> onpeerready
- This is only for PEER / P2P use case [Operation Mode to be used for: P2P write]
> nfc-manager-tech-discovered system message and nfc-ndef-discover MozActivity.
- 'nfc-manager-tech-discovered' system message is *never* meant to be used by Nfc Gaia applications. It is used only by 'System app - nfc_manager.js'. Also with this fix (v1a - Part4 patch), Gaia applications will never receive it even if they try to listen.

- 'nfc-ndef-discover' is purely a way to provide choice to the user. If the user is in say HomeScreen , and there is an incoming NDEF as a result of 'P2P' or 'Passive Tag' scenario, the user will be shown a picker to choose the Nfc App from. This provides app neutrality allowing user to make choice of Nfc App to be launched. 
Nfc Gaia App developers may avail this option if they want their 'app' to be displayed in the picker.  [Operation Mode to be used for: NDEF READ - Both P2P & Tag]
> and the one you're trying to do now : onforegrounddispatch
- This is for application in the foreground to detect passive Tag for subsequent Write operation
  [Operation Mode to be used for: Tag Write)]

> If you're going to add another system message(onforegrounddispatch) to do this, I would say it makes 
> this NFC API more difficult to use, and it isn't a good thing we'd like to see.
> I'll propose we make this API simpler.
Ok. Agreed. If you think that by having dom api for 'foregrounddispatch' makes it slightly simple, I am with you. I was only drawing everyone's attention to the pros and cons of one approach over another.
(One is W3C way  of doing things vs Android'ish way of things)
Hi Yoshi, Could you also please check the previous comments from 3 - 7 also?
(In reply to Siddartha P from comment #23)
> Hi Yoshi, Could you also please check the previous comments from 3 - 7 also?

I read it before,
A system message can be only forwarded to the foreground app is nice,
but how are we going to use this for WebNFC? We have to think about this.
Given we already had Bug 963533 and Bug 964183. And certainly this bug will also have these two problems. I really don't think landing a patch with knowing it has some potential issues is a good idea right now.

I also read your comment 22, and I also looked your patch from Part 1 to Part 5. 
I know what and how you're trying to fix this.
It's great we could have some new API for NFC.

But to put it in short, I have to emphasize again 
"We need to make our NFC WebAPI stable enough first, because if it's not, these's no chance it will be a feature in 1.5, 1.6 or so on".

I think you know many features, not just NFC, has been postponed, right now the stability is our major concern here.

If you try to land a NFC API but has problems in Bug 963533 and Bug 964183, we may not be able to land it. Even I agree to land it, but other Managers knowing this could decide to back it out in the last minute.
ok, got it.
Attachment #8384990 - Flags: feedback?(allstars.chh) → feedback-
blocking-b2g: --- → backlog
This bug is overlapping with Bug 991970(ontagfound), and no update for 7 months,
close this as WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.