1192458 - Consolidate push and desktop notification permissions

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

Allow push and desktop notifications to be toggled separately, but only prompt to enable notifications if a site requests push. This means enabling push takes a single click, instead of the current two clicks.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458 - Consolidate push and desktop notification permissions. r?mt

Comment 2

[Lina Butler [:lina]]

OK, here's a first cut.

* Push and notifications permissions can be set independently, but we'll only prompt to enable notifications.
* If push is allowed (in `about:permissions`), we'll deliver messages and skip quota enforcement.
* If push is blocked, we won't deliver any messages.
* Otherwise, if there's no push permission for a page, we check the notifications permission.
* If notifications are allowed, we'll deliver push messages and enforce the quota.
* Finally, if notifications are blocked, we won't deliver any messages.

If the user declines notifications when we prompt them, we'll drop messages, too. Technically, it's possible to enable push, but disable notifications, too. That'll restrict service workers to background updates...but that's OK, since we're relaxing the quota if push is explicitly enabled.

The default push state in `about:permissions` is now "allow". This isn't really accurate, but "depends on notifications" is a bit of a mouthful. I'm also unhappy that there's no way to restore the default state: once you set the permission for a site, you either consent to receiving pushes without a quota, or no pushes at all. I hid push from the "all sites" view to minimize the impact of toggling this, but I'm not fond of that, either.

Maybe, instead of push, we should have a "notifications" entry, with a slider to toggle push on and off? That seems like a clearer way to convey that the two are related. But I'm not the right person to make that call. :-)

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

https://reviewboard.mozilla.org/r/15489/#review13859

r=mt, but I'd like you do run this by someone who works on the permission manager.  Correctly explaining what is going on here is tricky.

I don't really understand the permission manager code sufficiently.  I think that you should ask a peer for review here.  When you do, you should explain exactly what it is that you intend from the changes here.

::: browser/components/preferences/aboutPermissions.js:328
(Diff revision 1)
> -    return this.UNKNOWN;
> +    return this.ALLOW;

Rather than fix this at allow when showing the name; I think that the right thing to do here is to change the string for unknown.  In other words, set the meaning of unknown to "has a quota"

::: browser/components/preferences/aboutPermissions.js:377
(Diff revision 1)
>    /**
> +   * Permissions that can't be set globally. These won't be shown in the "All
> +   * Sites" interface.
> +   */
> +  _noGlobal: ["push"],
> +
> +  /**
>     * Permissions that don't have a global "Allow" option.
>     */
> -  _noGlobalAllow: ["geo", "indexedDB", "camera", "microphone", "push"],
> +  _noGlobalAllow: ["geo", "indexedDB", "camera", "microphone"],

I think that push can move back to _noGlobalAllow.

::: dom/push/test/test_permissions.html:75
(Diff revision 1)
> -      ok(state === "denied", "permissionState() should resolve to denied.");
> +      { push: true, notifications: true, state: "granted" },

Style-nit: tri-states are probably best as strings.

::: dom/push/PushManager.cpp:364
(Diff revision 1)
>      nsCOMPtr<nsIPushClient> client =
>        do_CreateInstance("@mozilla.org/push/PushClient;1");
>      if (!client) {
>        callback->OnUnsubscribe(NS_ERROR_FAILURE, false);
> +      return NS_OK;

I don't understand this code well, and I don't know why you would need to make this change.  Are you fixing a bug?

::: dom/push/PushManager.cpp:573
(Diff revision 1)
>      if (NS_WARN_IF(NS_FAILED(rv)) || permission != nsIPermissionManager::ALLOW_ACTION) {
> +      rv = permManager->TestPermissionFromPrincipal(
> +        mProxy->GetWorkerPrivate()->GetPrincipal(),
> +        "desktop-notification",
> +        &permission);
> +
> +      if (NS_WARN_IF(NS_FAILED(rv)) || permission != nsIPermissionManager::ALLOW_ACTION) {
> -      callback->OnPushEndpoint(NS_ERROR_FAILURE, EmptyString());
> +        callback->OnPushEndpoint(NS_ERROR_FAILURE, EmptyString());
> -      return NS_OK;
> +        return NS_OK;
> -    }
> +      }
> +    }

Needs a comment explaining the logic.  It's a shame that this replicates the logic from below, as well as the code in Push.js and Push.jsm.  If you could centralize this, it would be good.

Comment 4

[Lina Butler [:lina]]

Hi Mark,

Could you have a look at this patch, please? Currently, if a site wants to use push notifications, the user is prompted twice: once for desktop notifications, and once for push.

We'd like to streamline this, so that allowing notifications automatically grants push permissions. We also want to let users explicitly disable push, though, so the interaction between the two states is tricky (comment 2 summarizes them). Does that seem reasonable, or should we simplify the permission model?

I'm also not sure of the best way to surface this in `about:permissions`, or "View Page Info > Permissions". It looks like the default state in `about:permissions` is "always ask," but "depends on quota" is more accurate here. OTOH, "quota" seems like an implementation detail...and we don't want to expose a UI for tweaking the default quota, because we might change the algorithm in a future release.

