+++ This bug was initially created as a clone of Bug #775857 +++ Same as bug 775857, but for the "Page Info" -> Permissions tab.

This is a work-in-progress patch very similar to that in bug 775857. I don't really know who to ask for feedback on this, but maybe Dao would have some insight or know who else to ask. Thanks!

Comment on attachment 705092 [details] [diff] [review] patch Review of attachment 705092 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/pageinfo/pageInfo.xml @@ +40,5 @@ > + <xul:radio anonid="allow" label="&permAllow;"/> > + <xul:radio anonid="block" label="&permBlock;"/> > + </xul:radiogroup> > + </xul:hbox> > + </content> Adding more XBL goop for this small bit of UI seem like overkill, and furthers our technical debt. I think you'd be better off moving the JS into pageinfo.js, and have a function to construct the UI via createElement() calls. Or maybe stuff it into a pageinfo.xul as a hidden "template", which you then hbox.cloneNode(true) to duplicate and customize for each row.

Comment on attachment 705092 [details] [diff] [review] patch Ok - sounds good. Clearing review for now.

Now with 50% less goop!

Comment on attachment 707280 [details] [diff] [review] patch v2 Review of attachment 707280 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/pageinfo/pageInfo.xul @@ +376,5 @@ > <vbox class="permission" id="permPluginsRow"> > <label class="permissionLabel" id="permPluginsLabel" > value="&permPlugins;" control="pluginsRadioGroup"/> > + <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline"> > + <label/> Why does align="baseline" need to be here? ::: browser/base/content/pageinfo/permissions.js @@ +239,5 @@ > } > } > + > +// XXX copied this from browser-plugins.js - is there a way to share? > +function makeNicePluginName(aName) { Can you add this as a method on nsIPluginHost? @@ +269,5 @@ > + let permissionEntry = permPluginTemplate.cloneNode(true); > + let permString = pluginHost.getPermissionStringForType(mimeType); > + permissionEntry.setAttribute("permString", permString); > + // XXX this whole "childNodes[i]" business is dumb - is there a better way? > + let label = permissionEntry.childNodes[0]; Using a class would be easier to read and would also allow reordering without breaking this code. @@ +275,5 @@ > + let radioGroup = permissionEntry.childNodes[2]; > + radioGroup.setAttribute("id", permString + "RadioGroup"); > + radioGroup.childNodes[0].setAttribute("id", permString + "#0"); > + radioGroup.childNodes[1].setAttribute("id", permString + "#1"); > + radioGroup.childNodes[2].setAttribute("id", permString + "#2"); Can these be changed from IDs to data-permString attributes? Then we shouldn't need to split the attribute value later just to get back the permstring.

