Closed Bug 829030 Opened 7 years ago Closed 7 years ago

Accessing the desktop notification API in the browser and approving permission errors out - API does not work

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: jsmith, Assigned: gwagner)

References

Details

Attachments

(1 file, 3 obsolete files)

Build: B2G 18 1/10/2013
Device: Unagi

Steps:

1. Go to http://jds2501.github.com/webapi-permissions-tests/webapi_permissions_test.html in the browser
2. Select Create Notification
3. Accept permissions

Expected:

A notification should appear with an icon, text, and a description.

Actual:

We fail miserably.

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
This definitely blocks.
blocking-basecamp: --- → ?
Summary: Accessing the desktop notification API and approving permission errors out - API does not work → Accessing the desktop notification API in the browser and approving permission errors out - API does not work
Assignee: nobody → anygregor
Jonas, should we remove the check or to you have a better idea?
Flags: needinfo?(jonas)
blocking-basecamp: ? → +
Talked to Gregor about this, so we have a plan.
Flags: needinfo?(jonas)
Attached patch WiP (obsolete) — Splinter Review
WiP. This works for our testcases.
Attached patch patch (obsolete) — Splinter Review
Attachment #700921 - Attachment is obsolete: true
Attachment #700976 - Flags: review?(jonas)
Comment on attachment 700976 [details] [diff] [review]
patch

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

::: dom/ipc/AppProcessChecker.cpp
@@ +50,5 @@
> +  rv = pm->TestPermissionFromPrincipal(principal, aCapability, &permission);
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +  if (NS_FAILED(rv) || !(permission == nsIPermissionManager::ALLOW_ACTION || permission == nsIPermissionManager::PROMPT_ACTION)) {

Line-break this at 80 columns.

Also, I think the logic would be easier to read as

return NS_SUCCEEDED(rv) && (permission == ALLOW || permission == PROMPT)

But that's up to you
Attachment #700976 - Flags: review?(jonas) → review+
This is just part 1 of >1
Whiteboard: [leave open]
Attached patch patch (obsolete) — Splinter Review
Attachment #700976 - Attachment is obsolete: true
Attachment #701047 - Flags: review?(jonas)
Component: General → Gaia
Target Milestone: --- → B2G C4 (2jan on)
Component: Gaia → General
Comment on attachment 701047 [details] [diff] [review]
patch

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

We concluded that this wasn't safe.

::: b2g/app/b2g.js
@@ +237,5 @@
>  
>  // Layers Acceleration.  We can only have nice things on gonk, because
>  // they're not maintained anywhere else.
>  #ifndef MOZ_WIDGET_GONK
> +pref("dom.ipc.tabs.disabled", false);

Don't check this in
Attachment #701047 - Flags: review?(jonas) → review-
Attached patch patchSplinter Review
New approach
Attachment #701047 - Attachment is obsolete: true
Attachment #701550 - Flags: review?(jonas)
Can someone land this? I'd like to test this patch asap in next build available.
Whiteboard: [leave open]
I'm assuming work week rules still apply basecamp+ bugs. Closing.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Duplicate of this bug: 818167
Andrea, you pushed this patch using yourself as an author. I assume Gregor Wagner should have been credited for it.
Ah... I didn't release it. Probably the reason was that the patch was without any header, so probably mq set my username.
Heh I didn't see it. I showed Andrea how to push patches so it's kind of my mistake anyway.
Andrea, if you don't see the correct username when you do |hg out|, you can change it with |hg qref -u|. Like |hg qref -u 'John Doe <johndoe@gmail.com>'|
A good way to prevent that kind of issue is to have `hg qnew` always add your username in the header but not `hg qref`.

If you have specified a username in your .hgrc, you can add this:
[defaults]
qnew = -U
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.