+++ This bug was initially created as a clone of Bug #549697 +++ Basically bug 708464 for desktop. Patch based on Blair's and Margaret's work in bug 549697.

Can we do this without notification box? This will trigger far too often and be annoying. Wear-down etc.pp. I prefer something like FlashBlock: the area of the plugin in the page is covered, with a single "Play" button in the middle. (And yes, I don't care much about plugins that have no UI.)

xref bug 711618

(In reply to Ben Bucksch (:BenB) from comment #2) > (And yes, I don't care much about plugins that have no UI.) There's a good chance that other user would dislike soundcloud.com & Co. breaking. The alternative to the notification bar would be to always enable invisible plugins.

(In reply to Dão Gottwald [:dao] from comment #4) > > The alternative to the notification bar would be to always enable invisible > plugins. There's a couple of sides to this IMO - see bug 549697 comment 24 which talks about Flash content breaking if there are invisible 'helper SWFs' that can't be click to play-ed. From a security standpoint, always allowing invisible plugins to load takes away a lot of click to play being a mitigation against 'driveby plugin exploits' (which we know are common). Then again, working on bug 690161 and having an acceptable UX for blocking vulnerable versions of plugins from loading could potentially be a better way of addressing that problem.

(In reply to Ian Melven :imelven from comment #5) > (In reply to Dão Gottwald [:dao] from comment #4) > > > > The alternative to the notification bar would be to always enable invisible > > plugins. > > There's a couple of sides to this IMO - see bug 549697 comment 24 which > talks about Flash content breaking if there are invisible 'helper SWFs' that > can't be click to play-ed. From a security standpoint, always allowing > invisible plugins to load takes away a lot of click to play being a > mitigation against 'driveby plugin exploits' (which we know are common). For mobile (bug 708464), we decided we would just play all plugins on the page if you clicked on one of them - this avoids the hidden "helper SWF" problem. We also decided we would only show a notification if there were no visible plugins on the page, which was our solution to Dão's concern in comment 4.

> The alternative to the notification bar would be to always enable invisible plugins. That's a catastrophe security-wise. Then you can just as well drop the whole feature. Comment 6: pretty much what I wanted to propose Note that if the user [x] remembers, the enable should still be specific for the plugins that existed at the time of the remembering, and never enable all plugins on the page. It is common that big, normal websites get infected and deliver exploit code unwillingly, and any solution must protect against that. Another alternative would be to just make invisible plugins visible and put the Play button there. More or less like the notification bar, just inside the page.

Here's the latest: * left or right clicking a plugin brings up a menu with the option to play it * if a visible plugin is played, all invisible plugins are also played * if there are no visible plugins, a hanger allows the user to play them (otherwise there is no hanger) * any permission related issues are implemented in bug 711618 Issue: the 'Tap here to activate plugin' message should really be 'Click here...' - is there an easy to differentiate between mobile and desktop in this case? (right now this is in toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd)

(In reply to David Keeler from comment #8) > * if a visible plugin is played, all invisible plugins are also played > * if there are no visible plugins, a hanger allows the user to play them > (otherwise there is no hanger) Is there differentiation between plugin types? That is: - if I click and play a flash element, does that also enable java applets on the page; - if there are visible flash elements and invisible java elements, do I get a doorhanger for the java?

(In reply to Tom Lowenthal [:StrangeCharm] from comment #9) > Is there differentiation between plugin types? Not at the moment, but it's certainly doable. One thing I'm concerned about is that such differentiation might result in more notifications and complexity than users want to deal with (thus resulting in them going back to always enabling plugins, which is not what we want). > That is: > - if I click and play a flash element, does that also enable java applets on > the page; Only if the java applets are "invisible" (essentially, too small to be seen). Enabling one visible plugin does not affect any other visible ones. > - if there are visible flash elements and invisible java elements, do I get > a doorhanger for the java? Not as currently implemented.

(In reply to David Keeler from comment #8) > * left or right clicking a plugin brings up a menu with the option to play it > * if a visible plugin is played, all invisible plugins are also played > * if there are no visible plugins, a hanger allows the user to play them > (otherwise there is no hanger) That sounds perfect. *Exactly* like that. Thanks!

As a security mitigation, what will/should happen in this scenario: a vulnerability is discovered in Java/Acrobat/whatever (or in specific older versions of these plugins), so we want to make only those plugins "click to play" without affecting Flash. Will this UI ignore the plugins that run automatically and the decision to display a hanger is based only on the semi-blocked plugins? Will the click-to-play UI show *which* plugins are being activated, or just that some plugin is going to be enabled? In the case of multiple kinds hidden plugins, will the UI show all the various types that will be enabled? Are we exposing to the DOM that a particular plugin element (<object> or <embed> is user-disabled?) This seems important for websites that rely primarily on plugins (e.g. Pandora) so that they can show alternate UI (plugins are disabled, please click to play) instead of timing out and showing a generic "please install Flash" or "Song initialization timed out, please hit refresh" UI.

need to schedule a secteam review

David, I wanted to let you know that we recently changed our implementation of the click to play plugins UI on mobile because we were running into problems when pages were loaded from the cache. The patch for this is in bug 719875, but I also filed bug 723617 about changing the platform side of things for a more robust solution.

(In reply to Ben Bucksch (:BenB) from comment #7) > Note that if the user [x] remembers, the enable should still be specific for > the plugins that existed at the time of the remembering, and never enable > all plugins on the page. Who will be the "originator" in case of a driveby attack scenery? Example: you(t)ube gets hacked, an exploit through flash in a driveby scenario. Naturally the user has allowed flash on yt. The driveby injections is done via a <script src="remote://"> tag, which loads more code from a remote page, loads malicious flash swf, etc. Would the user see, being on you(t)ube.com, that a plugin is being activated via a third party? Or would it be just granted allowance due user previous preference on you(t)ube? And of course this is an idealized scenario: once they were able to break into the the server, they could of course server the malicious code right from their on page, too. Probably no way to mitigate this ...

(In reply to Markus Fischer from comment #15) > Would the user see, being on you(t)ube.com, that a plugin is being activated > via a third party? Or would it be just granted allowance due user previous > preference on you(t)ube? It would appear that youtube is trying to load some plugin. However, as the plugin's uri is something the user has never seen before, it would not be automatically played. > And of course this is an idealized scenario: once they were able to break > into the the server, they could of course server the malicious code right > from their on page, too. Probably no way to mitigate this ... Right. And, if they replaced something the user had seen and enabled before (e.g. the flash object that plays the videos on youtube), it would get played automatically.

I have "Click to Play" right now using Firefox via the "NoScript" extension. If NoScript settings are set to Globally Allow Scripts and the Embeddings are applied to whitelisted items, then NoScript doesn't interfere with anything on a page except for plugins, which it displays with a NoScript graphic. When a Plugin is encountered on a webpage, NoScript displays its graphic over the area. To play the plugin, I simply click the NoScript-gracphic and the plugin is allowed to start playing.

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?

This is based off of David's patch. I have removed the context menu for the click to play plugins and plugins are now activated upon a single left-click on the plugin. Still left to implement: Add the ability to remember settings for a site if plugins have been activated on a site for >= 3 times.

I'd be happy to take the work for this bug :)

Jared, I mentioned this above in comment 14, but on mobile we had problems storing the plugins to play in an array. This was because we would re-set the array when navigating to a new page (like I see you're doing here in an unload listener), but we don't receive the "PluginClickToPlay" event when pages are loaded from the cache, so the array doesn't get re-populated in that case. You can look at the patch in bug 719875 for the solution we ended up using, but I think the real solution would be bug 723617.

(In reply to Margaret Leibovic [:margaret] from comment #21) > Jared, I mentioned this above in comment 14, but on mobile we had problems > storing the plugins to play in an array. This was because we would re-set > the array when navigating to a new page (like I see you're doing here in an > unload listener), but we don't receive the "PluginClickToPlay" event when > pages are loaded from the cache, so the array doesn't get re-populated in > that case. You can look at the patch in bug 719875 for the solution we ended > up using, but I think the real solution would be bug 723617. Thanks Margaret. I will include testing and fixing of loading from the cache in my next version. Margaret: Is this also why the mobile version of click-to-play plugins traverses the DOM and looks for embeds? Is it considered OK that we are setting an attribute on the embed element to know that it was played? That part seemed confusing to me, unless web developers are asking for status on click-to-play from their scripts.

(In reply to Jared Wein [:jaws] from comment #22) > Margaret: Is this also why the mobile version of click-to-play plugins > traverses the DOM and looks for embeds? Is it considered OK that we are > setting an attribute on the embed element to know that it was played? That > part seemed confusing to me, unless web developers are asking for status on > click-to-play from their scripts. Yes, this is the only reason we traverse the DOM. It's also the only reason that we set an attribute on the embed element. To be honest, I hadn't thought about web developers looking at that attribute, but I guess that could be useful. From a security standpoint, setting the attribute themselves wouldn't do anything, other than make the plugin object never get activated, so at least that's not something to worry about.

Ok, thanks for the explanation :) I don't know of another way to keep track of these elements, but I recommend that we rename that attribute to mozPlayed to reduce breakage of scripts but also because "played" isn't an adopted standard as I understand it. I'll file a separate bug for this refactoring and work on that too.

Wait, our chrome code sets an attribute on the content DOM element here? That's generally not what we want, we don't want our chrome code injecting observable things into the DOM tree. Doing so causes problems with serialization/deserialization, cloning and using the cloned node, and can also generally confuse code on the page itself. If our UI code really needs to store the played state on a plugin in a content DOM then we should store that state through internal interfaces in ways where the state does not get copied when a node is cloned etc. We already have nsIObjectLoadingContent, can we move that state there?

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #25) > Wait, our chrome code sets an attribute on the content DOM element here? > That's generally not what we want, we don't want our chrome code injecting > observable things into the DOM tree. Doing so causes problems with > serialization/deserialization, cloning and using the cloned node, and can > also generally confuse code on the page itself. > > If our UI code really needs to store the played state on a plugin in a > content DOM then we should store that state through internal interfaces in > ways where the state does not get copied when a node is cloned etc. We > already have nsIObjectLoadingContent, can we move that state there? This will be fixed in bug 730318.

Excellent, thanks!

Comment on attachment 600301 [details] [diff] [review] WIP patch for bug v3.1 Review of attachment 600301 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working. I have quite a few comments but most are minor or ideas for follow-ups. :) I think we should use "cursor: pointer" when hovering the click-to-play container to make the clickable area clear. ::: browser/base/content/browser.js @@ +7098,5 @@ > > + case "PluginClickToPlay": > + let clickToPlayBox = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox"); > + self.appendClickToPlayPlugin(plugin, self.isTooSmall(plugin, clickToPlayBox)); > + clickToPlayBox.addEventListener("click", self.clickHandlerForClickToPlayPlugins.bind(self), true); Note: These click event listeners are not removed after use. @@ +7123,5 @@ > + this.enablePlugins(browser); > + }, > + > + enablePlugins: function(browser) { > + // XXX not sure if we should enable plugins for the top document or not? IMO, it would be nice to only enable plugins for documents of the same origin but that could probably be done in a follow-up if others agree. @@ +7213,5 @@ > + callback: function() gPluginHandler.enablePlugins(browser) > + }], > + }; > + > + let notificationBar = notificationBox.getNotificationWithValue("ask-to-enable-plugin"); You could just use notification.barID here. @@ +7217,5 @@ > + let notificationBar = notificationBox.getNotificationWithValue("ask-to-enable-plugin"); > + if (!browser.haveVisiblePlugins && !notificationBar) { > + notificationBox.appendNotification(notification.message, > + notification.barID, > + notification.iconURL, Is there a reason we're using a notification box rather than a doorhanger? I believe new tab-specific prompts should use doorhangers unless there is some reason they can't. @@ +7221,5 @@ > + notification.iconURL, > + notificationBox.PRIORITY_WARNING_MEDIUM, > + notification.buttons); > + } else if (!invisible && notificationBar) { > + notificationBox.removeNotification(notificationBar, true); Add a comment about this case. My understanding is that we're removing a notification bar that's showing if we later find a visible plugin. Won't this cause a flickering notification bar in this case while the page is loading? Otherwise, if the page is done loading it's a bit unusual for the notification bar to just disappear since there's no way to bring it back. It would be nice to wait until we're more sure that we need a notification bar (perhaps with a timeout?) before showing one. For the page load case, it would be nice to wait until DOMContentLoaded (+ some max. time for huge pages) before showing a notification. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +116,5 @@ > carbonFailurePluginsMessage.restartButton.label=Restart in 32-bit mode > carbonFailurePluginsMessage.restartButton.accesskey=R > +askToEnablePluginsMessage.title=This page requests to use plugins. > +askToEnablePluginsMessage.enable.label=Enable plugins > +askToEnablePluginsMessage.enable.accesskey=E Nit: Not a fan of the "askToEnablePluginsMessage" identifier. "Ask" is usually implied in a notification message. Also, we should probably get UX feedback on the string if we haven't already. "Enable plugins" may be confused with enabling a plugin globally. Perhaps "Enable plugins on this page"? Note that doorhangers use a question for copy rather than a statement. ::: content/base/src/nsObjectLoadingContent.cpp @@ +2112,1 @@ > return NS_OK; Could you add a comment about what this change is for? ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd @@ +21,5 @@ > <!ENTITY pluginWizard.finalPage.restart.label "&brandShortName; needs to be restarted for the plugin(s) to work."> > > <!ENTITY missingPlugin "A plugin is needed to display this content."> > +<!ENTITY tapToPlayPlugin "Tap here to activate plugin."> > +<!ENTITY clickToPlayPlugin2 "Click here to activate plugin."> Shouldn't plugin be plural here (id + string) to align with the implementation.

Comment on attachment 600301 [details] [diff] [review] WIP patch for bug v3.1 Review of attachment 600301 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +7191,5 @@ > + if (!browser.clickToPlayPlugins) { > + browser.haveVisiblePlugins = false; > + browser.clickToPlayPlugins = []; > + // Clear the click-to-play array upon leaving this page. > + plugin.ownerDocument.defaultView.top.addEventListener("unload", We shouldn't use the unload event, since it will disable the bfcache. We should use pagehide here instead, but bug 723617 will have to be fixed first.

(In reply to Matthew N. [:MattN] from comment #28) > Comment on attachment 600301 [details] [diff] [review] > WIP patch for bug v3.1 > > Review of attachment 600301 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +7123,5 @@ > > + this.enablePlugins(browser); > > + }, > > + > > + enablePlugins: function(browser) { > > + // XXX not sure if we should enable plugins for the top document or not? > > IMO, it would be nice to only enable plugins for documents of the same > origin but that could probably be done in a follow-up if others agree. This will break, for example, plugins displaying advertising content from a 3rd party domain.

(In reply to Ian Melven :imelven from comment #30) > (In reply to Matthew N. [:MattN] from comment #28) > > Comment on attachment 600301 [details] [diff] [review] > > WIP patch for bug v3.1 > > > > Review of attachment 600301 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > @@ +7123,5 @@ > > > + this.enablePlugins(browser); > > > + }, > > > + > > > + enablePlugins: function(browser) { > > > + // XXX not sure if we should enable plugins for the top document or not? > > > > IMO, it would be nice to only enable plugins for documents of the same > > origin but that could probably be done in a follow-up if others agree. > > This will break, for example, plugins displaying advertising content from a > 3rd party domain. Which honestly would be very welcome, at least as an option.

This patch gets us closer to the implementation and is dependent on the patch in bug 730318. Note that with this patch, the doorhanger will always show, there is no delay in showing the doorhanger, and there is no way to remember the settings for a site. Plugin elements created after activation of the plugins will not be auto-activated.

This patch enables the click-to-play preference so we can get some testing on this in Nightly (and possibly Aurora). If the other parts of click-to-play plugins aren't landed in time for the merge to Aurora, then we'll change the pref to false for Fx14 during the Aurora cycle.

Comment on attachment 607677 [details] [diff] [review] Patch for bug (apply on top of patch for bug 730318) >+ gBrowser.addEventListener("load", function(aEvent) { >+ let browser = gBrowser.getBrowserForDocument(aEvent.target); >+ gPluginHandler.showPluginClickToPlayDoorhanger(browser); >+ }, true); There are progress listeners that should let you call showPluginClickToPlayDoorhanger at the right time. You shouldn't need a load event listener for this. Also, prefix custom properties you put on browsers with an underscore. >+@media (-moz-touch-enabled) { >+ :-moz-handler-clicktoplay .msgClickToPlay { >+ display: none; >+ } >+ :-moz-handler-clicktoplay .msgTapToPlay { >+ display: block; >+ } >+} This works only on windows.

I've fixed the issues that Dao pointed out and moved the -moz-touch-enabled code to the themes so that we can apply the -moz-touch-enabled correctly. I've also pushed a patch to inbound for bug 737675 which added support for -moz-touch-enabled on Android and Gonk.

Comment on attachment 607815 [details] [diff] [review] Patch for bug v2 (apply on top of patch for bug 730318) > startDocumentLoad: function XWB_startDocumentLoad(aRequest) { > // clear out feed data > gBrowser.selectedBrowser.feeds = null; > > // clear out search-engine data > gBrowser.selectedBrowser.engines = null; > >+ // initialize the click-to-play state >+ gBrowser.selectedBrowser._loadEventHandled = false; >+ gBrowser.selectedBrowser._clickToPlayDoorhangerShown = false; You don't want this for background tabs? >+ activatePlugins: function(aContentWindow) { >+ let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindowUtils); >+ let plugins = cwu.plugins; >+ for (let i in plugins) { >+ let plugin = plugins[i]; for (let plugin of plugins) { >+ let isAnyPluginVisible = false; >+ for (let plugin of plugins) { >+ let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox"); >+ if (overlay && !gPluginHandler.isTooSmall(plugin, overlay)) >+ isAnyPluginVisible = true; >+ } >+ if (plugins && plugins.length && !isAnyPluginVisible) { if plugins were null or plugins.length were 0, isAnyPluginVisible wouldn't be true >-<!ENTITY clickToPlayPlugin "Tap here to activate plugin."> >+<!ENTITY tapToPlayPlugin "Tap here to activate plugin."> <!-- Mobile only has one type of plugin possible. --> This looks like it should be a proper LOCALIZATION NOTE.

>+ if (/^about:/.test(aWebProgress.DOMWindow.document.documentURI)) { >+ aBrowser.addEventListener("click", BrowserOnClick, false); >+ aBrowser.addEventListener("pagehide", function () { >+ aBrowser.removeEventListener("click", BrowserOnClick, false); >+ aBrowser.removeEventListener("pagehide", arguments.callee, true); >+ }, true); >+ >+ // We also want to make changes to page UI for unprivileged about pages. >+ BrowserOnAboutPageLoad(aWebProgress.DOMWindow.document); >+ } I think this part should use function name instead of arguments.callee. e.g: aBrowser.addEventListener("click", BrowserOnClick, false); aBrowser.addEventListener("pagehide", function onPagehide() { aBrowser.removeEventListener("click", BrowserOnClick, false); aBrowser.removeEventListener("pagehide", onPagehide, true); }, true);

At patch v2, I have something on my chest. >+ let options = { timeout: Date.now() + 30000 }; >+ PopupNotifications.show(aBrowser, "click-to-play-plugins", >+ messageString, "addons-notification-icon", >+ action, null, options); >+ } The patch v2 uses "addons-notification-icon" icon as popup notification icon. However, this icon is used for add-ons installations. The role of click-to-play-plugins is distinct from it. Using same icon may causes some problem of expandability by add-ons. So Should it make the new icon element for click-to-play?

Comment on attachment 607815 [details] [diff] [review] Patch for bug v2 (apply on top of patch for bug 730318) Review of attachment 607815 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little bit uncertain about the timing when the doorhanger might kick in. Is it known if by the time STATE_STOP happens, the style resolution has happened and you will know what are the plugins' dimensions? ::: browser/base/content/browser.js @@ +5120,5 @@ > // document URI is not yet the about:-uri of the error page. > > + if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) { > + gPluginHandler.showPluginClickToPlayDoorhanger(aBrowser); > + we cannot have heavy operations in progress listeners as they can slow down networking. Since this call will iterate through plugins, it is potentially unbounded, so we should fire it from a setTimeout(.., 0) what is the intended behavior if the STATE_STOP happens for a background tab? From what I can tell this will display the doorhanger, which doesn't seem right as the doorhangers are associated with the current tab @@ +7189,5 @@ > + overlay.addEventListener("click", function(e) { > + if (e.button == 0) > + gPluginHandler.activatePlugins(contentWindow); > + }, true); > + can you reuse addLinkClickCallback here? at least you'll need to check for e.isTrusted but you should just use the same function that the plugin reload uses. your function can also avoid the closure by using event.target.defaultView.top instead of contentWindow @@ +7206,5 @@ > + let isAnyPluginVisible = false; > + for (let plugin of plugins) { > + let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox"); > + if (overlay && !gPluginHandler.isTooSmall(plugin, overlay)) > + isAnyPluginVisible = true; break the loop (or the function) when the first visible plugin is found ::: toolkit/mozapps/plugins/content/pluginProblem.xml @@ +53,1 @@ > <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/> Is this change in the stylesheet ordering necessary?

(In reply to Felipe Gomes (:felipe) from comment #39) > can you reuse addLinkClickCallback here? at least you'll need to check for > e.isTrusted but you should just use the same function that the plugin reload > uses. I suspect the isTrusted checks are leftovers from pre-bug 289940 code. They shouldn't be needed unless the relevant addEventListener calls are passing aWantsUntrusted=true (worth testing, though - in a followup if desired).

(In reply to Dão Gottwald [:dao] from comment #36) > >+ if (plugins && plugins.length && !isAnyPluginVisible) { > > if plugins were null or plugins.length were 0, isAnyPluginVisible wouldn't > be true Yeah, this is by design. We should only show the doorhanger if there are more than 0 plugins on a page and none of them are visible. (In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #37) > I think this part should use function name instead of arguments.callee. My patch only changes the indentation levels of this code, so I don't want to increase the scope of this bug by cleaning up other code nearby. We should just file a separate bug to clean up usages of arguments.callee. (In reply to Felipe Gomes (:felipe) from comment #39) > I'm a little bit uncertain about the timing when the doorhanger might kick > in. Is it known if by the time STATE_STOP happens, the style resolution has > happened and you will know what are the plugins' dimensions? STATE_STOP is as similar to the load event as we can get. I haven't seen any issues from this in my testing, but if we notice some down the road then I think we can address them at that time. > what is the intended behavior if the STATE_STOP happens for a background > tab? From what I can tell this will display the doorhanger, which doesn't > seem right as the doorhangers are associated with the current tab The doorhanger code handles this properly and doesn't show doorhangers for wrong browser. > ::: toolkit/mozapps/plugins/content/pluginProblem.xml > @@ +53,1 @@ > > <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/> > > Is this change in the stylesheet ordering necessary? The skin stylesheet should follow the content stylesheet. I checked mxr and I can't find any other instance of a skin stylesheet preceding a content stylesheet. (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #40) > I suspect the isTrusted checks are leftovers from pre-bug 289940 code. They > shouldn't be needed unless the relevant addEventListener calls are passing > aWantsUntrusted=true (worth testing, though - in a followup if desired). I added the e.isTrusted check for consistency and extra precaution for now. I think we should file a follow-up bug to investigate the necessity of the e.isTrusted checks and remove them after we know it is safe to do so.

Comment on attachment 608121 [details] [diff] [review] Patch for bug v3 (apply on top of patch for bug 730318) Review of attachment 608121 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +5169,5 @@ > > if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP && > + Components.isSuccessCode(aStatus)) { > + setTimeout(function() { gPluginHandler.showPluginClickToPlayDoorhanger(aBrowser) }, 0); > + have the function body in an own line to make it easier to read

Moving sec-review-needed to bug 738698 as OK'd by curtisk. 10:28 AM <jaws> k, i just think that i should file a new bug that is a meta bug for the whole feature and do the sec-review on that bug 10:28 AM <curtisk> jaws: that would be fine 10:28 AM <jaws> ok thanks 10:45 AM <jaws> ok, then i'll remove the sec-review-needed flag from 711552 10:45 AM <curtisk> wfm

I forgot to make the necessary style changes for gnomestripe and pinstripe.

Part 1 of this patch was backed out due to test failures. I'm investigating now. https://hg.mozilla.org/integration/mozilla-inbound/rev/4f74dc1b7f52

This patch encompasses the work from the two previous r+'d patches, but refactors some of the code to make it less complicated. Showing the notification is no longer delayed until the load event, and we no longer loop over all plugin objects for each click-to-play event. This patch also properly handles back-forward navigation. On pages where plugins are considered visible by our metric, but not user facing, such as Pandora, we will show the notification in a dismissed state so users can still enable plugins. We should improve our metric for determining if a plugin is "visible", but that shouldn't be included in this bug. Pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=93971b34ffd8

Comment on attachment 610322 [details] [diff] [review] Patch for bug v4 Review of attachment 610322 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +7250,5 @@ > + return; > + } > + > + let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox"); > + let contentWindow = browser.contentWindow; |contentWindow| is unused here. I'll remove it.

Try run for 93971b34ffd8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=93971b34ffd8 Results (out of 224 total builds): exception: 3 success: 196 warnings: 23 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-93971b34ffd8

Comment on attachment 610322 [details] [diff] [review] Patch for bug v4 > function pageShowEventHandlers(event) { > // Filter out events that are not about the document load we are interested in > if (event.originalTarget == content.document) { > charsetLoadListener(event); > XULBrowserWindow.asyncUpdateUI(); >+ >+ // |event.persisted| is true when the page is loaded from the BF cache. >+ if (event.persisted) >+ gPluginHandler.reshowClickToPlayNotificationOnPageShow(event); > } > } >+ reshowClickToPlayNotificationOnPageShow: function(aEvent) { >+ if (!Services.prefs.getBoolPref("plugins.click_to_play")) >+ return; >+ >+ let browser = gBrowser.getBrowserForDocument(aEvent.target.defaultView.top.document); This is just gBrowser.selectedBrowser. "function (aEvent) {" for anonymous functions. Better yet, give this function a name.

Comment on attachment 610322 [details] [diff] [review] Patch for bug v4 Review of attachment 610322 [details] [diff] [review]: ----------------------------------------------------------------- As we talked, with this new approach the first plugin will determine if the doorhanger is shown or not. Ultimately, the problem is that there is no perfect way to determine if the doorhanger needs to be displayed, given the nature of web page loading, styling, dynamic changes... So we will need to talk with UX to see what are the acceptable options. I'm inclined to think that never showing the doorhanger, but always adding the doorhanger icon is the way to go, specially when considering the goals of the feature (security for outdated plugins for example). I had already added some comments to the patch so I'm posting them as they may still be relevant afterwards. ::: browser/base/content/browser.js @@ +258,5 @@ > XULBrowserWindow.asyncUpdateUI(); > + > + // |event.persisted| is true when the page is loaded from the BF cache. > + if (event.persisted) > + gPluginHandler.reshowClickToPlayNotificationOnPageShow(event); Brief comment here to explain that this is needed because the click-to-play events are not re-fired on BF cache navigation @@ +7173,5 @@ > overlay.style.visibility = "hidden"; > } > }, > > + activatePlugins: function(aContentWindow) { Now that visible plugins will add the dismissed doorhanger, activatePlugins needs to remove the click-to-play icon from the URL bar @@ +7183,5 @@ > + for (let plugin of plugins) { > + let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + if (!objLoadingContent.activated) > + objLoadingContent.playPlugin(); > + plugin.removeEventListener("click", plugin.clickHandler, true); As Olli pointed out on IRC, no need to worry about this as the listener should go away by itself when the overlay is removed due to the plugins being activated @@ +7245,5 @@ > + _handleClickToPlayEvent: function(aPlugin) { > + let browser = gBrowser.getBrowserForDocument(aPlugin.ownerDocument.defaultView.top.document); > + let doc = aPlugin.ownerDocument; > + if (browser._clickToPlayPluginsActivated) { > + gPluginHandler.activatePlugins(doc.defaultView.top); a bit unecessary to iterate over all plugins @@ +7254,5 @@ > + let contentWindow = browser.contentWindow; > + overlay.addEventListener("click", function(aEvent) { > + if (aEvent.button == 0 && aEvent.isTrusted) > + gPluginHandler.activatePlugins(aEvent.target.ownerDocument.defaultView.top); > + }, true); For a follow-up: what's the accessibility status of the feature? If the overlay is tabbable you should also listen for an Enter keypress @@ +7264,5 @@ > + if (!browser._clickToPlayDoorhangerShown) > + gPluginHandler._showClickToPlayNotification(browser, browser._clickToPlayOverlayShown); > + }, > + > + reshowClickToPlayNotificationOnPageShow: function(aEvent) { This name is too specific, no need to include "OnPageShow". The comment added on the caller makes the purpose clear @@ +7277,5 @@ > + let overlay = aPlugin.ownerDocument.getAnonymousElementByAttribute(aPlugin, "class", "mainBox"); > + return overlay && !gPluginHandler.isTooSmall(aPlugin, overlay); > + } > + let plugins = cwu.plugins; > + let isAnyPluginVisible = plugins.some(isPluginVisible) missing semicolon at the end

I've addressed the various feedback. The other plugin overlays have a link that is added to them which accepts the "keydown" event and checks for the Return key. We should add something like that, but we'll need to figure out how we want to do implement it since a link might not make as much sense here since there will be no document navigation occurring.

Comment on attachment 610781 [details] [diff] [review] Patch for bug v5 >+ reshowClickToPlayNotification: function PH_reshowClickToPlayNotification(aEvent) { >+ if (!Services.prefs.getBoolPref("plugins.click_to_play")) >+ return; >+ >+ let contentWindow = gBrowser.selectedBrowser.contentWindow; >+ let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindowUtils); >+ if (cwu.plugins.length) >+ gPluginHandler._showClickToPlayNotification(browser); >+ }, aEvent is unused

Comment on attachment 610781 [details] [diff] [review] Patch for bug v5 We did an in person review of this patch, there were only some nits to be fixed and Jared is gonna check something on youtube before posting the next version

Fixed some nits. I couldn't reproduce the notification-disappearing issue I was seeing on YouTube last week. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1cc825e6fb87

Comment on attachment 611629 [details] [diff] [review] Tests for patch Review of attachment 611629 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for writing these tests, they look good! It'd be great to include a few more things: - a test for the case when the plugin is too small such that the overlay can't be shown, and then activate it via the doorhanger - It should also be possible to test the reshowDoorhanger function by using gTestBrowser.goBack() - And lastly test that the PluginClickToPlay event is fired 3 times when plugin_test2.html loads (asking because I don't see any tests about this in the patch that implemented that event) r+ for the ones implemented in this patch with the following 2 nits: ::: browser/base/content/test/browser_pluginnotification.js @@ +213,5 @@ > + var doc = gTestBrowser.contentDocument; > + var rect = doc.getAnonymousElementByAttribute(plugin1, "class", "mainBox").getBoundingClientRect(); > + ok(rect.width == 200, "Test 9a, Plugin overlay rect should have 200px width before being clicked"); > + ok(rect.height == 200, "Test 9a, Plugin overlay rect should have 200px height before being clicked"); > + Why not also check the plugin.activated state in these tests? @@ +239,5 @@ > + var plugin3Rect = doc.getAnonymousElementByAttribute(plugin3, "class", "mainBox").getBoundingClientRect(); > + ok(plugin3Rect.width == 0, "Test 9b, Plugin3 should have click-to-play overlay with zero width"); > + ok(plugin3Rect.height == 0, "Test 9b, Plugin3 should have click-to-play overlay with zero height"); > + > + Services.prefs.clearUserPref("plugins.click_to_play"); use registerCleanupFunction at the beginning of test() to reset this pref

Try run for 1cc825e6fb87 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=1cc825e6fb87 Results (out of 287 total builds): success: 260 warnings: 23 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-1cc825e6fb87

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=f9cb29edc917

http://hg.mozilla.org/integration/mozilla-inbound/rev/fed1018b5842 http://hg.mozilla.org/integration/mozilla-inbound/rev/dfddfef84096

http://hg.mozilla.org/mozilla-central/rev/fed1018b5842 http://hg.mozilla.org/mozilla-central/rev/dfddfef84096

Thanks for this! I've traded Flashblock for plugins.click_to_play;true on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120406 Firefox/14.0a1 ID:20120406031222 and like it for the most part. My only complaints are: - bug 742753 - an "Always activate plugins" option in the door hanger or - a whitelist Also, it might be Oracle's fault, but the behavior of clicking "Do I have Java?" at http://java.com/en/ with this feature on is less than ideal. Should follow up bugs be filled? Test URLs: http://grooveshark.com http://www.adobe.com/software/flash/about/ http://javatester.org/version.html http://java.com/en/download/installed.jsp

(In reply to alex_mayorga from comment #62) > Thanks for this! > > I've traded Flashblock for plugins.click_to_play;true on Mozilla/5.0 > (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120406 Firefox/14.0a1 > ID:20120406031222 and like it for the most part. > > My only complaints are: > - bug 742753 > - an "Always activate plugins" option in the door hanger or > - a whitelist > > Also, it might be Oracle's fault, but the behavior of clicking "Do I have > Java?" at http://java.com/en/ with this feature on is less than ideal. > > Should follow up bugs be filled? > > Test URLs: > http://grooveshark.com > http://www.adobe.com/software/flash/about/ > http://javatester.org/version.html > http://java.com/en/download/installed.jsp Alex, thanks for the feedback ! please see https://wiki.mozilla.org/Opt-in_activation_for_plugins which described the overall plan/current proposal for click to play - a whitelist is definitely a strong possibility. the feature proposal is currently being discussed on mozilla.dev.security and we asked the Mozilla UX team for feedback yesterday, also. > Also, it might be Oracle's fault, but the behavior of clicking "Do I have > Java?" at http://java.com/en/ with this feature on is less than ideal. is this the same as the problem with silverlight described in bug 743102 ? from comment 4 in that bug it sounds like it might be. Please note that bug 738698 is the meta bug for 'click to play' :)

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)

A whitelist may be a required feature if at least to cope with any sites that simply don't work correctly with click-to-play. For example, on http://nyan.cat/ the sound doesn't work with click-to-play, either this implementation or Flashblock. Whitelisting it is required to get it working and there are other sites where this may be the case too.

(In reply to Dave Garrett from comment #65) > A whitelist may be a required feature if at least to cope with any sites > that simply don't work correctly with click-to-play. For example, on > http://nyan.cat/ the sound doesn't work with click-to-play, either this > implementation or Flashblock. Whitelisting it is required to get it working > and there are other sites where this may be the case too. i see whitelisting as more meaning 'this plugin is always allowed to play by default (without a click)' across all sites. i think we definitely want to have an easy way for a user to choose 'always allow this plugin to play by default on a specific site as well. This is covered as a use case (imo) with "User is tired of always clicking to play a given plugin (i.e. YouTube, or their favorite Java game site)" in https://wiki.mozilla.org/Opt-in_activation_for_plugins although the exact UX around this use case is still being discussed on mozilla.dev.security Please note that bug 738698 is the meta bug for 'click to play' and this bug is marked fixed - mozilla.dev.security is probably a better place to discuss click to play overall. Thanks !

I wish we have better UI when click-to-play is enabled. Some thing like a play button [ > ] on a semi-opaque background to replace the plug-in display area. Do we have a bug on that?

(In reply to Biju from comment #67) > I wish we have better UI when click-to-play is enabled. > Some thing like a play button [ > ] on a semi-opaque background to replace > the plug-in display area. > > Do we have a bug on that? I'm not sure what you mean here. Do you want to just replace what is there now with a play button? Or are you proposing a mechanism that loads the plugin, "pauses" it, and throws the transparent overlay on top? The latter is probably not desirable because it does away with the security and resource-management gains we get from this feature.

(In reply to David Keeler from comment #68) > I'm not sure what you mean here. Do you want to just replace what is there > now with a play button? Make it beautiful(yet light) so has it looks cool/pretty/appealing to end users >Or are you proposing a mechanism that loads the >plugin, "pauses" it, and throws the transparent overlay on top? > The latter is probably not desirable because it does away with the security > and resource-management gains we get from this feature. Touche

Probably better to file a new bug on that and to leave this bug as it is.

I too have been thinking the click-to-play icon could be made more clear. I've filed bug 746859 to ask that it be changed from just the standard plugin icon to one a bit larger with a play icon. It'd be a simple change that would let users quickly see what's going on without having to read the text.

(In reply to Dave Garrett from comment #71) > I too have been thinking the click-to-play icon could be made more clear. > I've filed bug 746859 to ask that it be changed from just the standard > plugin icon to one a bit larger with a play icon. It'd be a simple change > that would let users quickly see what's going on without having to read the > text. Thanks for your feedback, Dave, I moved the dependency on bug 746859 which you filed over to bug 738698 which is the overall tracking bug for click to play, as this bug is already marked RESOLVED FIXED.

(In reply to Ian Melven :imelven from comment #72) > as this bug is already marked RESOLVED FIXED. This doesn't really matter.

This a feature to many people like, and I want ask to you if this will come activated on Firefox 14? Too I have a idea, why not create a global configuration on about:permissions like passwords, cookies, shared location and others?

(In reply to Yunier José Sosa Vázquez from comment #74) > This a feature to many people like, and I want ask to you if this will come > activated on Firefox 14? This feature will be disabled by default in Firefox 14 > Too I have a idea, why not create a global configuration on > about:permissions like passwords, cookies, shared location and others? A global configuration was just added to about:permissions in bug 711618 :)

(In reply to Jared Wein [:jaws] from comment #75) > (In reply to Yunier José Sosa Vázquez from comment #74) > > This a feature to many people like, and I want ask to you if this will come > > activated on Firefox 14? > > This feature will be disabled by default in Firefox 14 > > > Too I have a idea, why not create a global configuration on > > about:permissions like passwords, cookies, shared location and others? > > A global configuration was just added to about:permissions in bug 711618 :) Great! But I wished this would have been enabled by default in Firefox 14. I'll see that many users know this to activate plugins.click_to_play == true ;-)

(In reply to Yunier José Sosa Vázquez from comment #76) > (In reply to Jared Wein [:jaws] from comment #75) > > (In reply to Yunier José Sosa Vázquez from comment #74) > > > This a feature to many people like, and I want ask to you if this will come > > > activated on Firefox 14? > > > > This feature will be disabled by default in Firefox 14 > > > > > Too I have a idea, why not create a global configuration on > > > about:permissions like passwords, cookies, shared location and others? > > > > A global configuration was just added to about:permissions in bug 711618 :) > > Great! But I wished this would have been enabled by default in Firefox 14. > I'll see that many users know this to activate plugins.click_to_play == true > ;-) Thanks Yunier ! bug 738698 is the tracking bug for click to play. Also, please see https://wiki.mozilla.org/Opt-in_activation_for_plugins for some other ideas we are exploring or planning to implement - there's a lot to discuss and decide before click to play is turned on by default :)

Sure Ian, I help in all whatever it takes. I commented here https://bugzilla.mozilla.org/show_bug.cgi?id=738698#c12