e10s support for disabling site notifications

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(e10s+, firefox44 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Disabling notifications via the new "disable notifications from this site" action doesn't work in e10s. Looks like we aren't forwarding `alertdisablecallback`, and `RemoveFromPrincipal` can't be used from the content process.
(Assignee)

Comment 1

3 years ago
Created attachment 8670529 [details] [diff] [review]
0001-Bug-1212129-e10s-support-for-disabling-site-notifica.patch

Here's my attempt at a fix, thrown together on the plane. I had to add some includes to `StructuredCloneData.cpp` and `TabParent.cpp` because of unified sources.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Attachment #8670529 - Flags: feedback?(wchen)
Attachment #8670529 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

3 years ago
Blocks: 1212149
Comment on attachment 8670529 [details] [diff] [review]
0001-Bug-1212129-e10s-support-for-disabling-site-notifica.patch

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

It looks like we have an unnecessary round trip from child -> parent because we could just handle the disable event when we receive it in the parent. If you do go this route, you should probably factor out the code that removes the permission into its own method and use it from both the observer and the parent.
Attachment #8670529 - Flags: feedback?(wchen)
Nevermind, just realized that you don't get a principal from the message.

Updated

3 years ago
Attachment #8670529 - Flags: feedback+
Comment on attachment 8670529 [details] [diff] [review]
0001-Bug-1212129-e10s-support-for-disabling-site-notifica.patch

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

I don't really know about about the various notification environments and their lifetimes to give proper feedback. The only feedback I have is that we may want to keep in mind how we would implement closing all notifications for a site if the user disables one (bug 1212144).
Attachment #8670529 - Flags: feedback?(MattN+bmo)

Updated

3 years ago
tracking-e10s: --- → +
(Assignee)

Comment 5

3 years ago
Created attachment 8676595 [details]
MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r=wchen

Bug 1212129 - e10s support for disabling site notifications. r?wchen
Attachment #8676595 - Flags: review?(wchen)
Comment on attachment 8676595 [details]
MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r=wchen

https://reviewboard.mozilla.org/r/22773/#review20497

r+ with comments addresssed.

::: dom/notification/Notification.h:425
(Diff revision 1)
> +  static nsresult DisableAlerts(nsIPrincipal* aPrincipal);

How about moving this method to the Notification class and calling it RemovePermission?

::: dom/notification/Notification.cpp:1130
(Diff revision 1)
> +  nsCOMPtr<nsIPermissionManager> permissionManager =

Add an assertion for XRE_IsParentProcess()
Attachment #8676595 - Flags: review?(wchen) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8676595 [details]
MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r=wchen

Bug 1212129 - e10s support for disabling site notifications. r=wchen
Attachment #8676595 - Attachment description: MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r?wchen → MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r=wchen
https://hg.mozilla.org/mozilla-central/rev/023f1805d2d3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.