1212129 - e10s support for disabling site notifications

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Lina Butler (ex-Mozilla)]

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.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: 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.

Comment 2

[William Chen [:wchen]]

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.

Comment 3

[William Chen [:wchen]]

Nevermind, just realized that you don't get a principal from the message.

Comment 4

[Matthew N. [:MattN]]

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).

Comment 5

[Lina Butler (ex-Mozilla)]

Attachment: MozReview Request: Bug 1212129 - e10s support for disabling site notifications. r=wchen

Bug 1212129 - e10s support for disabling site notifications. r?wchen

Comment 6

[William Chen [: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()

Comment 7

[Lina Butler (ex-Mozilla)]

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

Comment 8

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1b2c4cc9a9

Comment 9

[Lina Butler (ex-Mozilla)]

https://hg.mozilla.org/integration/fx-team/rev/023f1805d2d331a9311ad75301dac09bde17b325
Bug 1212129 - e10s support for disabling site notifications. r=wchen

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/023f1805d2d3