SimplePush patches that affect Firefox Desktop UI or components.

This adds a easy way for users to enable Push in release builds. The same can be done via about:config, so I don't know if we should be setting a precedent for toggling preferences via the Preferences dialog. This does emulate how chrome allows features to be enabled via about:flags in a nicer UI.

How is a user supposed to know what "Allow websites to use Push Notifications" means? What do push notifications look like on desktop? Is there a support document that describes what this feature is?

I'm not sure if you've already discussed this with Madhava, but rather than requesting ui-review on a series of patches, it's probably more effective to just sit down with someone from UX and talk it through directly.

:gavin good idea. we'll work with webdev to build something similar to https://www.mozilla.org/en-US/firefox/geolocation/ :nsm, can you file a bug -- assign to me.

:dougt, Filed the bug. IMHO the UI option in Preferences is an odd man out, and we should stick to just having an about:config pref flip option instead.

(In reply to Nikhil Marathe from comment #6) > :dougt, Filed the bug. IMHO the UI option in Preferences is an odd man out, > and we should stick to just having an about:config pref flip option instead. Seems reasonable to me - I don't really see why someone would want to disable entirely given that it already has a per-site opt-in.

> I don't really see why someone would want to disable entirely given that it already has a per-site opt-in it follows what we did with geo. Also, it allows you to disable even for the sites you have already opt-ed in for.

Geolocation doesn't have preferences UI for a global toggle AFAIK.

There is a defaut setting switch in about:permissions, iirc. It's not directly user-visible, though it will be at some point. However, this is no global switch.

Minor formatting cleanup. ui-review=madhava Gavin can you review both of these please?

Comment on attachment 768411 [details] [diff] [review] Permission prompt doorhanger Looks fine, but I think this needs a "Learn more" link, and I think you should sit down with a UX person to make sure these strings make sense.

+1 on the more info. talking to webdev Wednesday about content.

Getting content on SUMO is probably the best bet. I would talk to mverdi and get an article with an in-product SUMO link alias set up (see app.support.baseURL).

Rebased patch onto changes from 889835 to show entry in pageinfo popup, pageinfo permissions tab and about:permissions.

Comment on attachment 774196 [details] [diff] [review] Page Info for Push permissions >--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties >+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties >@@ -11,9 +11,9 @@ permission.cookie.label = Set Cookies > permission.desktop-notification.label = Show Notifications > permission.image.label = Load Images > permission.install.label = Install Add-ons > permission.popup.label = Open Pop-up Windows > permission.geo.label = Access Your Location > permission.indexedDB.label = Maintain Offline Storage > permission.fullscreen.label = Enter Fullscreen > permission.pointerLock.label = Hide the Mouse Pointer >- >+permission.push.label = Receive Push Notifications I don't think this string makes sense. If you look at the other strings, you'll notice that it's supposed to describe what a web site can do. But web sites don't receive those notifications, they send them, as I understand it. (It's great to see how little you had to do for SitePermissions.jsm and slightly disturbing to see the about:permissions stuff next to it. about:permissions is half-abandoned and I hope we can get rid of it soon.)

(In reply to Dão Gottwald [:dao] from comment #16) > Comment on attachment 774196 [details] [diff] [review] > Page Info for Push permissions > > >--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties > >+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties > >@@ -11,9 +11,9 @@ permission.cookie.label = Set Cookies > > permission.desktop-notification.label = Show Notifications > > permission.image.label = Load Images > > permission.install.label = Install Add-ons > > permission.popup.label = Open Pop-up Windows > > permission.geo.label = Access Your Location > > permission.indexedDB.label = Maintain Offline Storage > > permission.fullscreen.label = Enter Fullscreen > > permission.pointerLock.label = Hide the Mouse Pointer > >- > >+permission.push.label = Receive Push Notifications > > I don't think this string makes sense. If you look at the other strings, > you'll notice that it's supposed to describe what a web site can do. But web > sites don't receive those notifications, they send them, as I understand it. > Webservers send the notification, but it is a web page (although 'invisible', see 868322 and 857464) in the browser that receives the notification.

(In reply to Nikhil Marathe [:nsm] from comment #17) > > >+permission.push.label = Receive Push Notifications > > > > I don't think this string makes sense. If you look at the other strings, > > you'll notice that it's supposed to describe what a web site can do. But web > > sites don't receive those notifications, they send them, as I understand it. > > > > Webservers send the notification, but it is a web page (although > 'invisible', see 868322 and 857464) in the browser that receives the > notification. If the web page is invisible to users, doesn't that make it an implementation detail? I'm not sure users will understand what you're trying to tell them here.

Comment on attachment 774196 [details] [diff] [review] Page Info for Push permissions Review of attachment 774196 [details] [diff] [review]: ----------------------------------------------------------------- The code looks fine (though I'm not currently in a position to test it). I agree with Dao that the language might need some tweaks (see below). ::: browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd @@ +42,5 @@ > <!ENTITY popup.label "Open Pop-up Windows"> > > <!ENTITY fullscreen.label "Fullscreen"> > + > +<!ENTITY push.label "Receive Push Notifications"> I agree with Dao that from a user's perspective, this might be a bit confusing. From a user's perspective, websites are not given permission to *receive* push notifications - they're given permission to *show* push notifications. Perhaps "Show Push Notifications" would be a little clearer for the average user. ::: browser/locales/en-US/chrome/browser/sitePermissions.properties @@ +15,5 @@ > permission.geo.label = Access Your Location > permission.indexedDB.label = Maintain Offline Storage > permission.fullscreen.label = Enter Fullscreen > permission.pointerLock.label = Hide the Mouse Pointer > +permission.push.label = Receive Push Notifications Same as above.

One reason I'm shying away from "Show Push Notifications" is that its similar to "Show Desktop Notifications", but both have completely different purposes. A push notification receiver is like a 'background service' in native apps. Granted the web doesn't have a good equivalent yet so users may not be aware of it. In such a scenario, what would be the best way to ask for permission from the user without confusing anyone too much? How about "Allow background activity?" or similar. :dolske also wished to add some icon or similar when such background activity is happening (i.e. a page is loaded in the background since it got a notification).

I'm happy to do a ui review here, but I'll need a screenshot or (even better) a build to try. Thanks!