According to bug 1193006 comment 19, UX decide to keep all options in the Permissions tab of Page Info panel, but want hide 'set cookies', 'install add-ons' and 'load images' permissions in the Permissions section of the Control Center

Hi Paolo, To hide the specific permissions in the Permissions section of the Control Center,I'd like to take the following approach: 1. add an optional filter parameter in SitePermissions.listPermissions method, https://dxr.mozilla.org/mozilla-central/source/browser/modules/SitePermissions.jsm#29 2. add 'showInControlCenter' properties in each items of gPermissionObject https://dxr.mozilla.org/mozilla-central/source/browser/modules/SitePermissions.jsm#149 3. then do listPermissions('showInControlCenter') to filter and render permissions with 'showInControlCenter' property https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7111 Does it sounds reasonable to you? Or any suggestions would be great.

Review commit: https://reviewboard.mozilla.org/r/51841/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51841/

The patch as proposed above is relatively simple so I made the PR and add related tests

Comment on attachment 8751119 [details] MozReview Request: Bug 1271868 - opt-out permissions in the Permissions section of the Control Center; r?paolo I'm fine with this approach, this makes the module more front-end related which might actually be good since its only customer is currently the Firefox front-end. I don't know if there are different plans here. Matt, do you think we can go ahead with this change and eventually move the SitePermissions module to the "browser" folder? Most of the platform calls are already available through nsIPermissionManager. Is there any plan to reuse the module for Android in the short term? I'll do a more detailed code review later.

Comment on attachment 8751119 [details] MozReview Request: Bug 1271868 - opt-out permissions in the Permissions section of the Control Center; r?paolo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51841/diff/1-2/

Comment on attachment 8751119 [details] MozReview Request: Bug 1271868 - opt-out permissions in the Permissions section of the Control Center; r?paolo add back feedback request from comment 4

https://reviewboard.mozilla.org/r/51841/#review49037 (In reply to :Paolo Amadini from comment #4) > I don't know if there are different plans here. Matt, do you think we can go > ahead with this change and eventually move the SitePermissions module to the > "browser" folder? AFAICT it's already in the browser/ folder… ::: browser/modules/SitePermissions.jsm:135 (Diff revision 2) > /* Holds permission ID => options pairs. > * > * Supported options: > * You didn't update this comment ::: browser/modules/SitePermissions.jsm:177 (Diff revision 2) > "desktop-notification": { > exactHostMatch: true, > labelID: "desktop-notification2", > + showInControlCenter: true > }, > Personally I would prefer an opt-out approach over opt-in as showing in CC should be the default as it's the most common.

