In today's nightly (2012-12-10) the Click-to-play doorhanger is appearing on every page with flash after a page load or reload.

On this YT page e.g., it's ok like previously: http://www.youtube.com/watch?v=gFmuNApHFec but not on this streaming site: http://www.jango.com/music/Rihanna?l=0

Not sure if related, but this appeared today here: - go to http://imgur.com/ - see the popup, click "never activate plugin for this site" - reload - popup appears again

We've discussed how to resolve this. 1) Let's do bug 820109 only once per browser session 2) Let's change the icon from the blue plugin to red blocked plugin in the awesomebar 3) Whenever the red blocked plugin icon is visible, let's animate it (fade out, fade in, fade out, fade in) Thanks for taking this on Jared!

We're targeting this for landing in tomorrow's beta, if at all possible.

(In reply to Paul Rouget [:paul] from comment #2) > Not sure if related, but this appeared today here: > - go to http://imgur.com/ > - see the popup, click "never activate plugin for this site" > - reload > - popup appears again These steps do not reproduce for me. May be the difference between enabling CTP manually and having a blocklisted Flash version. Please file a separate bug.

I'm in the middle of a clobber build so this patch is untested, but it *should* work as akeybl had described in the above comment. Requesting review before I can test it so that I can get a quicker turn-around-time on it to land it in time for beta.

(In reply to Alex Keybl [:akeybl] from comment #3) > We've discussed how to resolve this. > > 1) Let's do bug 820109 only once per browser session > 2) Let's change the icon from the blue plugin to red blocked plugin in the > awesomebar > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade > out, fade in, fade out, fade in) > > Thanks for taking this on Jared! Is your saying to resolve fix showing doorhanger popup on many pages which is talked in bug 810082 ? If it is so, I think fixing it sufficiency that we'll change to no showing popup when in normal CTP which is not blocking outdated plugins. If we talk about the annoying behavior reported in bug 810082 comment #35, the root problem is that it's very annoying the showing doorhanger popup steals user focusing. We should fix it.

Comment on attachment 690704 [details] [diff] [review] Patch (untested) Review of attachment 690704 [details] [diff] [review]: ----------------------------------------------------------------- This code will not work as your expect. ::: browser/base/content/browser-plugins.js @@ +649,3 @@ > PopupNotifications.show(aBrowser, "click-to-play-plugins", > messageString, "plugins-notification-icon", > mainAction, secondaryActions, options); you need to like this: PopupNotifications.show(aBrowser, "click-to-play-plugins", messageString, icon, mainAction, secondaryActions, options);

Thanks for catching that!

Comment on attachment 690734 [details] [diff] [review] Patch v.2 (untested) >+ _notificationDisplayed: false, please rename this to _notificationDisplayedOnce > notification.dismissed = false; > PopupNotifications._update(notification.anchorElement); >+ !this._notificationDisplayed = true; remove exclamation mark >+#blocked-plugins-notification-icon[showing] { >+ animation: fadeIn 500ms ease 0s 4 alternate both; >+} >+ >+@keyframes fadeIn { >+ from { >+ opacity: 0; >+ } >+ >+ to { >+ opacity: 1; >+ } >+} Rename this animation such that the name can't clash with random other animations and is linked to where this animation is used (i.e. blocked-plugins-notification-icon). >--- a/browser/themes/pinstripe/browser.css >+++ b/browser/themes/pinstripe/browser.css >+#blocked-plugins-notification-icon { >+ list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png); This isn't the red icon on OS X.

Comment on attachment 690734 [details] [diff] [review] Patch v.2 (untested) Review of attachment 690734 [details] [diff] [review]: ----------------------------------------------------------------- I comment with assumption which this patch is related to handle the feature of blocking vulnerable plugin. ::: browser/base/content/browser-plugins.js @@ +283,3 @@ > notification.dismissed = false; > PopupNotifications._update(notification.anchorElement); > + !this._notificationDisplayed = true; this._notificationDisplayed should not be configurated in this part. This part checks whether a scripted plugin is visible or not. @@ +649,1 @@ > PopupNotifications.show(aBrowser, "click-to-play-plugins", If we change this._notificationDisplayed to handle blocking VulnerablePlugin, I think we should refer this._notificationDisplayed in this.

Bug 820348 has been duplicated. But I seem that the patch which Jared attached is only related to blocking outdated plugins. Jared, will your next/other patch fix that showing many popup is very annoying when normal Click-To-Play? If you have no plan now (you'll work on other bug), may I file the new bug?

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #13) > Bug 820348 has been duplicated. > But I seem that the patch which Jared attached is only related to blocking > outdated plugins. > > Jared, will your next/other patch fix that showing many popup is very > annoying when normal Click-To-Play? > If you have no plan now (you'll work on other bug), may I file the new bug? Please file a new bug for general click-to-play.

(In reply to Jared Wein [:jaws] from comment #14) > Please file a new bug for general click-to-play. OK. I filed bug 820437 ;)

Thanks for the quick review Dao and feedback from :saneyuki_s. This solution also fixes bug 820437 so I'll dupe that here.

Comment on attachment 690981 [details] [diff] [review] Patch v.3 >+#blocked-plugins-notification-icon[showing] { >+ animation: flashPluginBlockedNotification 500ms ease 0s 5 alternate both; >+} >+ >+@keyframes flashPluginBlockedNotification { "flashPlugin" sounds like you're referring to Adobe Flash. Let's just call this "pluginBlockedNotification". >+ from { >+ opacity: 0; >+ } >+ >+ to { >+ opacity: 1; >+ } >+} nit: remove blank line >--- a/toolkit/themes/pinstripe/mozapps/jar.mn >+++ b/toolkit/themes/pinstripe/mozapps/jar.mn >@@ -63,16 +63,17 @@ toolkit.jar: > skin/classic/mozapps/plugins/notifyPluginBlocked.png (plugins/notifyPluginGeneric.png) > skin/classic/mozapps/plugins/notifyPluginCrashed.png (plugins/notifyPluginGeneric.png) > skin/classic/mozapps/plugins/notifyPluginGeneric.png (plugins/notifyPluginGeneric.png) > skin/classic/mozapps/plugins/notifyPluginOutdated.png (plugins/notifyPluginGeneric.png) > skin/classic/mozapps/plugins/pluginProblem.css (plugins/pluginProblem.css) > skin/classic/mozapps/plugins/pluginGeneric.png (plugins/pluginGeneric.png) > skin/classic/mozapps/plugins/pluginDisabled.png (plugins/pluginDisabled.png) > skin/classic/mozapps/plugins/pluginBlocked.png (plugins/pluginBlocked.png) >+ skin/classic/mozapps/plugins/pluginBlocked-16.png (plugins/pluginBlocked-16.png) It's confusing that this is packed as notifyPluginBlocked.png in winstripe and gnomestripe but pluginBlocked-16.png in pinstripe, while pinstripe also packages notifyPluginBlocked.png but uses it differently.

(In reply to Dão Gottwald [:dao] from comment #18) > >+@keyframes flashPluginBlockedNotification { > > "flashPlugin" sounds like you're referring to Adobe Flash. Let's just call > this "pluginBlockedNotification". Good call, thanks. > It's confusing that this is packed as notifyPluginBlocked.png in winstripe > and gnomestripe but pluginBlocked-16.png in pinstripe, while pinstripe also > packages notifyPluginBlocked.png but uses it differently. Yeah, I tried to fix it in this revision by using the correct icon for notifyPluginBlocked.png on pinstripe as well as including the 16px version for pluginBlocked-16.png. Changing notifyPluginBlocked.png to use the 16px gets confusing because it looks to be used for notification bars and there doesn't appear to be an easy way to dynamically switch that icon for HiDPI (maybe navigator.platform.contains("Mac") && window.matchMedia("(min-resolution: 2dppx)")).

Comment on attachment 691036 [details] [diff] [review] Patch v4 >+@media (min-resolution: 2dppx) { >+ #blocked-plugins-notification-icon { >+ list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png); Isn't this a 16px icon? That is, if it existed (see below), wouldn't it be a 16px icon? >--- a/toolkit/themes/pinstripe/mozapps/jar.mn >+++ b/toolkit/themes/pinstripe/mozapps/jar.mn >- skin/classic/mozapps/plugins/notifyPluginBlocked.png (plugins/notifyPluginGeneric.png) >+ skin/classic/mozapps/plugins/notifyPluginBlocked.png (plugins/notifyPluginBlocked.png) toolkit/themes/pinstripe/mozapps/plugins/notifyPluginBlocked.png doesn't exist, as far as I can tell.

Sigh, sorry for missing that.

[Approval Request Comment] Bug caused by (feature/regressing bug #): bug 810082 User impact if declined: the plugin popup appears too often Testing completed (on m-c, etc.): tested locally in a beta build Risk to taking this patch (and alternatives if risky): click-to-play/plugin blocklisting notification weirdness, none expected though String or UUID changes made by this patch: none

Comment on attachment 691103 [details] [diff] [review] Patch for beta and aurora This patch also applies to aurora.

Sorry, had to back this out for test failures. Note that typically it's preferred that a patch land on m-c first before going onto the branches. https://hg.mozilla.org/releases/mozilla-beta/rev/a3e25b271de5 https://tbpl.mozilla.org/php/getParsedLog.php?id=17843525&tree=Mozilla-Beta TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | waited too long for popup notification to show (plugin_test_scriptedPopup2.html) TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | notification should be showing (plugin_test_scriptedPopup2.html) TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | waited too long for popup notification to show (plugin_test_scriptedPopup1.html) TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | notification should be showing (plugin_test_scriptedPopup1.html)

Comment on attachment 691041 [details] [diff] [review] Patch v5 Review of attachment 691041 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-plugins.js @@ +286,1 @@ > } this._notificationDisplayedOnce should be checked *at first*. Because, after this._notificationDisplayedOnce=true, we need not to check haveVisibleCTPPlugin. So like this: > if (!this._notificationDisplayedOnce) { > ... > ... > if (notification && !haveVisibleCTPPlugin) { > ... > } > }

Relanded with test fixes on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/e904ee016207 https://hg.mozilla.org/releases/mozilla-aurora/rev/cb34bd8957ec https://hg.mozilla.org/integration/mozilla-inbound/rev/2605aa5980f9 Sorry about landing on beta so quick.

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #26) > Comment on attachment 691041 [details] [diff] [review] > Patch v5 > > Review of attachment 691041 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/browser-plugins.js > @@ +286,1 @@ > > } > > this._notificationDisplayedOnce should be checked *at first*. Because, after > this._notificationDisplayedOnce=true, we need not to check > haveVisibleCTPPlugin. > > So like this: > > > if (!this._notificationDisplayedOnce) { > > ... > > ... > > if (notification && !haveVisibleCTPPlugin) { > > ... > > } > > } This won't break correctness, but is a nice optimization we can make. Can you file a follow-up to make this change?

I tried patch v5 attachment 691041 [details] [diff] [review]. below is my feeling. (In reply to Alex Keybl [:akeybl] from comment #3) > We've discussed how to resolve this. > > 1) Let's do bug 820109 only once per browser session > 2) Let's change the icon from the blue plugin to red blocked plugin in the > awesomebar > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade > out, fade in, fade out, fade in) I think these change are good at blocking outdated plugins automatically. These indicate more clear information to user. However, at normal click-to-play, I feel this change (1) is not good. A user using normal click-to-play are regarded as that they understand the feature of normal click-to-play, because it is an opt-in feature. So showing the doorhanger popup per browser window might be very annoying for user who is used to use normal opt-in click-to-play.

(In reply to Jared Wein [:jaws] from comment #28) > This won't break correctness, but is a nice optimization we can make. Can > you file a follow-up to make this change? I filed bug 820678.

I've been testing this today on FF 18b4: (In reply to Alex Keybl [:akeybl] from comment #3) > We've discussed how to resolve this. > 1) Let's do bug 820109 only once per browser session > 2) Let's change the icon from the blue plugin to red blocked plugin in the > awesomebar a) The red icon is outdated plugins related only, the blue one still appears for normal plugins. Also this bug is flash related, but the red icon is displayed for other outdated plugins. > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade > out, fade in, fade out, fade in) b) The fading animation is also only once per session c) https://bugzilla.mozilla.org/show_bug.cgi?id=809846#c15 d) https://bugzilla.mozilla.org/show_bug.cgi?id=775857#c4

Any objections considering comment 32?

(In reply to Paul Silaghi [QA] from comment #32) > I've been testing this today on FF 18b4: > > (In reply to Alex Keybl [:akeybl] from comment #3) > > We've discussed how to resolve this. > > 1) Let's do bug 820109 only once per browser session > > 2) Let's change the icon from the blue plugin to red blocked plugin in the > > awesomebar > a) The red icon is outdated plugins related only, the blue one still appears > for normal plugins. Also this bug is flash related, but the red icon is > displayed for other outdated plugins. Yes, this is intended. > > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade > > out, fade in, fade out, fade in) > b) The fading animation is also only once per session This is happening once per window per session. I'm not sure why the animation doesn't happen each time. We should file a bug on it to investigate further. > c) https://bugzilla.mozilla.org/show_bug.cgi?id=809846#c15 I reopened that bug based on your comment there. > d) https://bugzilla.mozilla.org/show_bug.cgi?id=775857#c4 We don't have a way to override blocklisted plugin warnings on a per-domain basis.

(In reply to Jared Wein [:jaws] from comment #37) > > > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade > > > out, fade in, fade out, fade in) > > b) The fading animation is also only once per session > > This is happening once per window per session. I'm not sure why the > animation doesn't happen each time. We should file a bug on it to > investigate further. I filed bug 825035 for this.

I think before verifying this it would be better to wait for bug 825035, bug 809846 to be fixed and for feedback in bug https://bugzilla.mozilla.org/show_bug.cgi?id=810082#c69 which is part of this.

Verified fixed FF 19, 20b1, 17.0.3 ESR.

Actually there is a problem here. I don't understand why the first time CTP notification is not displayed on Ubuntu on FF 20, 21, 22, but is displayed on FF 19, 17.0.3 ESR. On all other OSes works fine on all FFs. Tested on http://imgur.com/