(In reply to Jared Wein [:jaws] from comment #6) > ::: browser/base/content/pageinfo/pageInfo.xul > @@ +376,5 @@ > > <vbox class="permission" id="permPluginsRow"> > > <label class="permissionLabel" id="permPluginsLabel" > > value="&permPlugins;" control="pluginsRadioGroup"/> > > + <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline"> > > + <label/> > > Why does align="baseline" need to be here? This makes the children's text labels all line up nicely. > ::: browser/base/content/pageinfo/permissions.js > @@ +239,5 @@ > > } > > } > > + > > +// XXX copied this from browser-plugins.js - is there a way to share? > > +function makeNicePluginName(aName) { > > Can you add this as a method on nsIPluginHost? Filed bug 836841. > @@ +275,5 @@ > > + let radioGroup = permissionEntry.childNodes[2]; > > + radioGroup.setAttribute("id", permString + "RadioGroup"); > > + radioGroup.childNodes[0].setAttribute("id", permString + "#0"); > > + radioGroup.childNodes[1].setAttribute("id", permString + "#1"); > > + radioGroup.childNodes[2].setAttribute("id", permString + "#2"); > > Can these be changed from IDs to data-permString attributes? Then we > shouldn't need to split the attribute value later just to get back the > permstring. The problem is that these ids need to be "[whatever permission value]#0" (or 1 or 2) to be interoperable with the rest of the code without serious modifications to how this whole thing works. So, rather than having another attribute on the radio buttons, I figured it would be easier to just use split to get the permString back.

Ignoring what would change when bug 836841 gets fixed, could you have another look at this? I'm hoping to get this uplifted to aurora before the merge (but if that doesn't happen, it's not the end of the world).

Comment on attachment 708798 [details] [diff] [review] patch v3 Review of attachment 708798 [details] [diff] [review]: ----------------------------------------------------------------- This also needs to update about:permissions. Can you upload a screenshot of what this looks like? ::: browser/base/content/pageinfo/pageInfo.xul @@ +375,5 @@ > </vbox> > <vbox class="permission" id="permPluginsRow"> > <label class="permissionLabel" id="permPluginsLabel" > value="&permPlugins;" control="pluginsRadioGroup"/> > + <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline"> The align="baseline" should be moved to CSS. ::: browser/base/content/pageinfo/permissions.js @@ +256,5 @@ > +function fillInPluginPermissionTemplate(aPluginName, aPermissionString) { > + let permPluginTemplate = document.getElementById("permPluginTemplate"); > + permPluginTemplate.setAttribute("permString", aPermissionString); > + let attrs = [ > + [ ".permPluginTemplateLabel", "value", aPluginName ], This will make the entry "Adobe Flash", but it would be better if it was "Activate Adobe Flash".

I am concerned about the storage mechanism being used here, which appears to be keyed on the plugin name. I do not think we should store any data based on plugin names, which can change in unpredictable ways and can contain version numbers. I understand that there is code which tries to strip out the versions, but that is guesswork at best. The only thing which is constant about plugins is their MIME type, and we should try to key any sort of plugin permission off of the MIME type. See bug 830267 comment 4 for related issues with the addon manager disabled/enabled state so that we can correctly implement bug 549697.

(In reply to Jared Wein [:jaws] from comment #9) > This also needs to update about:permissions. That's being handled in bug 775857. > Can you upload a screenshot of what this looks like? Sure thing - see pageInfo.png > The align="baseline" should be moved to CSS. I understand what you're saying, but is that really necessary? It takes something that is accomplished in one line and spreads it out to multiple lines in three other files in other directories (it would need to be in browser/themes/whatever/pageInfo.css, right?). I understand separating the styling from the structure, but I don't think we'll ever want the text to be unaligned, so there's no reason to style that differently. > This will make the entry "Adobe Flash", but it would be better if it was > "Activate Adobe Flash". Maybe the screenshot will explain why I did it this way.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > I am concerned about the storage mechanism being used here, which appears to > be keyed on the plugin name. I do not think we should store any data based > on plugin names, which can change in unpredictable ways and can contain > version numbers. I understand that there is code which tries to strip out > the versions, but that is guesswork at best. > > The only thing which is constant about plugins is their MIME type, and we > should try to key any sort of plugin permission off of the MIME type. See > bug 830267 comment 4 for related issues with the addon manager > disabled/enabled state so that we can correctly implement bug 549697. The use of the plugin names is just for display purposes - all storage of user preferences is handled by the permission manager using pluginHost.getPermissionStringForType as the key (which uses the mime type).

Jared - does the screenshot look okay? If so, are you okay with the rest of the changes in the patch? (basically, I'm asking r? again) (see also comment 12)

(In reply to David Keeler (:keeler) from comment #14) > Jared - does the screenshot look okay? If so, are you okay with the rest of > the changes in the patch? (basically, I'm asking r? again) (see also comment > 12) Yeah, I'll take a look at it tomorrow.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > I am concerned about the storage mechanism being used here, which appears to > be keyed on the plugin name. I do not think we should store any data based > on plugin names, which can change in unpredictable ways and can contain > version numbers. I understand that there is code which tries to strip out > the versions, but that is guesswork at best. > > The only thing which is constant about plugins is their MIME type, and we > should try to key any sort of plugin permission off of the MIME type. See > bug 830267 comment 4 for related issues with the addon manager > disabled/enabled state so that we can correctly implement bug 549697. Oh - I think I might see what you're saying. The permission key returned by pluginHost.getPermissionStringForType should really use a canonical MIME type for the plugin it maps to, not the plugin filename as it currently does. This issue seems familiar - is there a bug on it?

Other than bug 830267 which is not directly about the API, no.

Filed bug 838290.

We still need to have per-plugin "activate always" in the popup notification for this to be a worthwhile pursuit. Most users won't find the Page Info dialog, so it can't be the only place to set this.

Agreed - this isn't a complete solution to managing plugin permissions. However, due to the changes from bug 746374, what was previously a relief valve for people who needed to reset their plugin permissions (i.e. Page Info -> Permissions) no longer works. At a bare minimum, we need to be able to point people to this mechanism on support articles, etc. So, this is definitely worthwhile.

Comment on attachment 708798 [details] [diff] [review] patch v3 Review of attachment 708798 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but please file a bug to add/figure-out the changes needed for the popup-notification.

Thanks, Jared! I think bug 838106 should cover any per-plugin changes necessary in the popup notification. Try run: https://tbpl.mozilla.org/?tree=Try&rev=0af1ddfe0c2b

Wrong try options. Hopefully this is right: https://tbpl.mozilla.org/?tree=Try&rev=55f129007576

Had to fix up the tests a bit. Carrying over r+. Try run looked good: https://tbpl.mozilla.org/?tree=Try&rev=07ee55ba0d30 Checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/f30b42547a84

Comment on attachment 712326 [details] [diff] [review] patch v3.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 746374 User impact if declined: users cannot undo permissions set by the click-to-play popup notification Testing completed (on m-c, etc.): landed on m-c Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

Bug 746374 didn't land on esr17, so I believe it is unaffected by this bug.

Comment on attachment 712326 [details] [diff] [review] patch v3.1 low risk - will approve bug 839193

Thanks, Lukas! https://hg.mozilla.org/releases/mozilla-aurora/rev/a12a304c16af Also, the bug that caused this landed on 20, so 18 and 19 aren't affected.

Do we really need click-to-play be able to be individually set for a page? Are we sure there's real user value here? After all, there's some consistency problems present in a temporary fix here. The temporary nature of having both about:permissions and about:addons puts us in a strange position with specific regard to addons-related-to-permissions. These controls should be accessible in only one place once we get combined Preferences under control and consolidated, but I realize that could be a ways off and if we feel that per-site-click-to-play is important, we'll need a temporary solution. But, if we make that temporary solution accessible in both about:permissions and about:addons, we're reflecting a "global" state of click-to-play in about:addons which may not be accurate if users have changed click-to-play settings for individual pages. That's the big drawback. Why not instead have only global click-to-play for now, which can be viewed in both "All Sites" in about:permissions and on individual extensions in the addons manager? Then we get a neat 1:1 correlation: click-to-play can be changed in either place and changes the one the user isn't looking at.

Per-site click-to-play is important. My suggestion in bug 838106 was to expose the per-site settings via the "Options" button in the addon manager.

I see all possible plugins listed (even the ones that aren't used) for a specific domain (eg. youtube). Is this ok? Wouldn't be better to list only the used plugins?

Well, I think it's difficult to accurately say what plugins a domain uses, and even if it were per-page, the plugins a page uses can change easily (in theory, if not in practice), so it's better to just say, "Here's all your plugins; this page may or may not use them all, but you can set their permissions to whatever you want". (For comparison, the dialog has an images section whether or not the page actually has any images on it.)

Looks odd to me a list with 30 plugins on a page that doesn't use any of them (google.com).