STR: 0. Set plugins.click_to_play is true. 1. Open a page which has some elements using plugins, e.g: testcase.html 2. Click a plugin element(A). 3. Start plugin of element(A). Result: Start plugins of other elements too. I think that playing plugin should a clicked element only. This behavior is too simple. Firefox plays all plugins regardless of whether user wants to play all plugins or not. It will be risk if a page has malignant contents played with plugins. And from the viewpoint of usability, this behavior goes against user's expectation which Firefox plays plugins in clicked element only. The current Firefox click-to-play model notifies plugins are used in page by doorhanger notification. So Using doorhanger action is better when user want to plays all plugins in pages.

The problem with this approach is that, in the case of Flash, often SWF content relies on other SWFs including 'invisible' helper SWFs and will not work correctly. How can the user know what SWF's to click in what order to get a correctly functioning web page ? Click to play doesn't actually 'play' the content as well, it just allows the plugin instance to load normally.

(In reply to Ian Melven :imelven from comment #1) > The problem with this approach is that, in the case of Flash, often SWF > content relies on other SWFs including 'invisible' helper SWFs and will not > work correctly. How can the user know what SWF's to click in what order to > get a correctly functioning web page ? Click to play doesn't actually 'play' > the content as well, it just allows the plugin instance to load normally. When it need "helper" SWFs, user may be play *all plugins* by doorhanger or context-menu on placeholder of elements. (The context menu need to implement it...)

Other browsers (Chrome, Opera) permit to each element with to click an element. Also IE9, this behavior is like the Firefox current behavior. But IE9's approach does not provide click an element to play model, so IE9 permits via notification. IE9 approach makes user recognize that all plugins are switched by this notification. The click an element to play approach makes user recognize that only clicked element is played if user click an element. Plugin elements are not necessarily placed on near. So user almost always think that they are not related to single configuration for playing.

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #2) > (In reply to Ian Melven :imelven from comment #1) > > The problem with this approach is that, in the case of Flash, often SWF > > content relies on other SWFs including 'invisible' helper SWFs and will not > > work correctly. How can the user know what SWF's to click in what order to > > get a correctly functioning web page ? Click to play doesn't actually 'play' > > the content as well, it just allows the plugin instance to load normally. > > When it need "helper" SWFs, user may be play *all plugins* by doorhanger or > context-menu on placeholder of elements. (The context menu need to implement > it...) we could try to activate all hidden swf's when a visible swf is clicked, perhaps ? (and only activate the actual visible plugin instance that was clicked) ? does that sound more like what you are asking for ? also please note jared's work so far is a first pass on the feature :)

How about "try to activate all hidden swf's [that comply with same origin policy] when a visible swf is clicked (and only activate the actual visible plugin instance that was clicked)"?

(In reply to alex_mayorga from comment #5) > How about "try to activate all hidden swf's [that comply with same origin > policy] when a visible swf is clicked (and only activate the actual visible > plugin instance that was clicked)"? Personally, I don't think we should expect users to understand what a hidden plugin is - and helper SWF's are not necessarily same domain with their main SWF. But I do think we should aim for only activating the visible SWF (or other plugin content, I don't want to be too Flash-centric here) that has actually been clicked.

(In reply to Ian Melven :imelven from comment #4) > we could try to activate all hidden swf's when a visible swf is clicked, > perhaps ? (and only activate the actual visible plugin instance that was > clicked) ? does that sound more like what you are asking for ? also please > note jared's work so far is a first pass on the feature :) I think that "only activate the actual visible plugin instance that was clicked". At click-to--play, Firefox should only activate the plugin that user permits (by click) expressly. If it need to activate all plugins, user will permits to activate all plugins by doorhanger command.

(In reply to alex_mayorga from comment #5) > How about "try to activate all hidden swf's [that comply with same origin > policy] when a visible swf is clicked (and only activate the actual visible > plugin instance that was clicked)"? I think that it is not good. Its reason is written in Ian's comment #5. And also, from the viewpoint of security, the same origin can't guarantee the safety of plugin contents provided from its domain. If a site is defaced by cracker, same origin policy is meaningless.

I agree with :saneyuki for a number of reasons; - We shouldn't be going into this half heartedly, this is a great opportunity to increase security and (let's be honest) help curtail the use of plugins generally. It will be much less effective if everything is enabled with the first enabled element. - As a security concious user, I am expecting that the item I enable is the only one enabled. - Follow Opera and Chrome on this one, parity will help the cause tremendously, both in therms of users and webdevs. The only outcome I see to not following them is lower security. If this necessitates webdevs to change their strategy, so be it. They will have to do so to work on Chrome & Opera anyway. I don't see it as a sensible strategy to dilute this feature.

Additional reasons I have come to appreciate Noscript's model of click to play each element (or enable all if desired); - You can control how many plugins run at once, helping avoid situations where a number of plugins will slow down browsing - Various annoyances are often delivered through plugins (auto playing sound/video), and having control over those is great

This patch implements the behavior that if the user clicks on one plugin, only that plugin and other invisible plugins are played.

Comment on attachment 616755 [details] [diff] [review] patch Review of attachment 616755 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +7288,5 @@ > let pluginsPermission = Services.perms.testPermission(browser.currentURI, "plugins"); > let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox"); > > + if (browser._clickToPlayPluginsActivated || > + (browser._clickToPlaySinglePluginActivated && aIsInvisible)) { I don't think this is what we want. According to this, if a user activates a Flash plugin on the page, then 10 seconds later an invisible Java plugin is dynamically added to the page, the Java plugin will get activated.

(In reply to Jared Wein [:jaws] from comment #12) > I don't think this is what we want. According to this, if a user activates a > Flash plugin on the page, then 10 seconds later an invisible Java plugin is > dynamically added to the page, the Java plugin will get activated. That's handled in bug 746374.

Comment on attachment 616755 [details] [diff] [review] patch Review of attachment 616755 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +7218,5 @@ > + let overlay = aContentWindow.document.getAnonymousElementByAttribute(plugin, "class", "mainBox"); > + let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + if (!objLoadingContent.activated && plugin != aActivatedPlugin) { > + if (this.isTooSmall(plugin, overlay)) objLoadingContent.playPlugin(); > + else haveUnplayedPlugins = true; You should change the line of this if-else statement block and add bracket if possible. A browser should only the plugin that user clicks. A browser *must not* play invisible (hidden) plugins automatically if user clicks a plugin element. A cracker embeds evil contents that attacks vulnerabilities of plugins. So browser should only play the plugins that user permited obviously. If a web page need to play plugins with invisible (hidden) ones, user permits to play them via doorhanger notification. Therefore a browser has to not play invisible (hidden) plugins automatically. Helper plugin objects can be permited with the function of bug 746374.

Invisible plugins will not also played when a visible one is clicked.

Comment on attachment 623304 [details] [diff] [review] patch v2 + test Review of attachment 623304 [details] [diff] [review]: ----------------------------------------------------------------- This looks good so far, but I want to look at it a little longer.

Comment on attachment 623304 [details] [diff] [review] patch v2 + test Review of attachment 623304 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'd like to see it again before you check it in. ::: browser/base/content/browser.js @@ +7223,5 @@ > notification.remove(); > }, > > + activateSinglePlugin: function PH_activateSinglePlugin(aContentWindow, aPlugin) { > + let browser = gBrowser.getBrowserForDocument(aContentWindow.document); Can you move this line to be closer to where |browser| is used? @@ +7232,5 @@ > + let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + let plugins = cwu.plugins; > + let haveUnplayedPlugins = false; > + for (let plugin of plugins) { Can you to use Array.some here? @@ +7235,5 @@ > + let haveUnplayedPlugins = false; > + for (let plugin of plugins) { > + let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + if (plugin != aPlugin && !objLoadingContent.activated) { > + haveUnplayedPlugins = true; Add a |break;| here. ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd @@ +22,5 @@ > > <!ENTITY missingPlugin "A plugin is needed to display this content."> > <!-- LOCALIZATION NOTE (tapToPlayPlugin): Mobile (used for touch interfaces) only has one type of plugin possible. --> > <!ENTITY tapToPlayPlugin "Tap here to activate plugin."> > +<!ENTITY clickToPlayPlugins "Click here to activate plugin."> Going from plural to singular, we should change the entity name here so localizers can update their text.

Comment on attachment 623304 [details] [diff] [review] patch v2 + test Review of attachment 623304 [details] [diff] [review]: ----------------------------------------------------------------- I seem this looks good. Thank you, David! ::: browser/base/content/browser.js @@ +7315,5 @@ > > let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox"); > overlay.addEventListener("click", function(aEvent) { > if (aEvent.button == 0 && aEvent.isTrusted) > + gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin); In some edge cases, this |aPlugin| will be undefined. If this |aPlugin| is undefined, |overlay| will be undefined too. But in such cases, this part will not be called by changing Bug 752228. (Please look its bug. It describes one of their edge case) If we can, it is better that gPluginHandler.activateSinglePlugin does not use 2nd parameter |aPlugin|. (You need not accept this point)

what is a scenario where aPlugin is null? the overlay is null when the element is display:none but I'm not familiar with a case where aPlugin can be null.

(In reply to Jared Wein [:jaws] from comment #20) > what is a scenario where aPlugin is null? the overlay is null when the > element is display:none but I'm not familiar with a case where aPlugin can > be null. In the case of Bug 752228, I confirm with Venkman that aPlugin seems like undefined. I'm sorry if I could not come across to you via the stack trace which I pasted to https://bugzilla.mozilla.org/show_bug.cgi?id=752228#c2

Did you literally mean 'Array.some' or '[the array in question].some'? Also, would a 'break;' be appropriate in that case? Anyway, I made the changes I thought you meant.

Comment on attachment 623719 [details] [diff] [review] patch v3 + test Review of attachment 623719 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, sorry for the confusing review feedback in the other pass. r+ with the nit below fixed. ::: browser/base/content/browser.js @@ +7248,5 @@ > + > + let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + let plugins = cwu.plugins; > + let haveUnplayedPlugins = cwu.plugins.some(function(plugin) { The |plugins| variable here is unused so we can remove it.

Nit fixed. Carrying over r+. Try run: https://tbpl.mozilla.org/?tree=Try&rev=0d8d7aa1f43d

Broke a test; fixed it. Added a new test for something I wasn't testing before (dynamically loading more plugins after one has been clicked).

Comment on attachment 624114 [details] [diff] [review] patch v5 + tests v2 Review of attachment 624114 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_bug743421.js @@ +51,5 @@ > var plugin = gTestBrowser.contentDocument.getElementsByTagName("embed")[0]; > var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > ok(!objLoadingContent.activated, "Test 1b, Plugin should not be activated"); > > + popupNotification.mainAction.callback(); Why does this need to be changed?

Comment on attachment 624114 [details] [diff] [review] patch v5 + tests v2 > --- a/browser/base/content/test/browser_bug743421.js > +++ b/browser/base/content/test/browser_bug743421.js > @@ -47,17 +47,17 @@ function test1a() { > > function test1b() { > var popupNotification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser); > ok(popupNotification, "Test 1b, Should have a click-to-play notification"); > var plugin = gTestBrowser.contentDocument.getElementsByTagName("embed")[0]; > var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > ok(!objLoadingContent.activated, "Test 1b, Plugin should not be activated"); > > - EventUtils.synthesizeMouse(plugin, 100, 100, { }); > + popupNotification.mainAction.callback(); > setTimeout(test1c, 500); > } > > function test1c() { > var popupNotification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser); > ok(!popupNotification, "Test 1c, Should not have a click-to-play notification"); > var plugin = gTestBrowser.contentWindow.addPlugin(); This is the context for my previous comment.

Previously, popupNotification.mainAction.callback() was the same thing as clicking a plugin (i.e. it played every plugin). Now (due to the changes we're making in this bug), clicking one plugin does not play every plugin. This test assumes that an event happens that causes every plugin to play, so instead of clicking one plugin, we need to activate them all through the notification.

Comment on attachment 624114 [details] [diff] [review] patch v5 + tests v2 Review of attachment 624114 [details] [diff] [review]: ----------------------------------------------------------------- Oh ok, makes sense.

Try run for most recent patch: https://tbpl.mozilla.org/?tree=Try&rev=31f065390c1d

So, I was having trouble making the doorhanger go away. Turns out that the site I was testing on uses object/embed fallback, which looks like two plugins from the point of view of the window utils, but you can't actually activate both...

(In reply to neil@parkwaycc.co.uk from comment #33) > So, I was having trouble making the doorhanger go away. Turns out that the > site I was testing on uses object/embed fallback, which looks like two > plugins from the point of view of the window utils, but you can't actually > activate both... I seems your situation should be thought on other bug. I recommend filing a new bug which describes about your situation, and adding Bug 738698 & this to "Blocks" on it. (If you can, please attach the testcase to reproduce your bug)

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #0) > I think that playing plugin should a clicked element only. > And from the viewpoint of usability, this behavior goes against user's > expectation which Firefox plays plugins in clicked element only. Definitely. (In reply to Lozzy from comment #9) > - As a security concious user, I am expecting that the item I enable is the > only one enabled. Definitely. Many pages, like Dailymotion video pages, have, in addition of the central video, an auto-refreshing ad that happens to auto-play and to make noise, without caring about the central video already playing ! If a plug-in element, in order to work correctly, needs that some "helper" - or should we say "spy" ? - plug-in elements run too : 1) Well, this is a design problem in the page. 2) However, let's give the user the *additional* possibility of enabling all plug-ins in the page. Or enabling all plug-ins of each type (Flash, for instance) in the page. With a contextual menu item, an address bar notification or whatever.

http://img255.imageshack.us/img255/9088/ctp.png nightly 20.0a1 (2012-12-02) 2 questions: 1. why is the 3rd plugin so dark? 2. what's with the "unknown" plugin on the top?

(In reply to Paul Silaghi [QA] from comment #36) > 1. why is the 3rd plugin so dark? > 2. what's with the "unknown" plugin on the top? Could you file bugs for those issues? (2) is a regression and not occuring in the current beta/18, (1) might just be a style issue for the binding.

(In reply to Georg Fritzsche [:gfritzsche] from comment #37) > is a regression and not occuring in the current beta/18 Actually it's happening in beta/18 but not in release/17.0.1.

(In reply to Georg Fritzsche [:gfritzsche] from comment #37) > (In reply to Paul Silaghi [QA] from comment #36) > > 1. why is the 3rd plugin so dark? > > 2. what's with the "unknown" plugin on the top? > > Could you file bugs for those issues? (2) is a regression and not occuring > in the current beta/18, (1) might just be a style issue for the binding. bug 818008, bug 818009 filed

Verified fixed this on nightly 20.0a1 (2012-12-02) considering comment 39.