(In reply to agrigas from bug 1193006 comment #19) > > Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions > > should not be shown in permissions doorhanger, should we hiding those > > options in the Permissions tab of Page Info panel? Or provide new images for > > them to show on doorhanger > > They should remain only in the Page info panel and not on the permissions > dropdown of the control center. Why are those three different and why those three specifically? If the control center is *the* place to be in control of site permissions then why exclude these? Adding special cases just adds complexity and I don't yet see the benefit for this special case.

Comment on attachment 8751119 [details] MozReview Request: Bug 1271868 - opt-out permissions in the Permissions section of the Control Center; r?paolo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51841/diff/2-3/

change to opt-out approach and add docs. Wait UX feedback to see if this patch is still needed.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #8) > (In reply to agrigas from bug 1193006 comment #19) > > > Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions > > > should not be shown in permissions doorhanger, should we hiding those > > > options in the Permissions tab of Page Info panel? Or provide new images for > > > them to show on doorhanger > > > > They should remain only in the Page info panel and not on the permissions > > dropdown of the control center. > > Why are those three different and why those three specifically? If the > control center is *the* place to be in control of site permissions then why > exclude these? Adding special cases just adds complexity and I don't yet see > the benefit for this special case. When we decided this it was because changing cookie settings could have consequences the user may not understand that impact their experience in a significant way and could cause breakage thus we decided to leave it out. For load images, we were told that that permission was barely ever used -sub 1% and we wanted to be mindful to only have permissions surfaced that we felt there was a high chance a user would want to manage/access them on a site by site basis. Load images we felt made more sense to be in about: prefs as a global since it is something users most likely would want to apply across their browsing experience and not have to set on a site by site basis. Install add-ons is something also we thought most users don't want to set on a per site basis often and with add-on signing and a re-designed flow and management, it seemed better for it to be handled with the initial warning - 'do you want to allow X site to install this add-on' versus making the control to preemptively block the request to install it accessible to the average user. Its still available in the Page info area for a more advanced user that understands the value of it.

(In reply to agrigas from comment #11) > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #8) > > (In reply to agrigas from bug 1193006 comment #19) > > > > Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions > > > > should not be shown in permissions doorhanger, should we hiding those > > > > options in the Permissions tab of Page Info panel? Or provide new images for > > > > them to show on doorhanger > > > > > > They should remain only in the Page info panel and not on the permissions > > > dropdown of the control center. > > > > Why are those three different and why those three specifically? If the > > control center is *the* place to be in control of site permissions then why > > exclude these? Adding special cases just adds complexity and I don't yet see > > the benefit for this special case. > > When we decided this it was because changing cookie settings could have > consequences the user may not understand that impact their experience in a > significant way and could cause breakage thus we decided to leave it out. My understanding from the spec[1] is that we're not going to allow a user to change permissions from CC, only revert to defaults, so there isn't a risk of introducing breakage, only fixing breakage which is why I think it should be included. When a user is reporting an issue with a site, the control center IMO they should see all of the permissions that could be causing the issue. > For load images, we were told that that permission was barely ever used -sub > 1% and we wanted to be mindful to only have permissions surfaced that we > felt there was a high chance a user would want to manage/access them on a > site by site basis. Load images we felt made more sense to be in about: > prefs as a global since it is something users most likely would want to > apply across their browsing experience and not have to set on a site by site > basis. My understanding is that we will continue to only show non-default permissions so not excluding it wouldn't surface it to the users who don't use the feature. The low usage of the feature adds to my argument that we shouldn't add a special case for something used so little. > Install add-ons is something also we thought most users don't want to set on > a per site basis often and with add-on signing and a re-designed flow and > management, it seemed better for it to be handled with the initial warning - > 'do you want to allow X site to install this add-on' versus making the > control to preemptively block the request to install it accessible to the > average user. Its still available in the Page info area for a more advanced > user that understands the value of it. Again, not sure why you're talking about "setting" permissions as my understanding is that the CC section is only for reverting non-default permissions with the [X]. Are we actually going to allow users to set allow/deny from CC with the new design? If not, I think most of your arguments aren't relevant. [1] https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit#heading=h.67igcuwi6ha4

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #12) > Are we actually going to allow users to set allow/deny from CC with the new design? We will only have the X to revert the decision. I'll note that regardless of showing or hiding these specific permissions in the Control Center, if we go for an opt-out approach to permissions in the Control Center we will probably have to handle the case where a rarely used permission (current or future) has a label, but there is no associated icon. With this in mind, I don't think that solving this bug is urgent. At present we can still display these permissions with no associated SVG icon. This will only happen in the rare cases where they are actually different than the default for the website.

Comment on attachment 8751119 [details] MozReview Request: Bug 1271868 - opt-out permissions in the Permissions section of the Control Center; r?paolo Clearing review request for now.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #12) > (In reply to agrigas from comment #11) > > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #8) > > > (In reply to agrigas from bug 1193006 comment #19) > > > > > Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions > > > > > should not be shown in permissions doorhanger, should we hiding those > > > > > options in the Permissions tab of Page Info panel? Or provide new images for > > > > > them to show on doorhanger > > > > > > > > They should remain only in the Page info panel and not on the permissions > > > > dropdown of the control center. > > > > > > Why are those three different and why those three specifically? If the > > > control center is *the* place to be in control of site permissions then why > > > exclude these? Adding special cases just adds complexity and I don't yet see > > > the benefit for this special case. > > > > When we decided this it was because changing cookie settings could have > > consequences the user may not understand that impact their experience in a > > significant way and could cause breakage thus we decided to leave it out. > > My understanding from the spec[1] is that we're not going to allow a user to > change permissions from CC, only revert to defaults, so there isn't a risk > of introducing breakage, only fixing breakage which is why I think it should > be included. When a user is reporting an issue with a site, the control > center IMO they should see all of the permissions that could be causing the > issue. > > > For load images, we were told that that permission was barely ever used -sub > > 1% and we wanted to be mindful to only have permissions surfaced that we > > felt there was a high chance a user would want to manage/access them on a > > site by site basis. Load images we felt made more sense to be in about: > > prefs as a global since it is something users most likely would want to > > apply across their browsing experience and not have to set on a site by site > > basis. > > My understanding is that we will continue to only show non-default > permissions so not excluding it wouldn't surface it to the users who don't > use the feature. The low usage of the feature adds to my argument that we > shouldn't add a special case for something used so little. > > > Install add-ons is something also we thought most users don't want to set on > > a per site basis often and with add-on signing and a re-designed flow and > > management, it seemed better for it to be handled with the initial warning - > > 'do you want to allow X site to install this add-on' versus making the > > control to preemptively block the request to install it accessible to the > > average user. Its still available in the Page info area for a more advanced > > user that understands the value of it. > > Again, not sure why you're talking about "setting" permissions as my > understanding is that the CC section is only for reverting non-default > permissions with the [X]. > > Are we actually going to allow users to set allow/deny from CC with the new > design? If not, I think most of your arguments aren't relevant. > > [1] > https://docs.google.com/document/d/143nEfWfIvFZD2- > pFqE8GD85inFjFIhE2J8QeegQufc0/edit#heading=h.67igcuwi6ha4 Paolo answered this as well - the new design user's can clear out what they have set and will be asked again by the site so its not reverting to the default per say but rather if they have set allow and click the X they'll be asked if they want to allow or block again... For the advanced user that goes to the page info dialogue to block cookies for one specific site, they should know how to get back their to allow them as well... Cookies feels different to me then most notifications because we don't ask you permission to store them so its not something most people are probably that aware of compared to other permissions we ask with a notification...

(In reply to :Paolo Amadini from comment #13) > With this in mind, I don't think that solving this bug is urgent. Marking for re-triage as I think it's clear this isn't P2. I still think we should WONTFIX so we have a consistent experience for power users which will also allow users to fix problems with sites easier thus reducing lost users and support time. (In reply to agrigas from comment #15) > For the advanced user that > goes to the page info dialogue to block cookies for one specific site, they > should know how to get back their to allow them as well... Not if it was changed by an extension… but still why not just keep these consistent with the others so users can self-correct for issues and be in control while in control center…

Discussed in triage and since we only show non-default permissions in the control center this isn't going to add noise for the majority of users who don't change these settings.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #17) > Discussed in triage and since we only show non-default permissions in the > control center this isn't going to add noise for the majority of users who > don't change these settings. For the users who do change the settings, do the controls work? (i.e changing from blocked cookies to allowed cookies for a certain domain)?

Yes, of course. That's handled at the permission manager itself.

The spec still have some 'WILL NOT SHOW IN CONTROL CENTER' section. Please help update the spec accordingly https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/ Thanks

And it will be helpful if you can turn the google doc share permission from `view only` to `allow comment`, then we can have more efficient spec check by context :)

Updated - and google doc can now take comments.

thanks!