The "Permissions" tab in page info is closer to what I'm looking for: push would have "Use Default" (enforce the quota), "Allow" (no quota), and "Block". Maybe it's best to only expose push permissions in that tab, and remove them from `about:permissions` entirely?

Anyway, I'd love your feedback on both points. Thanks!

Comment 5

[Mark Hammond [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/15487/#review13981

I haven't looked at the permission changes in detail yet, but thought I'd publish this anyway.

::: dom/push/PushManager.cpp:573
(Diff revision 1)
>      if (NS_WARN_IF(NS_FAILED(rv)) || permission != nsIPermissionManager::ALLOW_ACTION) {

I doubt it matters much, but I'm not sure we want to enter that block if NS_FAILED(rv) is true (ie, we probably shouldn't fall back to checking the desktop notification but should immediately bail after notifying the callback

::: dom/push/PushRecord.jsm:160
(Diff revision 1)
> +      return Services.perms.testPermission(this.uri, "desktop-notification") ==

nit: I'd be inclined to just write this second line as |permission = Services.perms.testPermission(this.uri, "desktop-notification");| and fall through to the existing return

::: dom/push/PushService.jsm:808
(Diff revision 1)
> -    if (Services.perms.testExactPermission(scopeURI, "push") !=
> +    if (aPushRecord.hasPermission()) {

It looks alot like you are missing a |!| here? hasPermission() returning true doesn't appear to mean the permission was revoked.

If I'm correct, it also implies we need some additional test coverage?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

(In reply to Kit Cambridge [:kitcambridge] from comment #4)

> We'd like to streamline this, so that allowing notifications automatically
> grants push permissions. We also want to let users explicitly disable push,
> though, so the interaction between the two states is tricky (comment 2
> summarizes them). Does that seem reasonable, or should we simplify the
> permission model?

I'm not 100% sure about what you are asking. At face value, the code complexity looks reasonable (although I'd want to run with the patch applied to see it in action). However, if you are asking about the UX, you are probably asking the wrong person - I'd guess we want shorlander or his delegate.

> I'm also not sure of the best way to surface this in `about:permissions`, or
> "View Page Info > Permissions". It looks like the default state in
> `about:permissions` is "always ask," but "depends on quota" is more accurate
> here. OTOH, "quota" seems like an implementation detail...and we don't want
> to expose a UI for tweaking the default quota, because we might change the
> algorithm in a future release.
> 
> The "Permissions" tab in page info is closer to what I'm looking for: push
> would have "Use Default" (enforce the quota), "Allow" (no quota), and
> "Block". Maybe it's best to only expose push permissions in that tab, and
> remove them from `about:permissions` entirely?
> 
> Anyway, I'd love your feedback on both points. Thanks!

As above, I don't think I should offer an opinion here both due to my lack of experience with the Permissions Manager and my lack of UX skills ;)

If MattN doesn't mind, I think he should be the final reviewer for the permission manager stuff - best I can tell, he reviewed the patches when push permissions initially landed in bug 1038811, so I'll flag him for feedback now - he's likely to have opinions on all the above.

Comment 7

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #6)
> I'm not 100% sure about what you are asking. At face value, the code
> complexity looks reasonable (although I'd want to run with the patch applied
> to see it in action). However, if you are asking about the UX, you are
> probably asking the wrong person - I'd guess we want shorlander or his
> delegate.

My question is, "would this be too complicated for users to understand?" I ask because:

* All our existing permissions are independent. If a site wants to use the camera, geolocation and IndexedDB, the user has to grant each permission individually. On the other hand, maybe that's a bad example because they're clearly independent, whereas push and notifications are closely related. (Other platforms go further, and unify the two...granting notifications lets you send visible and silent notifications. If the user declines, you can't send either).

* The possible states for each permission are: {Always Ask, Deny, Allow}. Cookies seems to be an exception, with "Allow for session." This patch would introduce a new state: "depends on quota". Except, if we call it that, users might ask, "Well, what's the quota?"...and we don't want to expose this—or let them change it—because we may replace the quota entirely with another mitigation mechanism in a future release.

* The "Use Default" option defaults to one of the possible states. IOW, it means "one of the above." I was thinking a way to avoid exposing the "depends on quota" state for push is to make it the default—and only show "allow" and "deny". But then that makes the "use default" setting for Push "none of the above", and, AFAICT, no other permission works like this.

So it's not so much the code complexity I'm worried about, it's usability...but that's definitely a UX question, and I'll wait for MattN to weigh in first. It's also quite likely I'm overthinking this. :-)

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

We discussed this in-person and I pointed Kit to the plans for permission in control center and the W3C permissions API (which allows requesting multiple permissions at once).

Comment 9

[Bill Maggs (bmaggs)]

The plan that seems to make sense is to ask to allow push notifications once, without the need for a separate prompt to show notifications to the user in general. If you allow Push notifications, you may see some and not others. I think the attached graphic (with the megaphone graphic) is all users need to see. Further, I think that the default should be as shown,  Allow Push Notifications always.

Comment 10

[Bill Maggs (bmaggs)]

Attachment: Screenshot 2015-09-02 18.07.31.png

Comment 11

[Bill Maggs (bmaggs)]

(In reply to Bill Maggs from comment #9)
> The plan that seems to make sense is to ask to allow push notifications
> once, without the need for a separate prompt to show notifications to the
> user in general. If you allow Push notifications, you may see some and not
> others. I think the attached graphic (with the megaphone graphic) is all
> users need to see. Further, I think that the default should be as shown, 
> Allow Push Notifications always.

It would be good to get Security team input on having a single prompt to allow PNs with the default setting be Always Allow.

Comment 12

[Lina Butler [:lina]]

Consolidating the two isn't straightforward, because web (local) notifications and push request permission in different ways.

The notifications API provides a `requestPermission()` method that allows a site to ask for permission in advance. In fact, the chat demo does just that: https://github.com/johnmellor/push-api-appengine-demo/blob/faa4ce6b184786940063c7779d2ba815f203f678/chat.tpl#L253-L269 In contrast, push implicitly asks for permission, when the site first subscribes to receive push messages.

Unfortunately, we can't know the order in which sites will request permissions. It's OK if a site subscribes for push first, then asks for notifications: we can implicitly grant the notification permission, and avoid the second dialog. The problem is if the site asks for notifications first, as the GCM demo does. (I think this makes sense, actually...push isn't very useful without notifications). In that case, we'll still show the second dialog, since notifications is a subset of push.

The other approach is to make the "notifications" permission grant both, as this patch does. I've received pushback on this, since it changes the meaning of notifications from "local only" to "local and push." Also, sites that have previously requested local notifications can now receive pushes, too. This likely has privacy implications; if we go this route, I think we'd need to reset all existing notifications permissions.

Personally, I think the distinction between the two is unfortunate, and leads to combinatorial explosions like this. Android, iOS, and OS X have a single "notifications" permission that lets apps send both (IIRC, iOS didn't even support local notifications until iOS 8). I can see why some of our users would find this separation helpful, but I'm wary of bloating the product—and landing an ill-conceived hack—with more choices for the sake of choice... http://limi.net/checkboxes-that-kill/

Comment 13

[Tanvi Vyas[:tanvi]]

(In reply to Bill Maggs from comment #11)
> (In reply to Bill Maggs from comment #9)
> > The plan that seems to make sense is to ask to allow push notifications
> > once, without the need for a separate prompt to show notifications to the
> > user in general. If you allow Push notifications, you may see some and not
> > others. I think the attached graphic (with the megaphone graphic) is all
> > users need to see. Further, I think that the default should be as shown, 
> > Allow Push Notifications always.
> 
> It would be good to get Security team input on having a single prompt to
> allow PNs with the default setting be Always Allow.

I'm still getting caught up on this, as I haven't looked at it before.  But I had a nice chat with Bill on Friday.  I think the default should be for this session instead of always.  I personally went to the test chat app, and attempted to click the down arrow to allow just for this session, and instead ended up accidentally allowing always.  Since the control center is new, users who do this accidentally aren't going to know they can click on the icon in the url bar and change their minds.  At least for now, we should keep for this session.  Then once this gets more flushed out (are desktop and push notifications combined or separate? Do we notify users of notifications for sites that aren't currently open in any tabs?) we can revisit if necessary.


(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> The other approach is to make the "notifications" permission grant both, as
> this patch does. I've received pushback on this, since it changes the
> meaning of notifications from "local only" to "local and push." Also, sites
> that have previously requested local notifications can now receive pushes,
> too. This likely has privacy implications; if we go this route, I think we'd
> need to reset all existing notifications permissions.
>
I agree that this is problematic.  We should use the principle of least privileges - if a site just needs desktop notifications, don't grant access to desktop and push.

Comment 14

[Martin Thomson [:mt:]]

(In reply to Tanvi Vyas [:tanvi] from comment #13)
> At least for now, we should
> keep for this session.  

While this is an appealing position, I think that it does more harm than good here.

If we make this session-only by default, then that is the option that our users will choose.  Then we set up a training regime for clicking through the dialog (I wish that I could find dveditz's explanation regarding the certificate exception persistence checkbox to cite).

The risks of a persistent permission aren't non-existent, but the quota system limits the damage that a site can do.

Of course, without more information about the notification UX we will see, I can understand the desire to be cautious.  If revocation of consent isn't easy, then I appreciate the bias toward session-only.  That said, if revocation of consent isn't easy and obvious, we've failed to provide the right UX.

I would recommend firing up Chrome and playing with https://johnme-gcm.appspot.com/ and the notifications it generates.  We might be able to improve on that UX in some ways, but I don't think that it suffers from the problems you are concerned about.

> I agree that this is problematic.  We should use the principle of least
> privileges - if a site just needs desktop notifications, don't grant access
> to desktop and push.

I'd like to understand what you think a preferable alternative would be.  Again, it's easy to say that we want to give as little away as possible, but in practice asking any question of the user has a pretty high cost.  Asking for both permissions in this context is (at least in my opinion) a reasonable economy.

Maybe we could expose more checkboxes in the permission grant along the lines of https://www.dropbox.com/s/0nv9hqp508pjdxn/pn-03.png?dl=0

Comment 15

[Martin Thomson [:mt:]]

The conclusion from the discussion (with psackl, MattN, bmaggs and mt) today was that there would be a single permission.

The rationale being that the marginal utility of having two permissions did not justify the cost.  I think that the idea would be that any site that has been granted "notification" permission in the past will automatically be granted access to push.

We considered forcing those sites to re-acquire permission, but there was a mild preference against doing that since (again) the marginal utility isn't worth the added annoyance.

The subject of adding a "Learn more..." link to the dialog came up, but we decided that we would see what happened with the impending redesign (and what happened to the geolocation prompt there).

A single permission is consistent with the UX that Chrome presents.

Based on experience with WebRTC here, we might want to be careful to enable add-on control of this behaviour.  Some users will be concerned with Firefox phoning home; but the lesson there was that those users are best served by add-ons.

n-i for Tanvi so that she gets a reminder to review this in detail.

Comment 16

[Chris Karlof [:ckarlof]]

> The subject of adding a "Learn more..." link to the dialog came up, but we decided that we would see what happened with the impending redesign (and what happened to the geolocation prompt there).

One (currently) sad consequence of putting a link in a doorhanger is that if the user clicks on it, the doorhanger disappears, and it may not be obvious to the user how she summons it back.

Comment 17

[Chris Karlof [:ckarlof]]

Great, I feel a single unified permission is the right call. A couple of questions about the details:

1) Will Notification.requestPermission() and pushManager.subscribe() both trigger the same permissions doorhanger and grant the same permissions?

2) What will the permission options be in the permissions doorhanger? It currently offers "only for this session", which I feel is problematic (and isn't offered by Chrome). It adds complexity to de-allocate that state on shutdown or startup, and it's not clear if this option offers much user value. I feel it added more value when in the past it was harder to manager permissions, but it's since gotten better with control center and other planned improvements.

3) Will the permissions for Web and Push notifications also be unified into a single permission in the control center and permission manager in the prefs?

Comment 18

[Matthew N. [:MattN]]

(In reply to Chris Karlof [:ckarlof] from comment #16)
> > The subject of adding a "Learn more..." link to the dialog came up, but we decided that we would see what happened with the impending redesign (and what happened to the geolocation prompt there).
> 
> One (currently) sad consequence of putting a link in a doorhanger is that if
> the user clicks on it, the doorhanger disappears, and it may not be obvious
> to the user how she summons it back.

The site will likely ask for the permission again (either automatically in the same way or via the user clicking some button on the page again) and the user will likely read this page once so I don't think this is that big of a deal. Also, this is changing with the new design.

(In reply to Chris Karlof [:ckarlof] from comment #17)
> Great, I feel a single unified permission is the right call. A couple of
> questions about the details:
> 
> 1) Will Notification.requestPermission() and pushManager.subscribe() both
> trigger the same permissions doorhanger and grant the same permissions?

I believe the answer is yes but I'm not familiar with `pushManager.subscribe()`.

> 2) What will the permission options be in the permissions doorhanger? It
> currently offers "only for this session", which I feel is problematic (and
> isn't offered by Chrome). It adds complexity to de-allocate that state on
> shutdown or startup, and it's not clear if this option offers much user
> value. I feel it added more value when in the past it was harder to manager
> permissions, but it's since gotten better with control center and other
> planned improvements.

This partly depends on which version you're talking about and which version the new permission prompts land in.

> 3) Will the permissions for Web and Push notifications also be unified into
> a single permission in the control center and permission manager in the
> prefs?

Yes

Comment 19

[Chris Karlof [:ckarlof]]

> This partly depends on which version you're talking about and which version the new permission prompts land in.

Since we're targeting 44 for this work, what will be the options there?

Comment 20

[Bill Maggs (bmaggs)]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #18)
> (In reply to Chris Karlof [:ckarlof] from comment #16)
> > > The subject of adding a "Learn more..." link to the dialog came up, but we decided that we would see what happened with the impending redesign (and what happened to the geolocation prompt there).
> > 
> > One (currently) sad consequence of putting a link in a doorhanger is that if
> > the user clicks on it, the doorhanger disappears, and it may not be obvious
> > to the user how she summons it back.
> 
> The site will likely ask for the permission again (either automatically in
> the same way or via the user clicking some button on the page again) and the
> user will likely read this page once so I don't think this is that big of a
> deal. Also, this is changing with the new design.
> 
> (In reply to Chris Karlof [:ckarlof] from comment #17)
> > Great, I feel a single unified permission is the right call. A couple of
> > questions about the details:
> > 
> > 1) Will Notification.requestPermission() and pushManager.subscribe() both
> > trigger the same permissions doorhanger and grant the same permissions?
> 
> I believe the answer is yes but I'm not familiar with
> `pushManager.subscribe()`.
> 
> > 2) What will the permission options be in the permissions doorhanger? It
> > currently offers "only for this session", which I feel is problematic (and
> > isn't offered by Chrome). It adds complexity to de-allocate that state on
> > shutdown or startup, and it's not clear if this option offers much user
> > value. I feel it added more value when in the past it was harder to manager
> > permissions, but it's since gotten better with control center and other
> > planned improvements.
> 
> This partly depends on which version you're talking about and which version
> the new permission prompts land in.

Like Matt said, we're planning these changes for 44 and the Control Center UX team may have already created new designs for all the site permissions, or only some of them.  

My own preference would be to have the choices in Control Center be a lot more clear. They look the way they do now in part because our doorhangers are not modal unlike those in Chrome, which are modal per tab. You can just ignore our permissions, so they can disappear with no action taken.  So we need the "always" and "this session" concepts. I would to make important permissions modal per tab, so users know what they need to do. But I think the designers doing Control Center and Permissions in general have some new designs about these. There are NI'd here. 

> > 3) Will the permissions for Web and Push notifications also be unified into
> > a single permission in the control center and permission manager in the
> > prefs?
> 
> Yes

for right now in 43 the prompt will say Receive Push Notifications. We may be able to uplift something to that simplifies this to Allow Notifications, the preferable long term alternative.

Comment 21

[Bill Maggs (bmaggs)]

After talking to Designers working on the new Control Center and permissions dialogs, we agreed the simplest course of action is to simply reuse the existing Web Notifications permissions dialog and text, requiring no string changes but using a new icon rather than the megaphone, to signal this permission is something new. So the only changes to the prompt would be: 

1) changing the default choice for the prompt to be Always Allow rather than Allow for this session 
2) Add the Learn More link that was previously in the Receive Push Notifications link. 
3) new icon for the Notifications permission, to be determined by Steven Horlander, Bryan Bell, and the visual team. A thought bubble with an exclamation point is one choice being considered.

Comment 22

[Tanvi Vyas[:tanvi]]

I took a closer look at this and have provided by thoughts and questions below.

Number of Notifications:
If a website requests permissions for push notifications, I can understand why we would automatically grant permissions for desktop notifications.

If a website requests permission for desktop notification and only desktop notifications, we should try and avoid unnecessarily giving that website push permissions.  The problem with this, as Kit laid out in comment 12, is that some sites will ask for desktop notifications before asking for push.  We will have no way of knowing whether or not they are going to ask for push, and hence users may end up with two prompts.  Google calendar is a good example of a website where I want to enable local notifications but not push.

Ideally we could implement the following:
* If a website requests push, give access to push and desktop notifications.
* If a website requests desktop notifications, give access to just desktop notifications.

Websites that request desktop notifications before push will end up with two prompts.  Given the Push API is fairly new, do we think websites will adapt to this implementation to improve user experience (by asking for push first) or not?

String changes and Learn More:
If the answer is no, website will not adapt and we will end up with a poor user experience on Firefox, we will have to consolidate to one doorhanger.  The consolidated doorhanger should clearly articulate what permissions are being granted.  The text in the two doorhangers today reads as follows:
Push: Would you like to allow Push Notifications for this site.
Local: Would you like to show notifications from this site.

We can talk to UX and Matej about a new string for a consolidated doorhanger.  It should include something about how the browser is "receiving" notifications from the website.  And we should definitely continue to include the Learn More link.

Quota:
What is the current proposal on how the quota plays into the available options?  Should always allow or allow for this session be restricted by the quota?  Or are we going to add a state for "allow up until we reach the quota", making "allow" unbounded?

Legacy Desktop Notification Permissions:
The switch from desktop notifications to push is not to be taken lightly - a persistent connection with a server is much different than a local notification.  We should re-prompt when push permissions are requested for a user who has already enabled desktop notifications.  The one time prompt is not going to hinder user experience.  And it will give users a chance to deny this more invasive permission.

Global preference:
What happens if a user globally turns off push in about:permissions?  Will they still be able to use desktop notifications?

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458 - Consolidate push and desktop notification permissions. r?MattN

Comment 24

[Lina Butler [:lina]]

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

Comment 25

[Lina Butler [:lina]]

The latest patch subsumes Push and Web Notifications under the `desktop-notification` permission. It also adds a notifications entry to `about:permissions`. I didn't change the copy for the doorhanger.

Once this lands, we'll want to update https://www.mozilla.org/en-US/firefox/push, since the instructions to disable push will no longer be accurate.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

https://reviewboard.mozilla.org/r/15489/#review18283

Great job, very thorough! I can't give an official review for the stuff in dom/ so I'll flag nsm/wchen for that but it looked good at first glance other than the one thing I didn't understand.

::: browser/components/nsBrowserGlue.js:2677
(Diff revision 2)
>      const kFeatureKeys = { "geolocation" : "geo",
>                             "desktop-notification" : "desktop-notification",
> -                           "pointerLock" : "pointerLock",
> +                           "pointerLock" : "pointerLock"
> -                           "push" : "push"
>                           };

Nit: leave the trailing comma here so we don't change blame again next time we change the object.

::: browser/components/preferences/aboutPermissions.js:324
(Diff revision 2)
> -  get push() {
> -    if (!Services.prefs.getBoolPref("dom.push.enabled")) {
> +  get ["desktop-notification"]() {
> +    let isEnabled =
> +      Services.prefs.getBoolPref("dom.webnotifications.enabled") ||
> +      Services.prefs.getBoolPref("dom.push.enabled") ||
> +      Services.prefs.getBoolPref("dom.webnotifications.serviceworker.enabled");
> +    if (!isEnabled) {
>        return this.DENY;
>      }
>      return this.UNKNOWN;
>    },
> -  set push(aValue) {
> +  set ["desktop-notification"](aValue) {
>      let value = (aValue != this.DENY);
> +    // Disable push and web notifications.
> +    Services.prefs.setBoolPref("dom.webnotifications.enabled", value);
>      Services.prefs.setBoolPref("dom.push.enabled", value);
> +    Services.prefs.setBoolPref("dom.webnotifications.serviceworker.enabled",
> +                               value);
>    },
> +

I don't think we want users flipping all of these prefs and get in a broken state and about:permissions isn't really supported as there is no user-facing entrypoint (except SUMO :( ) so can you just add the permission to `_noGlobalDeny` to avoid having it show on the "All sites" page. I don't have as much of a problem with supporting toggling the permission for a specific site but I wouldn't do extra effort for a page that's not supported.

::: browser/locales/en-US/chrome/browser/browser.properties:378
(Diff revision 2)
>  webNotifications.showForSession=Show for this session
>  webNotifications.showForSession.accesskey=s

If we're sure we don't want the session option, you need to delete these strings too.

::: dom/push/PushManager.cpp:744
(Diff revision 2)
>        if (NS_SUCCEEDED(rv)) {
> -        switch (permission) {
> +        if (permission == nsIPermissionManager::ALLOW_ACTION) {
> -          case nsIPermissionManager::ALLOW_ACTION:
> -            state = PushPermissionState::Granted;
> +          state = PushPermissionState::Granted;
> -            break;
> +        } else if (permission == nsIPermissionManager::PROMPT_ACTION) {
> -          case nsIPermissionManager::DENY_ACTION:
> -            state = PushPermissionState::Denied;
> -            break;
> -          case nsIPermissionManager::PROMPT_ACTION:
> -            state = PushPermissionState::Prompt;
> +          state = PushPermissionState::Prompt;
> -            break;
> -          default:
> -            MOZ_CRASH("Unexpected case!");
>          }

What's this about? Why no DENY/Denied?

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

https://reviewboard.mozilla.org/r/15489/#review18341

This looks good to me.

::: browser/components/preferences/aboutPermissions.xul:254
(Diff revision 2)
> -                  <menuitem id="push-0" value="0" label="&permission.alwaysAsk;"/>
> +                  <menuitem id="desktop-notification-0" value="0" label="&permission.alwaysAsk;"/>

webNotifications.showForSession was removed above, so do we need this alwaysAsk option?

Comment 28

[William Chen [:wchen]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

https://reviewboard.mozilla.org/r/15489/#review18385

Comment 29

[Tanvi Vyas[:tanvi]]

Are we going to support a Show for this session option?  I thought the default was going to be "Show always" with a drop down for "Show for this session".

Which string is being used now with the attached patch?

Comment 30

[Lina Butler [:lina]]

(In reply to Tanvi Vyas [:tanvi] from comment #29)
> Are we going to support a Show for this session option?  I thought the
> default was going to be "Show always" with a drop down for "Show for this
> session".
> 
> Which string is being used now with the attached patch?

Oops...I thought we're removing "show for this session" entirely. The patch currently uses the "Would you like to show notifications for this site?" text, with the options "always show" and "always block". "Always show" is the default.

Comment 31

[Tanvi Vyas[:tanvi]]

I think we should include the for this session option.  In the new Control Center designs, it is pretty easy to change from always to session by clicking a checkbox.  Until this new design is implemented, we can add the "allow for this session" in the drop down.

Where should we track the work for a new string to make sure we get it into FF 44?  Here or in a different bug?

Thanks Kit!

Comment 32

[Lina Butler [:lina]]

(In reply to Tanvi Vyas [:tanvi] from comment #31)
> I think we should include the for this session option.  In the new Control
> Center designs, it is pretty easy to change from always to session by
> clicking a checkbox.  Until this new design is implemented, we can add the
> "allow for this session" in the drop down.

For Push, implementing "allow for session" seems tricky. We'll need to track transient subscriptions in IndexedDB, and drop them in a transaction on shutdown. `AsyncShutdown.jsm` will probably come in handy.

The other question for transient subscriptions is when to notify the service worker. I think we'll want to wait until the user visits the page again, or changes notification permissions via the management interface. Otherwise, the worker won't be able to request a new subscription, because the permission grant expired at the end of the last session. But that makes Push a lot less useful in that case.

> Where should we track the work for a new string to make sure we get it into
> FF 44?  Here or in a different bug?

Let's track the work in a different bug, if that's OK. We'll also want to update the "Learn More" link to reflect the consolidated permission.

> Thanks Kit!

No problem. Thanks for all your input!

Comment 33

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15489/#review18341

> webNotifications.showForSession was removed above, so do we need this alwaysAsk option?

Hmm, I think we do. This is the `UNKNOWN_ACTION` state, which you'll see if you close the permission dialog without selecting an option.

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458 - Consolidate push and desktop notification permissions. r=nsm,wchen r?MattN

Comment 35

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15489/#review18283

> I don't think we want users flipping all of these prefs and get in a broken state and about:permissions isn't really supported as there is no user-facing entrypoint (except SUMO :( ) so can you just add the permission to `_noGlobalDeny` to avoid having it show on the "All sites" page. I don't have as much of a problem with supporting toggling the permission for a specific site but I wouldn't do extra effort for a page that's not supported.

This felt wrong; I like your solution to jettison the global permission. :-)

> What's this about? Why no DENY/Denied?

I noticed we already default to `PushPermissionState::Denied` on line 735, so I tightened this up a bit. I'd also like to avoid crashing if the permission is `UNKNOWN_ACTION`.

Comment 36

[Bill Maggs (bmaggs)]

(In reply to Kit Cambridge [:kitcambridge] from comment #32)
> (In reply to Tanvi Vyas [:tanvi] from comment #31)
> > I think we should include the for this session option.  In the new Control
> > Center designs, it is pretty easy to change from always to session by
> > clicking a checkbox.  Until this new design is implemented, we can add the
> > "allow for this session" in the drop down.
> 
> For Push, implementing "allow for session" seems tricky. We'll need to track
> transient subscriptions in IndexedDB, and drop them in a transaction on
> shutdown. `AsyncShutdown.jsm` will probably come in handy.
> 
> The other question for transient subscriptions is when to notify the service
> worker. I think we'll want to wait until the user visits the page again, or
> changes notification permissions via the management interface. Otherwise,
> the worker won't be able to request a new subscription, because the
> permission grant expired at the end of the last session. But that makes Push
> a lot less useful in that case.
> 

After talking this over with the group, it seems like the user benefit of putting in a choice that won't be very discoverable and which he/she seems unlikely to understand isn't that high. We're erring on the side of caution in Psuh 1.0 by allowing users to disable, Do Not Disturb, or Manage all Notifications from every message; the basic experience in the permission should be yes/no; there's a lot of chances to change that or limit it afterwards.  When you factor in the complications with notifying the Service Worker that kit refers to, I think it best to keep it simple. 


> > Where should we track the work for a new string to make sure we get it into
> > FF 44?  Here or in a different bug?
> 
> Let's track the work in a different bug, if that's OK. We'll also want to
> update the "Learn More" link to reflect the consolidated permission.
> 
> > Thanks Kit!
> 
> No problem. Thanks for all your input!

Comment 37

[Lina Butler [:lina]]

(In reply to Tanvi Vyas [:tanvi] from comment #22)
> Legacy Desktop Notification Permissions:
> The switch from desktop notifications to push is not to be taken lightly - a
> persistent connection with a server is much different than a local
> notification.  We should re-prompt when push permissions are requested for a
> user who has already enabled desktop notifications.  The one time prompt is
> not going to hinder user experience.  And it will give users a chance to
> deny this more invasive permission.

I don't think we've officially answered this...what happens if a user has already granted the desktop notification permission to a site (like IRCCloud), then upgrades to a version of Firefox that supports Push?

Should we prompt the user with a different string? What happens if they decline? (With a single permission, the site will lose the ability to send both kinds of notifications). Or do we drop the `desktop-notification` permission, introduce a new one that covers both, and re-prompt for all sites?

Comment 38

[Lina Butler [:lina]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen r?MattN

Comment 39

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

Comment 40

[Matthew N. [:MattN]]

(In reply to Kit Cambridge [:kitcambridge] (PTO from 2015-10-06 to 2015-10-16) from comment #37)
> (In reply to Tanvi Vyas [:tanvi] from comment #22)
> > Legacy Desktop Notification Permissions:
> > The switch from desktop notifications to push is not to be taken lightly - a
> > persistent connection with a server is much different than a local
> > notification.  We should re-prompt when push permissions are requested for a
> > user who has already enabled desktop notifications.  The one time prompt is
> > not going to hinder user experience.  And it will give users a chance to
> > deny this more invasive permission.
> 
> I don't think we've officially answered this...what happens if a user has
> already granted the desktop notification permission to a site (like
> IRCCloud), then upgrades to a version of Firefox that supports Push?
> 
> Should we prompt the user with a different string? What happens if they
> decline? (With a single permission, the site will lose the ability to send
> both kinds of notifications). Or do we drop the `desktop-notification`
> permission, introduce a new one that covers both, and re-prompt for all
> sites?

I thought we decided on Vidyo that we don't need to reprompt.

Comment 41

[Matthew N. [:MattN]]

Comment on attachment 8669239 [details]
MozReview Request: Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

https://reviewboard.mozilla.org/r/21161/#review19057

It looks like this patch is implementing bug 1153500. This could have landed already without the permission change.

::: browser/themes/shared/jar.inc.mn:130
(Diff revision 1)
> +  skin/classic/browser/web-notifications-icon.svg              (../shared/web-notifications/icon.svg)
> +  skin/classic/browser/web-notifications-tray.svg              (../shared/web-notifications/tray.svg)

Having a different packaged file name just makes it harder to find the file to change so I would recommend having the source file name match the packaged one. I don't feel strongly about whether we need a directory or what the name is though.

::: browser/themes/shared/web-notifications/icon.svg:10
(Diff revision 1)
> +  <g id="artboard-1">

Nit: artboard-1 is unused

::: browser/themes/shared/web-notifications/tray.svg:1
(Diff revision 1)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="48" height="16" viewBox="0 0 96 32">
> +       
> +  <defs>
> +

Can you follow https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines please? e.g. license, whitespace,

::: browser/themes/shared/web-notifications/tray.svg:25
(Diff revision 1)
> +  	<path id="shape-notifcations-push" d="M27.000,23.969 L24.000,23.969 L24.000,29.977 L17.241,23.969 L5.000,23.969 C3.343,23.969 2.000,22.626 2.000,20.969 L2.000,6.969 C2.000,5.312 3.343,3.969 5.000,3.969 L27.000,3.969 C28.657,3.969 30.000,5.312 30.000,6.969 L30.000,20.969 C30.000,22.626 28.657,23.969 27.000,23.969 ZM18.000,8.969 C18.000,7.864 17.105,6.969 16.000,6.969 C14.895,6.969 14.000,7.864 14.000,8.969 L14.000,13.969 C14.000,15.073 14.895,15.969 16.000,15.969 C17.105,15.969 18.000,15.073 18.000,13.969 L18.000,8.969 ZM16.500,17.969 L15.500,17.969 C14.672,17.969 14.000,18.640 14.000,19.469 C14.000,20.297 14.672,20.969 15.500,20.969 L16.500,20.969 C17.328,20.969 18.000,20.297 18.000,19.469 C18.000,18.640 17.328,17.969 16.500,17.969 Z" class="cls-1"/>

Nit: cls-1 is unused in this file.

Comment 42

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #41)
> Comment on attachment 8669239 [details]
> MozReview Request: Bug 1192458, Part 2 - New icon for consolidated
> notification permission. r?MattN
> 
> https://reviewboard.mozilla.org/r/21161/#review19057
> 
> It looks like this patch is implementing bug 1153500. This could have landed
> already without the permission change.

Sorry, I wanted to avoid the merge conflict with consolidation, but I probably shouldn't have blocked the icon on this. Happy to split it out into a separate patch.

Tidied up the SVG per the guidelines.

Comment 43

[Lina Butler [:lina]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458, Part 2 - New icon for consolidated notification permission. r=MattN

Comment 44

[Lina Butler [:lina]]

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

Comment 45

[Lina Butler [:lina]]

Comment on attachment 8645262 [details]
MozReview Request: Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

Comment 46

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

Comment 47

[Lina Butler [:lina]]

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

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 8669833 [details]
MozReview Request: Bug 1192458, Part 2 - New icon for consolidated notification permission. r?MattN

https://reviewboard.mozilla.org/r/21261/#review19171

Comment 50

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/dbd2cecedbe3650c74e895dcec479b49af0c08ea
Bug 1192458, Part 1 - Consolidate push and desktop notification permissions. r=nsm,wchen,MattN

https://hg.mozilla.org/integration/fx-team/rev/8c261f0d4d3e47e04690824c953b5f8471ddd73e
Bug 1192458, Part 2 - New icon for consolidated notification permission. r=MattN

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbd2cecedbe3
https://hg.mozilla.org/mozilla-central/rev/8c261f0d4d3e

Comment 52

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] (PTO Oct. 8-9) from comment #40)
> (In reply to Kit Cambridge [:kitcambridge] (PTO from 2015-10-06 to
> 2015-10-16) from comment #37)
> > (In reply to Tanvi Vyas [:tanvi] from comment #22)
> > > Legacy Desktop Notification Permissions:
> > > The switch from desktop notifications to push is not to be taken lightly - a
> > > persistent connection with a server is much different than a local
> > > notification.  We should re-prompt when push permissions are requested for a
> > > user who has already enabled desktop notifications.  The one time prompt is
> > > not going to hinder user experience.  And it will give users a chance to
> > > deny this more invasive permission.
> > 
> > I don't think we've officially answered this...what happens if a user has
> > already granted the desktop notification permission to a site (like
> > IRCCloud), then upgrades to a version of Firefox that supports Push?
> > 
> > Should we prompt the user with a different string? What happens if they
> > decline? (With a single permission, the site will lose the ability to send
> > both kinds of notifications). Or do we drop the `desktop-notification`
> > permission, introduce a new one that covers both, and re-prompt for all
> > sites?
> 
> I thought we decided on Vidyo that we don't need to reprompt.

The permission is now granting access to two things when it formally only granted access to one.  The text is also different ("receive" notifications), the Learn More link will explain that this is for local notifications and Push, and there is a new icon (I think?).  Hence, we should re-prompt to get permission from the user to enable this expanded privilege.  A one time prompt is not that bad from a user experience point of view.

Comment 53

[Martin Thomson [:mt:]]

FWIW, I agree with Tanvi here.  This might actually have the (positive) effect of increasing visibility of the feature at the same time.