Bob Silverberg [:bsilverberg] Assignee
	Description • 7 years ago

This will use browserSetting [1] to manage which extension has control over the pref. This does not really belong in privacy so will go into the browserSettings API.

According to the notes in [2], this would make use of the `dom.webnotifications.enabled` although I am not sure about that. Toggling that setting does not affect notifications generated via the WebExtensions notifications API, but perhaps those are not expected to be affected. Perhaps this is only for Web API notifications.

I noticed that there is another pref called `notification.feature.enabled`, but I am not sure what it does and it defaults to false. I tested to see if toggling it affects WebExtensions API notifications, and it does not.

More investigation is required, but both of those prefs are booleans, so when we do figure out the implementation it can be a boolean called browser.browserSettings.desktopNotificationsEnabled.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/BrowserSetting
[2] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#

	Bob Silverberg [:bsilverberg] Assignee
	Comment 1 • 7 years ago

I did some more testing and verified that setting `dom.webnotifications.enabled` to false does disable Web API notifications but not WebExtensions API notifications. If that is sufficient to satisfy this requirement then we can go ahead with that. Rob, can you confirm that?

	Rob Wu [:robwu]
	Comment 2 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> I did some more testing and verified that setting
> `dom.webnotifications.enabled` to false does disable Web API notifications
> but not WebExtensions API notifications. If that is sufficient to satisfy
> this requirement then we can go ahead with that. Rob, can you confirm that?

I haven't tested, but what you've described sounds good.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 3 • 7 years ago

Because this only controls Web API notifications, I am going to name the setting `webAPINotificationsEnabled`.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 4 • 7 years ago

Kit, could you please confirm whether flipping the `dom.webnotifications.enabled` is an acceptable approach for an API that would allow a WebExtension to disable and enable Web API notifications?

	Lina Butler [:lina]
	Comment 5 • 7 years ago

TL;DR: For now, probably.

You'll probably also want to disable "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the pref will hide the `Notification` constructor and `serviceWorkerRegistration.showNotification()` function, so scripts that don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that OK for now?

We'll probably want to suppress delivering `push` events to service workers, too. Otherwise, we'll fire the event, the worker won't be able to show a notification, and we'll eventually revoke the site's push subscription because it sent too many silent pushes...even though it tried to do the right thing! (I think "do not disturb" has the same problem). There's an undocumented "dom.push.testing.notifyWorkers" pref that you can set to false to suppress this; we should probably make it public, if we go this route.

I think the correct long-term approach is to let you set permissions globally, though. All Web API notifications are controlled by the "desktop-notification" permission, so disabling it for all sites will do the right thing, without needing to flip a matrix of prefs. That's bug 1275599.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 6 • 7 years ago

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #5)

Thanks for the detailed response, Kit. 

> TL;DR: For now, probably.
> 
> You'll probably also want to disable
> "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the
> pref will hide the `Notification` constructor and
> `serviceWorkerRegistration.showNotification()` function, so scripts that
> don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that
> OK for now?
> 

I noticed in my manual testing that, even with only `dom.webnotifications.enabled` set to "false" the `Notification` constructor is no longer available and I was getting errors when trying to create a notification. I'm not sure if you're suggesting that would only happen if I also set `dom.webnotifications.serviceworker.enabled` to false, or if I am reading that incorrectly. In any case that is a potential issue.

Rob: do you think that is an acceptable result if developers use this API to disable notifications? 

> We'll probably want to suppress delivering `push` events to service workers,
> too. Otherwise, we'll fire the event, the worker won't be able to show a
> notification, and we'll eventually revoke the site's push subscription
> because it sent too many silent pushes...even though it tried to do the
> right thing! (I think "do not disturb" has the same problem). There's an
> undocumented "dom.push.testing.notifyWorkers" pref that you can set to false
> to suppress this; we should probably make it public, if we go this route.
> 

So is the only option right now to suppress delivering `push` events to service workers setting the "dom.push.testing.notifyWorkers" pref to false? I'm not sure what an undocumented pref is vs. a public one? Can I still use that pref in our API code regardless of whether it is public or not, or does something need to be done to make it public?

> I think the correct long-term approach is to let you set permissions
> globally, though. All Web API notifications are controlled by the
> "desktop-notification" permission, so disabling it for all sites will do the
> right thing, without needing to flip a matrix of prefs. That's bug 1275599.

That does sound like a good approach. Bug 1275599 looks like it's about creating a UI to allow users to better control permissions on a site-by-site level, so is it also about controlling permissions globally? That is what this bug would need. Is that something we can do now, without a UI, or is it expected that the ability to control permissions globally is something that will be included in bug 1275599?

	Lina Butler [:lina]
	Comment 7 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> I noticed in my manual testing that, even with only
> `dom.webnotifications.enabled` set to "false" the `Notification` constructor
> is no longer available and I was getting errors when trying to create a
> notification. I'm not sure if you're suggesting that would only happen if I
> also set `dom.webnotifications.serviceworker.enabled` to false...

