Currently, when managing plugins, you can only have a plugin fully-enabled or fully-disabled. It would be useful to have an intermediate click-to-start form (aka Flashblock, but for all plugins). This would help people to wean themselves off plugins, by having them available on pages where they're desired but disabled on other pages.

Yes, please! My current "go enable flash any time I want to use it" use pattern is moderately annoying. ;)

I was thinking of this same use case when I filed bug 495840 as well. The way Flashblock is currently implemented is a hack, it'd be nice to have real platform support for doing it right.

Do we need a dummy plugin that is loaded all the time (with flashblock-like functionality) for this or can this be done in the browser?

Flashblock is an extension, so there shouldn't be any problem at all integrating its functionality in the browser (or something that functions like it does).

I've been using NoScript as a Flashblock replacement (scripts allowed globally, just blocking plugins) and it's really useful. It blocks CPU-intensive Flash adverts, and allows you to open a load of YouTube videos in new tabs without them all starting and playing over each other. It also stops your computer unexpectedly coming to a crawl when you occasionally come across a page with a Java applet. It doesn't download the objects if you don't click-to-start them, saving bandwidth too. I think it's a good place to start when looking at a possible UE for this feature.

If we do this we should be able to whitelist certain plugins and domains.

(In reply to Christian Legnitto [:LegNeato] from comment #8) > If we do this we should be able to whitelist certain plugins and domains. Yes. The policy from the UX team is currently: - Allow Flash by default (but allow it to be click-to-play in prefs) - Make any other plugin click-to-play by default (especially Java because of security and performance hit) - If you've activated a click-to-play plugin on a certain URL 3 times, make it activate on subsequents loads *of that page* (to handle the intranet/bank Java app use case) - Make any plugin capable of being in one of three modes: always on / click to activate / always off.

…and the way to clear the "click to play -> automatic after 3 times" is to change the state of that plugin to either always on or always off and then back to click-to-play. This avoids having a management interface for the list of URLs that will always load. :P

What about non-visible plugin embeddings?

(In reply to [Baboo] from comment #11) > What about non-visible plugin embeddings? An info bar or door hanger notification.

I think we should also include a notice when there are known security problems in the plugin and whether there is an update available that addresses them. Also, if there is a user pref to control this behavior, then we would need some mechanism to say that certain versions of certain plugins need to be click-to-play regardless of the pref (for when there are serious security vulnerabilities).

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #10) > …and the way to clear the "click to play -> automatic after 3 times" is to > change the state of that plugin to either always on or always off and then > back to click-to-play. This avoids having a management interface for the > list of URLs that will always load. :P When a page is removed from the history (and isn't bookmarked) it should also be removed from this list.

For whitelisting I meant Firefox should also carry a default list. For example, Facebook video chat should whitelist java, pogo.com as well, apple.com for QuickTime, etc.

Attaching work-in-progress patches. I've split this into 4 main parts: * Platform bits - plugins, content, dom, etc. I'm over my head on this, think I'll need someone familiar with plugins/content to finish this part. * Addons Manager bits. This is the most complete part so far. * Browser bits. * Whitelist & defaults. I purposefully haven't started this part yet, and am leaving it until last.

(These should apply in any order.)

needs a full sec-team review

Re comment #12: How would a doorhanger or info bar work where there are more than one instances of a call to a plugin in the same Web page? I have seen several pages with multiple Flash presentations. Even Adobe's own Flash test page at <http://www.adobe.com/software/flash/about/> has three. There, the Flash plugin is automatically launched as part of rendering the Web page. How would a user select which Flash? How would this work where a plugin is launched by selecting a URI, a link to another Web resource? For example, see the bottom of my own <http://www.rossde.com/music.html>, where the 13 speaker icons are links to streaming audio URIs for RealPlayer or Winamp plugins. There, a plugin is not launched until the user selects a link. One size does not fit all. As noted above, some plugins are launched automatically; and some are launched by a user request.

(In reply to David E. Ross from comment #22) > Re comment #12: How would a doorhanger or info bar work where there are > more than one instances of a call to a plugin in the same Web page? There's really no way for a user to differentiate between different non-visible plugin usages so I think it might makes sense to have one prompt per different plugin per page and click-to-start for each individual visible plugin usage. I know the info bar is fine with having multiple at once but I'm not sure about doorhangers. If they don't have a way to handle it then I think it adding it for this might be worth it.

Pausing all Flash on a page can break some sites that use non-visible Flash (0 or 1 pixel sized that Adobe calls "helper SWFs"). Non-visible helper SWFs often execute ActionScript needed by other SWFs (or JavaScript like gmail's attachment uploader) on the same page. If a dependent SWF is Click To Played but its helper SWFs are not, then the Flash content can get stuck in a bad state. I suggest that Flash Click To Play either: 1. Allow ALL helper SWFs to run when a page is loaded 2. Or unpause ALL helper SWFs when ANY visible Flash on the page is Click To Played. Here is an Adobe KB article about helper SWFs: http://help.adobe.com/en_US/as3/mobile/WS4bebcd66a74275c36cfb8137124318eebc6-8000.html

Comment on attachment 564418 [details] [diff] [review] WiP v1 - plugin bits The patch I'm working on in bug 707886 will make this patch obsolete (it was already pretty severely bit-rotted by changes from bug 687265 anyway). That patch mostly achieves what this patch was trying to, although it doesn't add the parts to help support enabling click-to-play on a per-plugin basis. Since we don't need that for mobile, I think it should just be handled in a follow-up.

Once the tree opens again I'm going to land bug 707886, which implements most of the functionality from the WIP plugin patch in here. I'm attaching an updated version of Blair's WIP that I was using to test on desktop, and it will work with the platform patch from bug 707886.

I've spun off the browser bits (desktop ui, really) into bug 711552.

Hong, make sure your team is aware of this bug and the bugs in the "Blocks" and "Depends on" list, and request any changes/clarifications to the feature ASAP. This feature will affect almost any site that uses plugins.

hi guys i have enable plugins.click_to_play (on win/linux) it seems it blocks the plugin fine but when i click on the tap to play nothing happens. Have to manually switch off plugins.click_to_play to work with plugins. So any ideas what going on & when will this be fixed?

(In reply to beelzebub360 from comment #29) > hi guys > i have enable plugins.click_to_play (on win/linux) > it seems it blocks the plugin fine but when i click on the tap to play > nothing happens. > Have to manually switch off plugins.click_to_play to work with plugins. > So any ideas what going on & when will this be fixed? The desktop front-end part of this feature hasn't been implemented yet (the platform support is there, but the feature is only implemented on mobile for now). The desktop work is happening in bug 711552.

how is it going to work? it will tell the website that the plugin is not installed until user clicks the click-to-play container? or the site will get info that the browser has this plugin installed, but the plugin won't start functioning until the user clicks the click to play container? I'd prefer the 1st approach, so some sites could do a clever check: imagine if youtube would first check whether your browser supports html5 video with the codec used for the video or not and if not - only then it would suggest you to install flash, to be able to watch the video. However, that approach would require the page to be reloaded after the user click the click-to-play container, as page's contents may be dependent on the information about the plugins installed on the user's browser. Could someone please shed some light on that?

(In reply to Sean Newman from comment #31) > how is it going to work? it will tell the website that the plugin is not > installed until user clicks the click-to-play container? > or the site will get info that the browser has this plugin installed, but > the plugin won't start functioning until the user clicks the click to play > container? > > I'd prefer the 1st approach, so some sites could do a clever check: imagine > if youtube would first check whether your browser supports html5 video with > the codec used for the video or not and if not - only then it would suggest > you to install flash, to be able to watch the video. > However, that approach would require the page to be reloaded after the user > click the click-to-play container, as page's contents may be dependent on > the information about the plugins installed on the user's browser. > > Could someone please shed some light on that? Sean, there's some discussion of the plans/open issues including site detection of plugin availability at https://wiki.mozilla.org/Opt-in_activation_for_plugins and also on mozilla.dev.security

Thanks for this! too i also have traded Flashblock for plugins.click_to_play;true But by when it will be stable enough to test? will it have the option to control(enable on demand) for all types of plugins? & what about say a page has two flash player windows but want to play only the second one will it be possible ? (flashblock allows this)

Per discussion with curtisk, for click to play security reviews, we're going to track the click to play meta bug (bug 738698) and its blocking review bug (bug 744534)

(In reply to magnumarchonbasileus from comment #33) > Thanks for this! too > > i also have traded Flashblock for plugins.click_to_play;true > > But by when it will be stable enough to test? > will it have the option to control(enable on demand) for all types of > plugins? > & what about say a page has two flash player windows but want to play only > the second one > will it be possible ? (flashblock allows this) See https://wiki.mozilla.org/Opt-in_activation_for_plugins which covers a large range of users and use cases In the 'implementation plan' section it also talks about phases in the evolution of click to play - the feature is also still being discussed. Also, bug 738698 is the meta ('overall') bug for click to play/opt in activation of plugins.

Bug 775857 depends on having a way to make individual plugins click-to-play or not, which I think is the goal of this bug (and if it isn't, well, it is now).

Blair - I know you're probably unavailable, but I'll mark you as reviewer as a placeholder until I find someone else or you have time to review this. Thanks!

For all plugins which are not Flash, please go ahead and make click-to-play the default setting as part of this patch.

First off, i applaud this direction: whitelisting. Can I request an enhancement? Let the user self-manage global permits. With the existing implementation, whitelisting is per site. It can quickly become annoying to repetitively enable a plugin across site after site after site. So, let the user apply a global permit to a specific plugin - across all sites. If they later want to remove this permission, let them deselect a checkbox within the add-ons manager. This seems much more fair to both users and vendors - i.e. not favoring flash over silverlight; or the integrated pdf reader over adobe's pdf plugin. Empowering the user has always been a cornerstone of Firefox. It would be nice to see this mantra maintained and not make usability decisions for the user. Please let me know your thoughts. Thnx! -Mark

Whitelisting individual plugins is bug 820054.

Thnx! With all the news the past few days on this defaulted to on, I didn't find that one.

The reason, i would want such a thing is that sometimes I only want to see the stuff once, so I would like to turn it off. Animations can be annoying sometimes. So, there might need to be another option like "Run only once". Not sure how that would work.

Updating patch to latest version and switching review to Mossop.

Comment on attachment 711930 [details] [diff] [review] patch updated Finally able to go through my review queue again, so stealing this back since I've already put a lot of thought into this.

Comment on attachment 711930 [details] [diff] [review] patch updated Review of attachment 711930 [details] [diff] [review]: ----------------------------------------------------------------- Some high-level stuff to sort out first, mostly relating to differences from attachment 564419 [details] [diff] [review]: The UI feels unnecessarily complex, due to the checkbox shows only when the plugin is enabled. There are essentially 3 states we support (disabled, ask me, enabled), but the checkbox separates out click-to-play as a completely independent state. It makes it so there's two pieces of information to look for, and more than one action needed to change the state (check enabled state, click enable, check click-to-play-state, click checkbox). Instead, it should just be exposed as 3 mutually exclusive states that are related - as I alluded to above, the old patch did this :) (But if there's been some UX discussion explicitly on this, I'd love to hear it!) The UI here is using the term "click to play", but I don't think we're exposing that terminology anywhere else (please correct me if I'm wrong) - everything seems to be "play this" or "activate" or something like that. Also, the UI in about:permissions words click-to-play as "Always Ask" - we should have some consistency with that (whether its changing the wording here or there). In terms of Add-ons Manager API design (ignoring UI, nsPluginHost, nsPluginTag, etc), wherever possible we try to avoid having anything specific to one provider/one type of add-on (which leads to each provider reinventing the wheel). "Click-to-play" is very tied to how plugins implement the way to ask each time, so the API names should be more generic and phrased as "always ask" or something. Also, the provider should advertise that the "plugin" add-on type supports an "ask me" state (via a flag when registering the type), since it's not standard to all types. Relying on a permission flag being present only sometimes is too fragile for API consumers - especially since it currently breaks convention with other permissions flags that describe a possible state change, since it uses one flag for 3 states (not available, clicktoplay enabled, clicktoplay disabled).

Thanks for the review. I asked for some UX feedback, so I'll address your comments when I hear back from them.

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #46) > The UI feels unnecessarily complex, due to the checkbox shows only when the > plugin is enabled. There are essentially 3 states we support (disabled, ask > me, enabled), but the checkbox separates out click-to-play as a completely > independent state. It makes it so there's two pieces of information to look > for, and more than one action needed to change the state (check enabled > state, click enable, check click-to-play-state, click checkbox). Instead, it > should just be exposed as 3 mutually exclusive states that are related - as > I alluded to above, the old patch did this :) (But if there's been some UX > discussion explicitly on this, I'd love to hear it!) Wow, you found a major inconsistency in the add-ons manager: A disabled add-on still has a "Preferences" button, and yet (in this patch) no "Click to play" button. That Preferences button currently does nothing on a disabled add-on. We need to be consistent here on whether the preferences of disabled add-ons can be 1. viewed and 2. manipulated. Because the Preferences button is deactivated on disable, which seems more like a bug than a decision, I'm assuming that there are current technical barriers to exposing the Preferences of a disabled add-on. So, for now, let's be consistent (and easy) and hide both the Preferences and Click to play button on a disabled add-on. tl;dr recommendation: keep hiding "click to play" on disable, but also hide Preferences button on disable > The UI here is using the term "click to play", but I don't think we're > exposing that terminology anywhere else (please correct me if I'm wrong) - > everything seems to be "play this" or "activate" or something like that. > Also, the UI in about:permissions words click-to-play as "Always Ask" - we > should have some consistency with that (whether its changing the wording > here or there). Again, you've well-spotted another inconsistency. Since we can't readily allow users to disable an add-on from about:permissions (not that we'd want to, as the add-ons manager being the better tool), let's show the same checkbox in about:permissions as is in about:addons. This gives us a few ux benefits: • No need for near-but-not-quite consistency of a drop-down in about:permissions that doesn't include a disable/block state. All other plugins in about:permissions have this disable state, yet for reasons above we don't and probably shouldn't allow that functionality here, which is so covered in the addons manager. Currently, it looks like we've just forgotten the disable/block state in about:permissions, which won't exactly inspire security confidence in users • We can describe exactly what this thing does (click to play) rather than ask users to figure out that "Always ask" is not meant in the same way as arrow panel notification ask, but rather in the passive way that not-playing "asks," and be consistent in our usage of the phrase "Click to play" to boot tl;dr recommendation: replace the about:permissions dropdown with a click-to-play checkbox identical to the one in the add-ons manager. Continue removing disabled add-ons from this about:permissions list for now.

I don't understand the previous comment. Are you saying that we should *not* have a tri-state UI? If so, why not? Isn't that preferable to a single enable/disable switch and a separate click-to-play checkbox? We are actively avoiding the word "play" in relation to this feature elsewhere in the UI. We should at least call it "Click to activate" and not "play". I'm not sure why we need to be consistent about the "options" of disabled addons. Technically, we *can* show you the options for a disabled plugin, but cannot show you the options for a disabled addon. So let's keep the options button enabled where we can, and disable it when we technically can't?

Can't the options of disabled plug-ins/add-on's be shown as greyed out? So, you get both an indication of what they are and can change them and you also get the hint that the add-on/plug-in is disabled (because the info is grey).

Comment on attachment 711930 [details] [diff] [review] patch updated Review of attachment 711930 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginHost.cpp @@ +2045,5 @@ > // We have a valid new plugin so report that plugins have changed. > *aPluginsChanged = true; > + // Make all new non-Flash plugins click-to-play by default > + if (!pluginTag->mIsFlashPlugin) > + pluginTag->Mark(NS_PLUGIN_FLAG_CLICKTOPLAY); As i ran into the same thing already: This probably shouldn't be mixed into this, adressing the various places where we need to flip the defaults would be better in a separate patch.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #48) > Wow, you found a major inconsistency in the add-ons manager: A disabled > add-on still has a "Preferences" button Oh, that only happens for plugins (ugh, terminology) - its a known bug. Bug 806271 would fix it by making that button never show (plugins don't really have preferences - its an implementation detail being accidentally exposed). > I'm assuming that there are current technical > barriers to exposing the Preferences of a disabled add-on. Plugins don't have preferences (see above), but for add-ons yes this is a technical limitation. > Again, you've well-spotted another inconsistency. Since we can't readily > allow users to disable an add-on from about:permissions (not that we'd want > to, as the add-ons manager being the better tool), Actually, I wonder if we should consider doing that. It fits in with about:permissions if you consider disabling a plugin as "don't let any site use this", enabling a plugin a "always automatically let sites use this", and click-to-play as "ask me" - all of which can be seen as permissions, IMO. The Add-ons Manager is still there as a full management UI, which is more than just enabling/disabling.

Assuming that about:permissions consistency could be looked at separately, are there any blocking issues with the tri-state drop-down for this?

I'm concerned that a tri-state drop-down would be too much of a change both visually and, to a lesser extent, architecturally, but I'll look into it. The original patch had three buttons that were visible at all times, which, in my opinion, took up unnecessary space. The reasoning behind just adding a checkbox was I thought it would be the smallest change that could possibly work.

This is the patch with a tristate menu and the requested architecture changes (as I understand them).

Comment on attachment 716265 [details] [diff] [review] patch with tristate menu Review of attachment 716265 [details] [diff] [review]: ----------------------------------------------------------------- Reading through this (without really knowing the AddonManager though) and taking it for a short spin, this looks good to me :) I'm not a UI/UX person, but this seems to fit in with the other click-to-play elements. ::: dom/plugins/base/nsPluginHost.cpp @@ +2069,5 @@ > // We have a valid new plugin so report that plugins have changed. > *aPluginsChanged = true; > + // Make all new non-Flash plugins click-to-play by default > + if (!pluginTag->mIsFlashPlugin) > + pluginTag->Mark(NS_PLUGIN_FLAG_CLICKTOPLAY); Please remove that bit for now though?

Comment on attachment 716265 [details] [diff] [review] patch with tristate menu Review of attachment 716265 [details] [diff] [review]: ----------------------------------------------------------------- Over to Boriss first.

(In reply to David Keeler (:keeler) from comment #54) > The original patch had three buttons that were visible at all times Some background: I originally did that because shorlander had been experimenting with using an "on/off" set of connected buttons for add-ons, as part of the Australis refresh. But that was awhile ago, and I don't know the current status of that idea. Source: https://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-InContentUI-%28Addons-Manager%29.jpg (Req'ing info from shorlander for the status of that, regardless of whether we do it here and now.)

Try build for Boriss: https://tbpl.mozilla.org/?tree=Try&rev=1ca377c1766c Those connected buttons look nice! Maybe a tri-state [Always On][Ask Me][Always Off] thing would work for plugins?

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #57) > Comment on attachment 716265 [details] [diff] [review] > patch with tristate menu > > Review of attachment 716265 [details] [diff] [review]: > ----------------------------------------------------------------- > > Over to Boriss first. Tri-state is ok for now, but if this menu takes the place of previous enable/disable - and duplicates its functionality in the case of "Never Activate," the states should be "Ask to Enable," "Always Enable," and "Disable." Preferably "Remove" too, but one fish at a time.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60) > Tri-state is ok for now, but if this menu takes the place of previous > enable/disable - and duplicates its functionality in the case of "Never > Activate," the states should be "Ask to Enable," "Always Enable," and > "Disable." Preferably "Remove" too, but one fish at a time. It looks like we'll stay with the "activate" language for the other plugin activation UI (click-to-play), maybe it would make sense to match that?

Re comment #60: Plugins are generally installed independently and not by browsers or E-mail clients. Also, many browser plugins are also used by non-browser -- non-Mozilla -- applications. Thus, the suggestion that a "Remove" capability be implemented in Mozilla is highly questionable.

(In reply to Georg Fritzsche [:gfritzsche] from comment #61) > (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60) > > Tri-state is ok for now, but if this menu takes the place of previous > > enable/disable - and duplicates its functionality in the case of "Never > > Activate," the states should be "Ask to Enable," "Always Enable," and > > "Disable." Preferably "Remove" too, but one fish at a time. > > It looks like we'll stay with the "activate" language for the other plugin > activation UI (click-to-play), maybe it would make sense to match that? We should be consistent everywhere. Ideally, all of this language should be changed to enable/disable to be consistent with longer-running add-on language, both in the click-to-play UI and in the add-ons manager. I understand that may be hard to do quickly.

I think you and Shorlander and lco need to actually decide that. Perhaps the decision is something nuanced like "activate is used for single instances and enable is used for the plugin as a whole". But in any case IMO it wouldn't make sense for the in-page UI to say "click here to enable Adobe Flash" since it's not a permanent enabling decision.

Comment on attachment 716265 [details] [diff] [review] patch with tristate menu (In reply to Benjamin Smedberg [:bsmedberg] from comment #64) > I think you and Shorlander and lco need to actually decide that. Perhaps the > decision is something nuanced like "activate is used for single instances > and enable is used for the plugin as a whole". But in any case IMO it > wouldn't make sense for the in-page UI to say "click here to enable Adobe > Flash" since it's not a permanent enabling decision. I agree: this topic needs more thought, and going with "enable/disable" for the foreseeable future - particularly because you're correct that "activate" in the click-to-play sense isn't isomorphic to the current enable/disable functionality in the plugin and extension sense. And, thinking about this further, we're setting up strings as term here that, on such a new feature, we may be loathe to change quickly in the next few versions. "Activate" does make semantic sense for users who will read about and use this feature and aren't particularly familiar with the history of enable/disable in the add-ons manager. So, I'm going to ui-r+ this patch, with the caveat that this whole issue needs larger discussion regarding the language we use around add-ons, plugins, and click-to-activate (which has changed several times before this particular bug). For now, this is functional and the words "activate" do make semantic sense for this feature. The major drawback of not "solving" this now is essentially inconsistency within the add-ons manager. But, considering how few users use the add-ons manager, I think that's a tradeoff we should be ok with.

Comment on attachment 716265 [details] [diff] [review] patch with tristate menu Review of attachment 716265 [details] [diff] [review]: ----------------------------------------------------------------- UI is looking good :) I have some issues with the API however, which still feels inconsistent/complex: * The userDisabled property is still used as normal, but its now using different permissions flags - which really just mean the same as the old permissions flags. Would also be generally nice (not a hard requirement) to not break backwards compat. * The new userAskToActivate property is governed by one permissions flag. So there are 3 flags for 4 states... except there's a magic combination where two of those states end up meaning the same thing, even though they're different in the API. I think the fact that 'disabled' and 'clicktoplay' are implemented as separate toggles in nsPluginHost needs to be completely ignored when it comes to the API - its an unfortunate implementation detail that is better off not being exposed. Put another way: effectively there are only 3 distinct states that matter outside of nsPluginHost, the API should clearly reflect that. Oh, and the elephant in the room: Tests. ::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd @@ +91,5 @@ > <!ENTITY cmd.disableTheme.label "Stop Wearing Theme"> > <!ENTITY cmd.disableTheme.accesskey "W"> > +<!ENTITY cmd.askToActivate.label "Ask to Activate"> > +<!ENTITY cmd.alwaysActivate.label "Always Activate"> > +<!ENTITY cmd.neverActivate.label "Never Activate"> These also need tooltips, as well as one for the menulist itself. ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +2222,5 @@ > // 1-15 are different built-in views for the add-on type > VIEW_TYPE_LIST: "list", > > TYPE_UI_HIDE_EMPTY: 16, > + TYPE_UI_ASK_ENABLE_DISABLE_TRISTATE: 32, This isn't purely a UI thing, so I'd rather have this flag be named something like TYPE_SUPPORTS_ASKTOACTIVATE ::: toolkit/mozapps/extensions/PluginProvider.jsm @@ +307,5 @@ > return aVal; > }); > > + this.__defineGetter__("userAskToActivate", function() aTags[0].clicktoplay); > + this.__defineSetter__("userAskToActivate", function(aVal) { Will need to fire off some kind of addon event here. Creating a new event seems like overkill - can just fire onPropertyChanged, which takes an array of changed properties. And of course you'll need to make sure to handle that event in the UI. If you open two instances of the Add-ons Manager (open a tab and type about:addons, rather than use the menuitem/shortcut), and change a setting in one tab, check the other tab stays in sync. @@ +311,5 @@ > + this.__defineSetter__("userAskToActivate", function(aVal) { > + if (aTags[0].clicktoplay == aVal) > + return; > + > + aTags.forEach(function(aTag) { Nit: Generally, should prefer for-of loops (more readable code, runs faster). @@ +418,5 @@ > + permissions |= AddonManager.PERM_CAN_NEVER_ACTIVATE; > + > + var prefs = Cc["@mozilla.org/preferences-service;1"] > + .getService(Ci.nsIPrefBranch); > + if (prefs.getBoolPref("plugins.click_to_play")) { Use Services.prefs instead of the getService call (less verbose code, and it avoids an XPCOM call). ::: toolkit/mozapps/extensions/content/extensions.js @@ +1153,5 @@ > + aAddon.userDisabled = true; > + } > + }, > + > + cmd_tristatePlaceholder: { No need to hack around commands for this - just put it with the other UI update code in gDetailView.updateState(). ::: toolkit/mozapps/extensions/content/extensions.xml @@ +875,5 @@ > oncommand="document.getBindingParent(this).userDisabled = true;"/> > <xul:button anonid="remove-btn" class="addon-control remove" > label="&cmd.uninstallAddon.label;" > oncommand="document.getBindingParent(this).uninstall();"/> > + <xul:menulist anonid="tristate-menulist"> In details view, the text label of this dropdown button is cropped for me on Windows (sadly, these don't always resize right in some situations). Adding a crop="none" here fixes it. @@ +1278,5 @@ > this._preferencesBtn.hidden = !this.mAddon.optionsURL; > > + let addonType = AddonManager.addonTypes[this.mAddon.type]; > + if (addonType.flags & AddonManager.TYPE_UI_ASK_ENABLE_DISABLE_TRISTATE) { > + this._tristateMenulist.hidden = false; Don't unhide this until after selectedItem is set, to avoid any potential flashing as it changes. @@ +1282,5 @@ > + this._tristateMenulist.hidden = false; > + this._enableBtn.hidden = true; > + this._disableBtn.hidden = true; > + this._askToActivateMenuitem.disabled = !this.hasPermission("ask_to_activate"); > + this._alwaysActivateMenuitem.disabled= !this.hasPermission("always_activate"); Nit: Missing space before =

Thanks for the review! I think I addressed everything you mentioned (particularly tests). Carrying over ui-r+.

Comment on attachment 721469 [details] [diff] [review] patch v2 Review of attachment 721469 [details] [diff] [review]: ----------------------------------------------------------------- r-, as we've been hashing out some details outside of Bugzilla.

This patch makes use of userDisabled as we discussed over email.

Comment on attachment 726271 [details] [diff] [review] patch v3 Review of attachment 726271 [details] [diff] [review]: ----------------------------------------------------------------- This is shaping up nicely. There are some things that make me twitchy, but its due to touching areas where PluginProvider is already buggy - and fixing all that is way out of scope. Just gotta not make it worse :) ::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd @@ +95,5 @@ > +<!ENTITY cmd.alwaysActivate.label "Always Activate"> > +<!ENTITY cmd.alwaysActivate.tooltip "Always run this plugin when encountered"> > +<!ENTITY cmd.neverActivate.label "Never Activate"> > +<!ENTITY cmd.neverActivate.tooltip "Completely disable this plugin"> > +<!ENTITY cmd.tristateMenu.tooltip "Change when this plugin runs"> Replace 'plugin' with 'add-on' in these. For most people they're the same, so better to at least be consistent (hope to change the category name to 'Media' in bug 842510). And any addon type can implement this functionality, not just plugins - so if we were to use that wording, it would need to be special-cased like we do for themes... which isn't worth it for plugins IMO. 'runs' is also very plugin-specific and technical, and 'when encountered' feels awkward. How about something like 'Ask to use this add-on each time', 'Always use this add-on', and 'Never use this add-on' ? ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +2199,5 @@ > // Indicates that the Addon can be disabled by the user. > PERM_CAN_DISABLE: 4, > // Indicates that the Addon can be upgraded. > PERM_CAN_UPGRADE: 8, > + // Indicates that the Addon can be activated when asked to do so. This comment is a little unclear, since it's not about being activated per-se, but rather setting it to use that mode. So how about wording it something like: Indicates that the Addon can be set to be optionally enabled on a case-by-case basis. @@ +2218,5 @@ > // 1-15 are different built-in views for the add-on type > VIEW_TYPE_LIST: "list", > > TYPE_UI_HIDE_EMPTY: 16, > + TYPE_SUPPORTS_ASK_TO_ACTIVATE: 32, Add a comment explaining what this does. ::: toolkit/mozapps/extensions/PluginProvider.jsm @@ +443,5 @@ > this.__defineGetter__("operationsRequiringRestart", function() { > return AddonManager.OP_NEEDS_RESTART_NONE; > }); > > this.__defineGetter__("permissions", function() { This needs to take into account the current state too - it should exclude the permission to switch to the state it's already in. For instance, if userDisabled===true, then permissions shouldn't include PERM_CAN_DISABLE. ::: toolkit/mozapps/extensions/content/extensions.js @@ +1124,5 @@ > + return ((addonType.flags & AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE) && > + hasPermission(aAddon, "ask_to_activate")); > + }, > + doCommand: function cmd_askToActivateItem_doCommand(aAddon) { > + aAddon.userDisabled = "askToActivate"; Need a AddonManager.WHATEVER constant for this, rather than using a hard-coded string everywhere. @@ +1159,5 @@ > + isEnabled: function cmd_tristateMenu_isEnable(aAddon) { > + if (!aAddon) > + return false; > + let addonType = AddonManager.addonTypes[aAddon.type]; > + return (addonType.flags & AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE); Disabling the menulist should be moved to gDetailView.updateState() too, since this isn't a real command (and you're making sure the menulist is in the right state in updateState anyway). ::: toolkit/mozapps/extensions/content/extensions.xml @@ +1277,5 @@ > + this._enableBtn.hidden = true; > + this._disableBtn.hidden = true; > + this._askToActivateMenuitem.disabled = !this.hasPermission("ask_to_activate"); > + this._alwaysActivateMenuitem.disabled = !this.hasPermission("enable"); > + this._neverActivateMenuitem.disabled = !this.hasPermission("disable"); As with normal Enable/Disable buttons, these should be hidden if not available. Also, if none of the menuitems are available to use (as is the case when appDisabled==true), the whole menulist should be hidden. ::: toolkit/themes/linux/mozapps/extensions/extensions.css @@ -746,5 @@ > display: none; > } > > menulist { /* Fixes some styling inconsistencies */ > - font-size: 100%; Is removing this really needed? (I haven't been able to test the patch on Linux yet) It affects inline preferences, where the font looks too small compared to other elements without this.

Thanks for the review - I think this is coming along nicely. I've addressed all of your comments to the best of my understanding except for the css issue on linux. With that line included, the text of the drop-down button overflows. Is there an easy way to fix this, or is this a known bug? (I think you've mentioned this before...)

Gah - forgot to add. This patch requires the one in bug 854467.

(In reply to David Keeler (:keeler) from comment #71) > I've addressed all of your comments to the best of my understanding except > for the css issue on linux. With that line included, the text of the > drop-down button overflows. Is there an easy way to fix this, or is this a > known bug? (I think you've mentioned this before...) Yea, I noticed the same in the details view on Windows a few patch revisions ago. I just tested in my Linux VM, and don't see the issue myself, so I can't test this... but adding crop="none" to the <menulist> should in theory fix it.

Comment on attachment 729125 [details] [diff] [review] patch v4 Review of attachment 729125 [details] [diff] [review]: ----------------------------------------------------------------- Awesome :) I want to say "Ship it!", but the r+ is dependent on: * The suggestion in comment 37 working as expected (if so, no additional review necessary) * No objections to my reply in the "Java CtP Design Ideas" thread on the firefox-dev mailing list, regarding the "Always Activate" option (maybe give it another day) ::: toolkit/mozapps/extensions/content/extensions.xml @@ +1283,5 @@ > + this._enableBtn.hidden = true; > + this._disableBtn.hidden = true; > + this._askToActivateMenuitem.hidden = !this.hasPermission("ask_to_activate"); > + this._alwaysActivateMenuitem.hidden = !this.hasPermission("enable"); > + this._neverActivateMenuitem.hidden = !this.hasPermission("disable"); For consistency with the details view (and all other instances of .addon-control), set these to disabled and let the CSS hide them.

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #74) > the "Java CtP Design Ideas" thread on the firefox-dev mailing list Ho-ho, and possibly purposefully delay landing this until the new doorhanger design is ready to land (AFAIK, no bug yet for that).

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #75) > (In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from > comment #74) > > the "Java CtP Design Ideas" thread on the firefox-dev mailing list > > Ho-ho, and possibly purposefully delay landing this until the new doorhanger > design is ready to land (AFAIK, no bug yet for that). Per firefox-dev it's fine to land in 23 now when done.

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #73) > (In reply to David Keeler (:keeler) from comment #71) > > I've addressed all of your comments to the best of my understanding except > > for the css issue on linux. With that line included, the text of the > > drop-down button overflows. Is there an easy way to fix this, or is this a > > known bug? (I think you've mentioned this before...) > > Yea, I noticed the same in the details view on Windows a few patch revisions > ago. I just tested in my Linux VM, and don't see the issue myself, so I > can't test this... but adding crop="none" to the <menulist> should in theory > fix it. crop="none" alone didn't seem to do it, but adding sizetopopup="always" made it so the text would never be cut off. Is this an okay solution?

(In reply to David Keeler (:keeler) from comment #77) > crop="none" alone didn't seem to do it, but adding sizetopopup="always" made > it so the text would never be cut off. Is this an okay solution? Yep. If it works in the other case, may as well use it there too.

Sounds good. https://tbpl.mozilla.org/?tree=Try&rev=2b062cc86389

Ok - there were numerous test failures I had to fix due to "enable click-to-play" now meaning both setting the pref and setting the enabledState on the plugin tag in question. As a result, I even had to convert a few tests from plain mochitests to chrome mochitests. Jared - if you could take a look at those test changes, I would appreciate it.

Comment on attachment 738724 [details] [diff] [review] patch v5 Review of attachment 738724 [details] [diff] [review]: ----------------------------------------------------------------- I'm kinda confused by these test changes. Are you saying that since the semantics of click-to-play has changed, that all of these tests need to be tweaked so they still function as they used to? I'm kinda torn here, because I would prefer that the tests represent what the user will experience. If that means that some of these tests are obsolete then we should just remove them.

The issue is that the way click-to-play is enabled has changed. It used to be that setting plugins.click_to_play to true would make every plugin click-to-play. However, this is no longer sufficient (after this patch). plugins.click_to_play must be true, but for any given plugin, it must also be the case that its enabledState field is STATE_CLICKTOPLAY. So, when testing click-to-play, we have to set the field on each plugin. Incidentally, this is what the user will experience - they will have to set plugins.click_to_play to true and enable click-to-play for each plugin of choice in the addon manager. Setting the pref is not ideal right now, so eventually I'm hoping to have the pref be true by default or maybe even remove the pref entirely.

Comment on attachment 738724 [details] [diff] [review] patch v5 Review of attachment 738724 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/test_bug787619.html @@ +3,5 @@ > <title>Test for Bug 787619</title> > + <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > + <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> > + <script type="application/javascript;version=1.7"> > + function getTestPlugin() { Can this file not include head.js? This getTestPlugin() function is now different from the head.js version (head.js one takes a single argument). The more that these diverge the more confusing these tests may get. ::: toolkit/mozapps/extensions/test/xpcshell/test_pluginBlocklistCtp.js @@ +146,3 @@ > do_check_true(gPluginHost.isPluginClickToPlayForType("application/x-test")); > + // clean up plugin state > + plugin.enabledState = Components.interfaces.nsIPluginTag.STATE_ENABLED; I would prefer that we do: > let previousEnabledState = plugin.enabledState; > plugin.enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY; > do_check_true(...); > plugin.enabledState = previousEnabledState;

(In reply to Jared Wein [:jaws] from comment #83) > ::: browser/base/content/test/test_bug787619.html > Can this file not include head.js? This getTestPlugin() function is now > different from the head.js version (head.js one takes a single argument). > The more that these diverge the more confusing these tests may get. It appears that that head.js is for browser-chrome tests, and as such it doesn't work so well for mochitest-chrome tests. I think it makes the most sense to just convert that test to a browser-chrome test (I'm already upgrading it from a mochitest-plain test and all of the other plugin tests in that directory are browser-chrome ones (indeed, there aren't even any mochitest-chrome tests in that directory)). > ::: toolkit/mozapps/extensions/test/xpcshell/test_pluginBlocklistCtp.js > @@ +146,3 @@ > > do_check_true(gPluginHost.isPluginClickToPlayForType("application/x-test")); > > + // clean up plugin state > > + plugin.enabledState = Components.interfaces.nsIPluginTag.STATE_ENABLED; > > I would prefer that we do: > > let previousEnabledState = plugin.enabledState; > > plugin.enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY; > > do_check_true(...); > > plugin.enabledState = previousEnabledState; Sounds good.

Ok - it took a long time to change all the tests I had to, but I'm pretty confident everything's in good shape: https://tbpl.mozilla.org/?tree=Try&rev=2434f5f7b0b2 https://tbpl.mozilla.org/?tree=Try&rev=fbc32339e546 https://tbpl.mozilla.org/?tree=Try&rev=8cd7d4d31ec3 https://hg.mozilla.org/integration/mozilla-inbound/rev/450bbfd48532

Will bug 775857 take care of the consistency between about:permissions, page info/permissions and addons manager/plugins settings ?

bug 775857 will make about:permissions handle the fact that plugins are click-to-play on a case-by-case basis, if that's what you mean.

always activate/never activate/ask to activate options work properly. Verified fixed in nightly 24.0a1 (2013-05-19), aurora 23.0a2 (2013-05-19)

When upgrading from FF22 to FF23 a plugin in 'Enable' state will be migrated to 'Always Activate'. This doesn't look like a smart move to me. For anybody already relying on the click-to-play feature in FF22, this my come as a bad surprise. In FF23 plugins would start without the user being prompted. One needs to manually set every single plugin to 'Always Ask' in order to restore the click-to-play functionality.

(In reply to Christian Riechers from comment #91) > When upgrading from FF22 to FF23 a plugin in 'Enable' state will be migrated > to 'Always > Activate'. 'Always activate' is the equivalent of 'Enable' > One needs to manually set every > single plugin to 'Always Ask' in order to restore the click-to-play > functionality. Download Firefox nightly to see the click to play upcoming new look http://nightly.mozilla.org/

(In reply to Paul Silaghi [QA] from comment #92) > 'Always activate' is the equivalent of 'Enable' I do understand that. However, the issue is when migrating from FF22 to FF23. This will effectively disable click-to-play without notifying the user. Hence this has a significant security impact IMV. > Download Firefox nightly to see the click to play upcoming new look I'm not worried about the new look.

(In reply to Christian Riechers from comment #93) > (In reply to Paul Silaghi [QA] from comment #92) > > 'Always activate' is the equivalent of 'Enable' > I do understand that. However, the issue is when migrating from FF22 to > FF23. This will effectively disable click-to-play without notifying the > user. Hence this has a significant security impact IMV. Worth noting that click-to-play wasn't a user facing feature before, so this affects only more technical users which tested a not-yet-finished feature.

(In reply to Georg Fritzsche [:gfritzsche] from comment #94) > Worth noting that click-to-play wasn't a user facing feature before, so this > affects only more technical users which tested a not-yet-finished feature. Also worth noting is that searching "click-to-play Firefox" returns "About 42,072 results" so this affects only users that encountered one of those results and followed the instructions on it.

Christian(In reply to Christian Riechers from comment #93) > (In reply to Paul Silaghi [QA] from comment #92) > > 'Always activate' is the equivalent of 'Enable' > I do understand that. However, the issue is when migrating from FF22 to > FF23. This will effectively disable click-to-play without notifying the > user. Hence this has a significant security impact IMV. > > Download Firefox nightly to see the click to play upcoming new look > I'm not worried about the new look. That is exactly my problem. I got here by searching why the click-to-play setting suddenly stopped working.