Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Description • 11 years ago

This is mentioned by Jonas during the last NFC API review.

It seems we could just send 'nfc-ndef-discovered' event or system message only to system app, instead of broadcasting.

	Ken Chang[:kenkai]
	Comment 1 • 11 years ago

Garner, need your comments.

	Garner Lee Assignee
	Comment 2 • 11 years ago

The SystemMessegner sendMessage call should be able to handle sending to one target.
b2g/chrome/content/shell.js:220 (pageURL), :240 (manifestURL)

	Garner Lee Assignee
	Comment 3 • 11 years ago

WIP.

	Garner Lee Assignee
	Comment 5 • 11 years ago

Comment on attachment 8407035 [details] [diff] [review]
(v2) 0001-Bug-964183-B2G-NFC-Don-t-broadcast-nfc-ndef-discover.patch

Current marionette tech-discovered test (but not lost) exists. I believe Dimi have a related bug landing soon with test cases?

	Garner Lee Assignee
	Comment 6 • 11 years ago

Try pending:
https://tbpl.mozilla.org/?tree=Try&rev=f96874bebf97

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 7 • 11 years ago

Comment on attachment 8407035 [details] [diff] [review]
(v2) 0001-Bug-964183-B2G-NFC-Don-t-broadcast-nfc-ndef-discover.patch

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

I think we need to deliever those messages to the app with nfc-manager permission,
writing a fixed URL here might not be a good idea.

Also you need to fix the existing test case, otherwise the tests will fail after your patch is landed.

	Garner Lee Assignee
	Comment 8 • 11 years ago

(In reply to Yoshi Huang[:allstars.chh] from comment #7)
> Comment on attachment 8407035 [details] [diff] [review]
> (v2) 0001-Bug-964183-B2G-NFC-Don-t-broadcast-nfc-ndef-discover.patch
> 
> Review of attachment 8407035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need to deliever those messages to the app with nfc-manager
> permission,
> writing a fixed URL here might not be a good idea.
> 
> Also you need to fix the existing test case, otherwise the tests will fail
> after your patch is landed.

I'll go back and verify the permissions is still enforced for the single message. The fixed string is a bit of a platform issue to solve: A system message that "knows" the pageURL and manifestURL of System App, or a gecko_const.js or setting service (without using a fixed key lookup value) of some sort that can make it available if there's no dedicated message path.

Fabrice, do you have a suggestion?

	[:fabrice] Fabrice Desré
	Comment 9 • 11 years ago

gecko knows the manifest url of the system app. It's accessible from a pref, see https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#226

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 10 • 11 years ago

(In reply to Garner Lee from comment #8)
> I'll go back and verify the permissions is still enforced for the single
> message. The fixed string is a bit of a platform issue to solve: A system
> message that "knows" the pageURL and manifestURL of System App, or a
> gecko_const.js or setting service (without using a fixed key lookup value)
> of some sort that can make it available if there's no dedicated message path.
> 
The nfc-manager-tech-discovered system message already goes with nfc-manager permission.

However my point is that from a API perspective, it's a bad idea to say 
"This system message would be sent to 'System app'",
also that there could be other vendors or OEMs have their own gaia implementation, they could possibly put nfc_manager in other certified app, instead of System app.
And I think it would be better to say "This system message will be sent to the app with nfc-manager permission"

Back to the first place, the NFC API right now is already doing this, it's just that it's doing a broadcast to all apps, with nfc-mananger permission checking.
So I would say this is a low-priority bug and we don't have to fix it right now.(before 1.4 Sprint 6)

	Garner Lee Assignee
	Comment 11 • 11 years ago

From broadcastMessage (and _sendMessageCommon), it appears the filter is by the permission, so it's not actually broadcast to all apps, just looped through.

In that view, if having a "NFC Manager App" outside System App is something a device maker may want to customize and route by permission filtering (but not by being more restrictive with adding page URLs and manfiest URLs), then I agree with you this improvement bug is low priority pending more discussion on the goals.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 12 • 10 years ago

The main problem of this bug is that System message is used to wake up app which is not running yet. And NFC uses System Message to notify System app, which is always running.
So the problem is not we should dispatch the nfc-manager-tech-discovered/lost to some particular app, but is to use DOM callback for this.

Will do this in Bug 1127182