Yes. Service workers don't use the `Notification` constructor; instead, they have `serviceWorkerRegistration.showNotification()`. Disabling `dom.webnotifications.serviceworker.enabled` will hide that method from service workers. `dom.webnotifications.enabled` only controls the `Notification` constructor, not `showNotification`.

> So is the only option right now to suppress delivering `push` events to
> service workers setting the "dom.push.testing.notifyWorkers" pref to false?

Yes. You can definitely use the pref in your API code; I only meant undocumented in the sense that it doesn't have a default value, and it's only used in the push tests.

> Is that something we can do now, without a UI, or is it
> expected that the ability to control permissions globally is something that
> will be included in bug 1275599?

You're right, I think it only covers the UI part. :MattN suggested extending the permission manager to do this in bug 1313939, comment 5, but I'm not sure if that went anywhere. We can always add another pref to suppress notifications and `push` events (but keep `new Notification()` and `.showNotification()` exposed) for now, and remove it if and when we can deny permissions globally.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 8 • 7 years ago

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #7)
> 
> Yes. Service workers don't use the `Notification` constructor; instead, they
> have `serviceWorkerRegistration.showNotification()`. Disabling
> `dom.webnotifications.serviceworker.enabled` will hide that method from
> service workers. `dom.webnotifications.enabled` only controls the
> `Notification` constructor, not `showNotification`.
>

Thanks for the clarification. So I could set these prefs, but we'll still have the problem of this potentially breaking web sites.
 
> > So is the only option right now to suppress delivering `push` events to
> > service workers setting the "dom.push.testing.notifyWorkers" pref to false?
> 
> Yes. You can definitely use the pref in your API code; I only meant
> undocumented in the sense that it doesn't have a default value, and it's
> only used in the push tests.
> 

Ok, that sounds like a workable solution for the push case.

> > Is that something we can do now, without a UI, or is it
> > expected that the ability to control permissions globally is something that
> > will be included in bug 1275599?
> 
> You're right, I think it only covers the UI part. :MattN suggested extending
> the permission manager to do this in bug 1313939, comment 5, but I'm not
> sure if that went anywhere. We can always add another pref to suppress
> notifications and `push` events (but keep `new Notification()` and
> `.showNotification()` exposed) for now, and remove it if and when we can
> deny permissions globally.

It does sound like having a pref to disable notifications without removing the exposed functions is something we'd need in order to not break web sites. How difficult would that be to do?

It also sounds like the best long term solution is to deal with this via a global permission, rather than prefs. I gather from your last sentence that we have no way, via code, to currently disable a permission globally - is that correct? From a WebExtensions API perspective we don't really need there to be a UI, we could control all of this through code. Although that raises the issue of how a user would re-enable notifications themselves if an extension disabled them. Currently that would work if either the extension provided a way of re-enabling them or it would happen automatically if the extension was disabled or uninstalled.

