Closed
Bug 964183
Opened 11 years ago
Closed 10 years ago
B2G NFC: Don't broadcast 'nfc-manager-tech-discovered' system message
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog)
RESOLVED
DUPLICATE
of bug 1127182
tracking-b2g | backlog |
People
(Reporter: allstars.chh, Assigned: dgarnerlee)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Blocks: 948721
Updated•11 years ago
|
No longer blocks: b2g-NFC-2.0
Updated•11 years ago
|
Assignee: nobody → psiddh
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)
Assignee | ||
Comment 3•11 years ago
|
||
WIP.
Updated•11 years ago
|
blocking-b2g: --- → backlog
Updated•11 years ago
|
Assignee: psiddh → dgarnerlee
Reporter | ||
Updated•11 years ago
|
Summary: B2G NFC: Don't broadcast 'nfc-ndef-discovered' system message → B2G NFC: Don't broadcast 'nfc-manager-tech-discovered' system message
Updated•11 years ago
|
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8402134 -
Attachment is obsolete: true
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?
Attachment #8407035 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 6•11 years ago
|
||
Try pending: https://tbpl.mozilla.org/?tree=Try&rev=f96874bebf97
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.
Attachment #8407035 -
Flags: review?(allstars.chh)
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?
Flags: needinfo?(fabrice)
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
Flags: needinfo?(fabrice)
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)
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.
Reporter | ||
Updated•11 years ago
|
Target Milestone: 1.4 S6 (25apr) → ---
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•