Closed Bug 896748 Opened 6 years ago Closed 6 years ago

Electrolysis: Notification request box doesn't open

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Notification.requestPermission() throws an exception and doesn't throw an exception.
Blocks: fxe10s
Wow. I meant "and doesn't show a permission request"
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #783195 - Flags: review?(felipc)
Comment on attachment 783195 [details] [diff] [review]
v1

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+      var requestingWindow = aRequest.window.top;
>+      chromeWin = this._getChromeWindow(requestingWindow).wrappedJSObject;
>+      browser = chromeWin.gBrowser.getBrowserForDocument(requestingWindow.document);
>+      if (!browser) {
>+        // find the requesting browser or iframe
>+        browser = requestingWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                    .getInterface(Ci.nsIWebNavigation)
>+                                    .QueryInterface(Ci.nsIDocShell)
>+                                    .chromeEventHandler;
>+      }

Not your code, but if you're changing this you might as well fix it: we should just always use the chromeEventHandler trick, rather than first trying getBrowserForDocument. I can't think of any cases where it would give a different answer, and chromeEventHandler works in more cases.

>diff --git a/dom/ipc/AppProcessChecker.cpp b/dom/ipc/AppProcessChecker.cpp

> AssertAppProcess(PBrowserParent* aActor,
>                  AssertAppProcessType aType,
>                  const char* aCapability)
> {
>+#ifndef MOZ_WIDGET_GONK
>+// We currently don't want to check permissions of
>+// content processes on Desktop.
>+  return true;
>+#endif

Why not? This seems like a change that merits wider discussion.

>diff --git a/toolkit/modules/PopupNotifications.jsm b/toolkit/modules/PopupNotifications.jsm

>+  _isActiveBrowser: function (browser) {
>+    return browser.docShell
>+      ? browser.docShell.isActive
>+      : (this.window.gBrowser.selectedBrowser == browser);
>+      // Todo: e10s should use something more like docShell.isActive.

The comment should probably specify that the entire reason why this helper exists is for the e10s case, and that otherwise we'd just use .isActive.
>Why not? This seems like a change that merits wider discussion.
Browsers are not apps, from my understanding they implement a very different model of how permissions are implemented. This is not the right time to think about the possible security benefits of explicitly checking the security privileges. This is something for the sandboxing era. We already made somewhat similar decision in bug 891954.

I totally agree to the rest.
Depends on: 900707
Comment on attachment 783195 [details] [diff] [review]
v1

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

Looks alright, needs an update with the comments from Gavin. And as I understood, the AppProcessChecker.cpp change will be removed and moved to bug 900707, right?

::: browser/components/nsBrowserGlue.js
@@ +1754,3 @@
>  
> +    try {
> +      /* This is only supposed to work with remote tabs. */

what is only supposed to work with remote tabs? is it the existence of aRequest.element?
Attachment #783195 - Flags: review?(felipc) → feedback+
Attached patch v2 (obsolete) — Splinter Review
Addressed feedback and added better comments. I moved the permission stuff to Bug 900707.
Attachment #783195 - Attachment is obsolete: true
Attachment #789128 - Flags: review?(felipc)
Comment on attachment 789128 [details] [diff] [review]
v2

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

::: browser/components/nsBrowserGlue.js
@@ +1764,3 @@
>  
> +    try {
> +      // This is only works in e10s mode, because of "element".

s/is//

@@ +1766,5 @@
> +      // This is only works in e10s mode, because of "element".
> +      browser = aRequest.element;
> +      chromeWin = browser.ownerDocument.defaultView;
> +    } catch (e) {}
> +

chromeWin will now be undefined in non-e10s mode, but it's still used in the function.

I think you can just move the chromeWin = browser... line after the if () below
and not use _getChromeWindow

that means that also you probably don't need to wrap the .element in a try catch, unless that throws in non-e10s?  But please keep the comment saying that it's only used for e10s
Attachment #789128 - Flags: review?(felipc) → feedback+
Attached patch v3Splinter Review
Sorry about not testing a normal build. This is nice then before I think.
Attachment #789128 - Attachment is obsolete: true
Attachment #791489 - Flags: review?(felipc)
Attachment #791489 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/7ca66a4c4c5e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Blocks: 908692
You need to log in before you can comment on or make changes to this bug.