This isn't particularly urgent, so maybe it's better to wait to address it with permissions, rather than add a new pref as discussed above. What do you think? Are either of those things likely to happen before 57?

	Rob Wu [:robwu]
	Comment 9 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> (In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #5)
> 
> Thanks for the detailed response, Kit. 
> 
> > TL;DR: For now, probably.
> > 
> > You'll probably also want to disable
> > "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the
> > pref will hide the `Notification` constructor and
> > `serviceWorkerRegistration.showNotification()` function, so scripts that
> > don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that
> > OK for now?
> > 
> 
> I noticed in my manual testing that, even with only
> `dom.webnotifications.enabled` set to "false" the `Notification` constructor
> is no longer available and I was getting errors when trying to create a
> notification. I'm not sure if you're suggesting that would only happen if I
> also set `dom.webnotifications.serviceworker.enabled` to false, or if I am
> reading that incorrectly. In any case that is a potential issue.
> 
> Rob: do you think that is an acceptable result if developers use this API to
> disable notifications?

I think that it is better to have a preference that causes Notification.requestPermission to fail, opposed to hiding the API.
If that is not feasible, then hiding the related APIs is acceptable.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 10 • 7 years ago

Kit, I neglected to needinfo you on comment #8, which includes some followup questions for you. I would appreciate it if you could take a look and reply when you have the time.

	Lina Butler [:lina]
	Comment 11 • 7 years ago

Ack, I'm sorry I missed your comment, Bob. After thinking some more about this, I'm not sure that adding another pref to disable notifications globally is the way to go.

Matt brought up some good points in bug 1313939, comment 5, which apply to this approach as well. If the disable notifications pref is set, but the permission manager has `ALLOW` for that domain, which takes precedence? If you navigate to the site, open the control center, and change the notification permission, what happens? Does the pref still override the per-site setting, or do we allow exceptions?

Reading https://developer.chrome.com/extensions/contentSettings, it looks like `chrome.contentSettings.notifications` also takes a pattern...so ISTM we'd need to use the pref if `<all_urls>` is given, and the permission manager otherwise. However, the permission manager doesn't support wildcards, either, so "http://*.example.com/*" wouldn't work at all, and a global Boolean pref wouldn't help here.

We could turn the pref into a blacklist, but now we're adding a complicated hack around the permission manager, and we still don't have good answers to Matt's questions.

I think it really would be best to wait until we have a way to set permissions globally. Unfortunately, per bug 1275599, comment 6, it looks like that's cut from the MVP, because of the risk that users will accidentally break their sites. But maybe we can still implement support for global blocks (and wildcards) in the permission manager, without exposing them in the UI.

Johann, what do you think?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 12 • 7 years ago

Thanks for the reply, Kit. I believe Johann and I (and some others) are going to meet today to discuss this whole permissions thing.

	Johann Hofmann [:johannh]
	Comment 13 • 7 years ago

Thanks for alerting me, I proposed a solution in bug 1313939 and made it block the meta-bug of this. Supporting wildcards in the permission manager sounds hard right now, but a global toggle might be possible, depending on how that bug goes.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 14 • 7 years ago

Thanks Johann. As per our conversation I'm going to mark this as blocked by 1313939.

	Shane Caraveo (:mixedpuppy)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review196064

I'd like to know for sure that this doesn't unexpectedly break on android since the pref is defaulted only for firefox.  Otherwise I'd r+.  r? me again when you got an answer or fix.

::: toolkit/components/extensions/ext-browserSettings.js:88
(Diff revision 1)
>    setCallback(value) {
>      return {[this.prefNames[0]]: value};
>    },
>  });
>  
> +ExtensionPreferencesManager.addSetting("webAPINotificationsEnabled", {

how about just webNotificationsEnabled?

::: toolkit/components/extensions/ext-browserSettings.js:133
(Diff revision 1)
>              return aboutNewTabService.newTabURL;
>            }, URL_STORE_TYPE, true),
> +        webAPINotificationsEnabled: getSettingsAPI(extension,
> +          "webAPINotificationsEnabled",
> +          () => {
> +            let prefValue = Services.prefs.getIntPref("permissions.default.desktop-notification");

this pref seems to only be defaulted for browser, not toolkit.  Does getSettingsAPI protect against prefs throwing?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 17 • 7 years ago

Thanks for the review, Shane. I was thinking about this a bit and now I'm wondering if we should only allow an extension to make the default "block", which is what the bug originally asked. If we allow extensions to also set the default to "allow" that would mean that notifications would be allowed on all sites, and the user would never get prompted, which seems like a bad idea. What do you think?

	Johann Hofmann [:johannh]
	Comment 18 • 7 years ago

Yeah that seems like a bad idea. :)

	Johann Hofmann [:johannh]
	Comment 19 • 7 years ago

To say a little more, we are not intending to expose the "Allow Always" option in the UI. The current mockup shows just a single checkbox saying "Never prompt for notification permission" or something like that. It's not checked by default, but if the user checks it we would flip the pref that you're modifying to 2/BLOCK. This work is tracked in bug 1368744.

I would love if we could collaborate to add an indicator to preferences that the setting is being controlled by an extension once these two bugs have landed.

We could also consider to add more indicators if we notice users being confused, e.g. in page info, the identity popup or even the identity block.

	Shane Caraveo (:mixedpuppy)
	Comment 20 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #17)

Yeah, I think the addon shouldn't set allow_always.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review196064

I've also changed the behaviour so it can only be used to disable notifications, not to enable notifications.

	Shane Caraveo (:mixedpuppy)
	Comment 23 • 7 years ago

Sorry, now I wonder if an extension disables it, shouldn't it be able to re-enable (but not override user selection)?  I can see not allowing to just randomly turn it on.

	Shane Caraveo (:mixedpuppy)
	Comment 24 • 7 years ago

ie. if the pref is not user set, and it is enabled, and the extension disables it, it seems like it should be able to undo that setting.

	Shane Caraveo (:mixedpuppy)
	Comment 25 • 7 years ago

Sorry for the noise, after irc and looking it's fine.

	Shane Caraveo (:mixedpuppy)
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review198302

Lets run try/android

	Pulsebot
	Comment 28 • 7 years ago

Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ac09a374b69
Allow WebExtensions to disable Web API notifications, r=mixedpuppy

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2ac09a374b69

	Will Bamberg [:wbamberg]
	Comment 30 • 7 years ago

Docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/webNotificationsDisabled

I haven't updated the compat data yet. I had a question: although the JSON file suggests this is supported on Android, some of the comments above suggest that it isn't. Is there some nuance we should capture in the compat data for Android?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 31 • 7 years ago

Thanks Will, the docs look good. I'm not sure if this will work on Android or not. Johann, what do you think?

	Johann Hofmann [:johannh]
	Comment 32 • 7 years ago

We just don't have a default pref on Android right now (we could change that), but setting the pref should still work. :)

	marius.santa
	Comment 33 • 7 years ago

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.