Closed
Bug 824421
Opened 12 years ago
Closed 12 years ago
Cannot request access to the desktop notification webapi in the browser - no permission prompt is seen, webapi can't be used
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)
People
(Reporter: jsmith, Assigned: gwagner)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
Build: B2G18 12/23/2012
Device: Unagi
Steps:
1. Go to http://jds2501.github.com/webapi-permissions-tests/webapi_permissions_test.html in the browser
2. Select create notification
Expected:
A permission prompt should appear asking the user if they would like to grant access to the desktop notification webapi.
Actual:
Nothing happens. When I install a hosted app that calls out for desktop notification permissions, that same test works fine. So the permission prompt is probably failing to show in a browser context.
Relevant code from test case:
var notification = navigator.mozNotification.createNotification(
"Sample title", "Sample description", "http://jds2501.github.com/webapi-permissions-tests/qalogo.png");
notification.show();
Reporter | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Reporter | ||
Comment 1•12 years ago
|
||
And when I mean "works fine" in the actual results I mean I get implicit access to desktop notification webapi and as a result, the desktop notification is fired with the correct name, description, and icon.
However, the permission prompt won't even show up in the browser. And I don't even get access to the webapi in the browser at all, even though the permissions matrix says otherwise.
Blocks: finalize-permissions
Comment 2•12 years ago
|
||
Andrew, can you comment on the expected use of the notification API in the browser context? Should the user see a prompt? Is this API supported in this context?
Flags: needinfo?(overholt)
Reporter | ||
Comment 3•12 years ago
|
||
Data comparison:
Permissions matrix states the following:
* Web Content - PROMPT_ACTION
* Hosted Web App - ALLOW_ACTION
* Privileged Web App - ALLOW_ACTION
* Certified Web App - ALLOW_ACTION
Permissions Installer code states the following:
"desktop-notification": {
app: ALLOW_ACTION,
privileged: ALLOW_ACTION,
certified: ALLOW_ACTION
},
Current behavior from testing states the following:
* Web Content - DENY_ACTION
* Hosted Web App - ALLOW_ACTION
* Privileged Web App - ALLOW_ACTION
* Certified Web App - ALLOW_ACTION
Comment 4•12 years ago
|
||
Doug says he'll handle this. It's definitely something that should be usable from web content.
Assignee: nobody → doug.turner
Flags: needinfo?(overholt)
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
Assignee: doug.turner → wchen
Comment 5•12 years ago
|
||
Someone who know more about permissions should take a look in case there is a more appropriate way to hook up permissions for web content. The only other permissions that are exposed to web content are geolocation (currently being treated as a special case) and fmradio (currently not implemented).
Attachment #696403 -
Flags: review?(doug.turner)
Updated•12 years ago
|
Target Milestone: --- → B2G C4 (2jan on)
Comment 6•12 years ago
|
||
Comment on attachment 696403 [details] [diff] [review]
Added prompt for notification permission requests from web content.
Review of attachment 696403 [details] [diff] [review]:
-----------------------------------------------------------------
gregor, can you also look at this?
::: b2g/components/ContentPermissionPrompt.js
@@ +55,1 @@
> permissionManager.addFromPrincipal(aPrincipal,
This logic is getting a bit complex. Maybe we should create a new function that takes the permission name and returns a bool:
function shouldAllowPermission(aPermission, aPerm, aPrincipal) {
let type = permissionManager.testExactPermissionFromPrincipal(aPrincipal, aPerm);
if (type == Ci.nsIPermissionManager.PROMPT_ACTION) {
return true;
}
if (type != Ci.nsIPermissionManager.UNKNOWN_ACTION) {
return false;
}
if (PROMPT_FOR_UNKNOWN.indexOf(aPermission) >= 0) {
return true;
}
if (PROMPT_FOR_WEB_CONTENT.indexOf(aPermission) >= 0) {
return aPrincipal.isInBrowserElement;
}
return false;
}
I think that this is a bit more verbose but more readable. What do you think?
Attachment #696403 -
Flags: review?(doug.turner)
Attachment #696403 -
Flags: review?(anygregor)
Attachment #696403 -
Flags: review+
Assignee | ||
Comment 7•12 years ago
|
||
I think we should combine this PROMPT_FOR_UNKNOWN and PROMPT_FOR_WEB_CONTENT since they mean the same. I will post a patch soon.
Assignee | ||
Comment 8•12 years ago
|
||
Assignee: wchen → anygregor
Attachment #699095 -
Flags: review?(doug.turner)
Assignee | ||
Updated•12 years ago
|
Attachment #699095 -
Flags: review?(jonas)
Updated•12 years ago
|
Target Milestone: B2G C4 (2jan on) → ---
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #696403 -
Attachment is obsolete: true
Attachment #699095 -
Attachment is obsolete: true
Attachment #696403 -
Flags: review?(anygregor)
Attachment #699095 -
Flags: review?(jonas)
Attachment #699095 -
Flags: review?(doug.turner)
Attachment #699183 -
Flags: review?(jonas)
Attachment #699183 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 12•12 years ago
|
||
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Reporter | ||
Comment 13•12 years ago
|
||
Well, I'm getting the perm prompt, I can deny perms, but accepting perms blows up:
01-09 12:28:58.921: E/GeckoConsole(109): [JavaScript Error: "Desktop-notification message app-notification-send from a content process with no desktop-notification privileges." {file: "chrome://browser/content/shell.js" line: 747}]
01-09 12:28:58.931: I/Gecko(109): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
Gregor mentions that he's going to fix this as part of bug 823974.
Hmm.. I'm not convinced that bug 823974 will fix that problem. But let's verify after that bug is fixed.
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #14)
> Hmm.. I'm not convinced that bug 823974 will fix that problem. But let's
> verify after that bug is fixed.
Andrea - Were you able to reproduce what I hit in comment 13 with your patch applied from bug 823974?
Flags: needinfo?(amarchesini)
Comment 16•12 years ago
|
||
Yes, I see the prompt dialog. I cannot reproduce this bug.
Flags: needinfo?(amarchesini)
Comment 17•12 years ago
|
||
Sorry guys, it still crashes.
Reporter | ||
Comment 18•12 years ago
|
||
Basic flows looks okay. Marking as verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•