If you disable QuickTime on the attached link, the box is just black with no indication of the plugin being disabled. There should be something like the plugin finder service (placeholder telling you to install the plugin) letting you that you have disabled the plugin and something there to enable it. One thing it should not have is the information bar at the top like the plugin finder service does.

I believe in the case of that link the black box is due to there being an embed that specifies height and width. In the case of the following link it doesn't display a black box. http://www.student.oulu.fi/%7esairwas/object-test/video/qt1.html

Is this the same on all plaforms or only windows where there is only a black box? Just wondering why there hasn't been a lot of dupes, me toos, etc..

This issue came up in the forums here: http://forums.mozillazine.org/viewtopic.php?t=620879 This is going to become a possible support headache when someone disables a plugin, then forgets it was disabled, then comes asking for help - "stating that the plugin no longer does anything."

Would be nice to improve this..

I really feel this is going to be a huge support headeache with loads of "XXX plugin not working" We really going to go at least another two years (judging by the mozilla 2 plans that I have seen) to not include some sort of placeholder saying the user has disabled this extension, click this to renable?

David, is this something you could work on? Not the highest of priorities, but it would be nice to improve the user experience with disabled plugins here.

Will take a look and see what I can do

Just done a quick review of the notifications we give for the various cases: When a plugin is included using an OBJECT tag, we seem to never display the plugin binding in content, at least from what I could see. The notification bar is displayed in the following cases: | Empty tag | Fallback content | -------------+-------------+--------------------+ Unknown | Yes | Yes | | | | Blocklisted | No | Yes | | | | Disabled | No | No | When a plugin is included using an EMBED tag there is no fallback content. We notify in the following ways: | In page display | Notification bar | -------------+-------------------+--------------------+ Unknown | Yes | Yes | | | | Blocklisted | No | Yes | | | | Disabled | No | No | I think first we want to identify what about those sets we want to change.

(In reply to comment #10) > Just done a quick review of the notifications we give for the various cases: > > When a plugin is included using an OBJECT tag, we seem to never display the > plugin binding in content, at least from what I could see. The notification bar > is displayed in the following cases: > > | Empty tag | Fallback content | > -------------+-------------+--------------------+ > Unknown | Yes | Yes | > | | | > Blocklisted | No | Yes | > | | | > Disabled | No | No | The use cases here are a little murky. Disabling plugins can (and is) often done to prevent annoyances, but I think that providing an understanding of what annoyances are being avoided is worthwhile, so showing an empty tag seems sensible there. Same with blocklisting. Though we should probably use different placeholder graphics for the three cases (like ? ! X) Unknown could result in a bunch of "hey, do you want vendor-specific plugin XYZ?" overload, but the user did go to the site, and the user is similarly welcome to leave it. So, then, I think that in all cases we should show an empty tag. Only in the case of blocklisting will we probably also want to show a notification bar since the disabling action not taken by the user. I don't think we want to show the notification bar in the unknown case as the author presumably used an OBJECT tag so they could employ fallback content. Perhaps if there's no available fallback content, we could then bring in the notification bar as an offer? > When a plugin is included using an EMBED tag there is no fallback content. We > notify in the following ways: > > | In page display | Notification bar | > -------------+-------------------+--------------------+ > Unknown | Yes | Yes | > | | | > Blocklisted | No | Yes | > | | | > Disabled | No | No | > > I think first we want to identify what about those sets we want to change. Again, I'd think we'd want to always show the user something about where the missing content would have been placed. So assuming in page display means "draw a square where it would have been showing that something's not being rendered there" then I think it should be "yes" for all these cases. I agree that disabled plugins shouldn't bring down the notification bar. cc'ing mconnor as he's thinking a lot about this stuff these days, and alex as there's a hidden icon request in here!

This is the patch for the content side. This sets us up so that for the three distinct error states for plugin tags, unknown, disabled and blocklisted we dispatch the error events PluginNotFound, PluginDisabled, PluginBlocklisted and attach the pseudoclasses -moz-type-unsupported, -moz-handler-disabled, -moz-handler-blocked respectively. This allows the frontend to attach different bindings and respond differently for each case. The implementation is fairly self-explanatory I think, we just retain a PluginSupportState instead of mTypeUnsupported. To clarify this is the precise behaviour that exists with this patch for failing plugins: embed tags always have the pseudo-class attached and dispatch error events. object tags with no significant fallback content or with a pluginurl param have the psuedo-classes attached and dispatch error events. object tags with significant fallback content and no pluginurl param do not have the psuedo-classes attached or dispatch error events. We currently behave slightly differently to this, object tags with fallback content that are not loaded from a channel still dispatch errors. I believe that is an error and have fixed the testcase from bug 425013 accordingly. In order to test this I have included a simple test plugin. It is about as simple as a plugin can get, it is enough for the plugin registry to correctly detect it on the main 3 platforms, but not enough to actually instantiate on a page, but that is all that is needed for the tests. I was considering dispatching the error event at all times since that could give the frontend more capabilities for dealing with blocked/disabled plugins that have fallback, but I think that needs more thought. The mochitest runs through all the cases I could think of to verify the above behaviour for working and all 3 failure states. One final issue is that I needed to move the test for VerifiedDownload.plugin in order to make the tests work. What is happening at the moment is everytime the plugin host checks for new plugins (pretty much every time a plugin tag is encountered) it skims through the file lists and finds VerifiedDownload.plugin as a potential new plugin, only later is it discarded when we cancel the load. This is enough to dispatch the plugins-list-changed notification which causes the blocklist service to reset the blocklist status on all plugins. This was happening in the middle of the test causing failures, it is also likely causing a small unnecessary perf hit whenever we load a page containing plugins. By moving the test to IsPluginFile we avoid all this by just ignoring the file during the initial file scan.

Just a couple of typo fixes...

This is the frontend handling that I have put together. The unknown plugin case behaves the same as currently. The blocklisted case adds in-page display of the blocklisting, but clicking on the message does nothing, perhaps we want to load the blocklist details page there? The disabled case does not display any notification bar, but clicking on the in-page display will open the add-ons manager to the plugins pane.

Comment on attachment 334257 [details] [diff] [review] content patch rev 1 >+++ b/content/base/src/nsObjectLoadingContent.cpp >+ PluginSupportState mPluginState; I would prefer that this remain private, with a setter. Perhaps even a setter that can assert that the caller is not passing in ePluginOtherState, etc? >@@ -887,18 +888,26 @@ nsObjectLoadingContent::ObjectState() co > case eType_Null: > if (mSuppressed) > return NS_EVENT_STATE_SUPPRESSED; > if (mUserDisabled) > return NS_EVENT_STATE_USERDISABLED; ... >+ case ePluginDisabled: >+ state |= NS_EVENT_STATE_HANDLER_DISABLED; So how does NS_EVENT_STATE_HANDLER_DISABLED differ from NS_EVENT_STATE_USERDISABLED, exactly? I would have thought they would be identical... We really don't want to add bits here unless we actually need to. If we need to refactor existing bits to better express things, maybe we need to do that. I don't see any CSS changes to actually make use of your new pseudo-classes. Are they not needed? I didn't look at the tests. I assume they're reasonable.

(In reply to comment #16) > (From update of attachment 334257 [details] [diff] [review]) > >+++ b/content/base/src/nsObjectLoadingContent.cpp > >+ PluginSupportState mPluginState; > > I would prefer that this remain private, with a setter. Perhaps even a setter > that can assert that the caller is not passing in ePluginOtherState, etc? I'll implement that. > >@@ -887,18 +888,26 @@ nsObjectLoadingContent::ObjectState() co > > case eType_Null: > > if (mSuppressed) > > return NS_EVENT_STATE_SUPPRESSED; > > if (mUserDisabled) > > return NS_EVENT_STATE_USERDISABLED; > ... > >+ case ePluginDisabled: > >+ state |= NS_EVENT_STATE_HANDLER_DISABLED; > > So how does NS_EVENT_STATE_HANDLER_DISABLED differ from > NS_EVENT_STATE_USERDISABLED, exactly? I would have thought they would be > identical... The problem I hit was that NS_EVENT_STATE_USERDISABLED and NS_EVENT_STATE_SUPPRESSED which I agree seem perfectly named for the job are passed out as a result of content policies being in effect (either rejecting all plugins, or rejecting the server). This means that were we to reuse them plugins blocked by adblock and related extensions would claim to be blocklisted and so on. It seemed to me that being blocked by content policies were different enough to being blocked by user disabled/blocklisted plugins to need a different state > We really don't want to add bits here unless we actually need to. If we need > to refactor existing bits to better express things, maybe we need to do that. > > I don't see any CSS changes to actually make use of your new pseudo-classes. > Are they not needed? There is another frontend patch to come (assuing the ui-r goes through) that hooks up the new pseudo-classes. I can attach what I have now if needs be.

Yeah, I realize that those bits are used by content policy. Are we really using them in a sufficiently different way here? I guess we don't really want SUPPRESSED since we want UI in all these cases, right? But is there really a difference from our point of view between "user disabled this specific plug-in" and "user disabled all plug-ins"?

I think the problem comes in the frontend. It is quite important that the user knows that all plugins are disabled or just specific plugins are disabled, if only because for the latter we know what to do, open the add-ons manager where they have control. The former is something that would happen with certain add-ons installed or certain preferences set. The UI there ought to be different (though provided by the extension or whatever). I guess maybe the binding could do a content policy check to see which disabled case it is being displayed for, but that doesn't seem very efficient to me.

No, that's certainly the wrong way to go. On the other hand, it sorta feels like we're wasting a lot of information here: we have N bits, but nowhere close to 2^N possible states, right? Can we somehow rationalize the bits better?

I guess looking at it that way we have 6 states (working, unknown, plugin disabled, blocklisted, plugins disabled, server blocked), and they are exclusive. Should be able to fit that into 3 bits. If that makes sense I could work on adjusting things to fit that.

Well I guess they aren't really exclusive, but if we assume we are only ever going to pass through one state (as we do currently anyway) then that holds.

We should base the bits on exclusivity (so unknown excludes plugin disabled or blocklisted, but blocklisted doesn't exclude disabled)... If it turns out we do need all these bits, then we need them.

>cc'ing mconnor as he's thinking a lot about this stuff these days, and alex as >there's a hidden icon request in here! Adding three new icons to the inventory: -plugin unknown -plugin disabled -plugin blocklisted

We currently pass through 3 different errors from the content policy (REJECT_SERVER, REJECT_TYPE, then any other rejection). These are exclusive as the content policy only returns one failure case. As you mention we can't have a disabled or blocklisted plugin when there is no plugin, however we can have the content policy errors at the same time as any of those. This I think gives 19 different cases, which we could potentially pass through in 5 bits (1 less than this patch uses). As the code stands at the moment we'll never pass out the unknown/disabled/blocklisted states when a content policy rejection happened, and we cannot determine from the plugin host that a plugin is both blocklisted and disabled so we won't really be using all of the cases, but I guess it is possible in the future that we might want to pass out that much detail, though just losing the disabled+blocklisted case saves us an additional bit. I'll look at just how much more complex it gets to save that bit.

It doesn't sound like it's worth it. I'll review as-is. Thanks for looking through that stuff!

This is the same stuff, just implemented the public setter as requested.

Comment on attachment 334743 [details] [diff] [review] content patch rev 2 [checked in] r=jst, but you'll need to make sure the test plugin still applies and lives in the right place after Josh's plugin dirs reorg.

Could you please hold off on landing this until I've had a chance to take a look? Thanks.

+ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT))) No need to include mac in that, just do "ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)". Looks good, thanks.

Comment on attachment 334258 [details] frontend display Boriss, this is I think what we want on the frontend side, is this ok by you? The shot shows three different cases, top to bottom they are plugin not installed, plugin disabled and plugin blocklisted.

Comment on attachment 334743 [details] [diff] [review] content patch rev 2 [checked in] Landed the content patch: http://hg.mozilla.org/mozilla-central/rev/4b9f9c1f7e1d Waiting on ui-review before posting the frontend work.

(In reply to comment #32) > Landed the content patch: > http://hg.mozilla.org/mozilla-central/rev/4b9f9c1f7e1d Mossop - These look pretty good, just had a couple questions: - Some minor improvements on the blocklisted use case could go a long way to give explanation to the user (see #12) and read a bit less authoritarian. In the attachment, this is the only use case there the user is given nowhere to go - no link to an add-ons manager nor explanation of what's going on. So, what are the possibly reasons that a plugin would have been blocked? Could it be that the user has blocked it in the add-ons manager, rather than any protection issue? And if so, as Mossop mentions in #19, can we open the add-ons manager and allow the user to enable the plug-in? - Is it possible to center the icon and text message, horizontally and vertically, in the content space?

(In reply to comment #33) > These look pretty good, just had a couple questions: > > - Some minor improvements on the blocklisted use case could go a long way to > give explanation to the user (see #12) and read a bit less authoritarian. In > the attachment, this is the only use case there the user is given nowhere to go Ok, I hadn't realised the screenshot wasn't full width. For the notification bar for hte blocked case there are two buttons on the right. One to learn more (which loads the blocklist webpage with details about blocks in effect) and one to search for updated plugins. > - no link to an add-ons manager nor explanation of what's going on. So, what > are the possibly reasons that a plugin would have been blocked? Could it be > that the user has blocked it in the add-ons manager, rather than any protection > issue? And if so, as Mossop mentions in #19, can we open the add-ons manager > and allow the user to enable the plug-in? Currently the user cannot blocklist or un-blocklist a plugin, only disable. For the disabled case as shown in the screenshot you can click on the placeholder and get the add-ons manager up. > - Is it possible to center the icon and text message, horizontally and > vertically, in the content space? Maybe, I'll tinker with that.

> For the notification > bar for hte blocked case there are two buttons on the right. One to learn more > (which loads the blocklist webpage with details about blocks in effect) and one > to search for updated plugins. Sounds good. How do you feel about changing the wording to just say this plug-in has been disabled? If the user can disable a plugin, it seems we can't really assume that it's been disabled for their protection.

Content patch rev 2 looks good! Thanks Mossop for clearing up that the attached screenshot hid two available user options on the blocked case.

Comment on attachment 334743 [details] [diff] [review] content patch rev 2 [checked in] 33 tests fails on 64 linux: not ok - Should have been blocked plugins got 1, expected 14 ok - Plugin 1 did not send the event not ok - Plugin 2 did not send the event not ok - Plugin 3 did not send the event not ok - Plugin 4 did not send the event not ok - Plugin 5 did not send the event not ok - Plugin 6 did not send the event not ok - Plugin 7 did not send the event not ok - Plugin 8 did not send the event not ok - Plugin 9 did not send the event not ok - Plugin 10 did not send the event not ok - Plugin 11 did not send the event not ok - Plugin 12 did not send the event not ok - Plugin 13 did not send the event not ok - Plugin 14 did not send the event ok - Fallback plugin 1 should not have sent the event ok - Fallback plugin 2 should not have sent the event ok - Fallback plugin 3 should not have sent the event ok - Fallback plugin 4 should not have sent the event ok - Plugin 1 did not exist ok - Plugin 1 had an incorrect border style ok - Plugin 2 did not exist not ok - Plugin 2 had an incorrect border style got "solid", expected "dashed" ok - Plugin 3 did not exist not ok - Plugin 3 had an incorrect border style got "solid", expected "dashed" ok - Plugin 4 did not exist not ok - Plugin 4 had an incorrect border style got "solid", expected "dashed" ok - Plugin 5 did not exist not ok - Plugin 5 had an incorrect border style got "solid", expected "dashed" ok - Plugin 6 did not exist not ok - Plugin 6 had an incorrect border style got "solid", expected "dashed" ok - Plugin 7 did not exist not ok - Plugin 7 had an incorrect border style got "solid", expected "dashed" ok - Plugin 8 did not exist not ok - Plugin 8 had an incorrect border style got "solid", expected "dashed" ok - Plugin 9 did not exist not ok - Plugin 9 had an incorrect border style got "solid", expected "dashed" ok - Plugin 10 did not exist not ok - Plugin 10 had an incorrect border style got "solid", expected "dashed" ok - Plugin 11 did not exist not ok - Plugin 11 had an incorrect border style got "solid", expected "dashed" ok - Plugin 12 did not exist not ok - Plugin 12 had an incorrect border style got "solid", expected "dashed" ok - Plugin 13 did not exist not ok - Plugin 13 had an incorrect border style got "solid", expected "dashed" ok - Plugin 14 did not exist not ok - Plugin 14 had an incorrect border style got "solid", expected "dashed" ok - Fallback plugin 1 did not exist ok - Fallback plugin 1 had an incorrect border style ok - Fallback plugin 2 did not exist ok - Fallback plugin 2 had an incorrect border style ok - Fallback plugin 3 did not exist ok - Fallback plugin 3 had an incorrect border style ok - Fallback plugin 4 did not exist ok - Fallback plugin 4 had an incorrect border style not ok - Plugin lost its blocklist status not ok - Should have been unknown plugins got 12, expected 14 ok - Plugin 1 did not send the event ok - Plugin 2 did not send the event ok - Plugin 3 did not send the event ok - Plugin 4 did not send the event not ok - Plugin 5 did not send the event not ok - Plugin 6 did not send the event ok - Plugin 7 did not send the event ok - Plugin 8 did not send the event ok - Plugin 9 did not send the event ok - Plugin 10 did not send the event ok - Plugin 11 did not send the event ok - Plugin 12 did not send the event ok - Plugin 13 did not send the event ok - Plugin 14 did not send the event ok - Fallback plugin 1 should not have sent the event ok - Fallback plugin 2 should not have sent the event ok - Fallback plugin 3 should not have sent the event ok - Fallback plugin 4 should not have sent the event ok - Should not have been any disabled plugins ok - Should not have been any blocked plugins ok - Plugin 1 did not exist ok - Plugin 1 had an incorrect border style ok - Plugin 2 did not exist ok - Plugin 2 had an incorrect border style ok - Plugin 3 did not exist ok - Plugin 3 had an incorrect border style ok - Plugin 4 did not exist ok - Plugin 4 had an incorrect border style ok - Plugin 5 did not exist not ok - Plugin 5 had an incorrect border style got "solid", expected "none" ok - Plugin 6 did not exist not ok - Plugin 6 had an incorrect border style got "solid", expected "none"

(In reply to comment #37) > (From update of attachment 334743 [details] [diff] [review]) > 33 tests fails on 64 linux: There seem to be two distinct failures going on here. The first is that the plugin host is resetting the blocklist status on the plugin, this can happen if there is something in the plugins directory that looks like a plugin but doesn't load correctly (bad .so basically). The second is more odd, that plugin5 and plugin6 testcases don't trigger the plugin not found code. I don't have immediate access to a 64 bit machine to test this but can probably get a VM set up in the next few days.

(In reply to comment #38) > this can happen if > there is something in the plugins directory that looks like a plugin but > doesn't load correctly (bad .so basically). I do have 2 .so files which are for 32bit. If I remove the whole plugins dir, tests are ok.

(In reply to comment #39) > (In reply to comment #38) > > this can happen if > > there is something in the plugins directory that looks like a plugin but > > doesn't load correctly (bad .so basically). > I do have 2 .so files which are for 32bit. If I remove the whole plugins dir, > tests are ok. Ok I think that that is not really a fault with the code then. The test could possibly be better but I think the only way to avoid what is happening is to replace the blocklist service, but that is I think not a good thing to do during mochitests.

Comment on attachment 334258 [details] frontend display ui-r+ per IRC

This is the frontend patch. It adds icons for the disabled and blocked placeholders but they are just copied from the regular icon for now. Only question is as it is now the disabled placeholder opens the EM when clicked and is done in a way that all toolkit apps will get. We might instead want to make that Firefox specific? However since the text for the placeholder is toolkit anyway it seems to make more sense this way.

Comment on attachment 338916 [details] [diff] [review] frontend patch rev 1 Let's just ignore this one shall we

This one actually works, moves off the add-ons manager opening to browser code.

Comment on attachment 339264 [details] [diff] [review] frontend patch rev 2 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > missingPluginInstaller.prototype.newMissingPlugin = function(aEvent){ >+ var notificationBox = gBrowser.getNotificationBox(browser); >+ >+ // If there is already a missing plugin notification then do nothing >+ if (notificationBox.getNotificationWithValue("missing-plugins")) >+ return; >+ > if (!browser.missingPlugins) > browser.missingPlugins = {}; > > var pluginInfo = getPluginInfo(aEvent.target); > > browser.missingPlugins[pluginInfo.mimetype] = pluginInfo; Shouldn't you leave this like it was, with the check for the existing notification after the setting of browser.missingPlugins? That way the pluginfinder correctly offers to install all of the potential missing plugins if the user triggers it. >+ var bundle_browser = document.getElementById("bundle_browser"); I know you didn't change this, but while you're at it, have it use gNavigatorBundle instead? >+missingPluginInstaller.prototype.newBlockedPlugin = function(aEvent){ >+ try { >+ if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin")) >+ return; >+ } catch (ex) {} // if the pref is missing, treat it as false, which shows the infobar This pref is misnamed I guess, since it applies to both... oh well, not a major (or new) problem I guess. The two comments above also apply to this new method. Does splitting these into two separate methods really simplify things? Looks like it increases code duplication quite a bit. >+missingPluginInstaller.prototype.newDisabledPlugin = function(aEvent){ >+ // Since we are expecting also untrusted events, make sure >+ // that the target is a plugin >+ if (!(aEvent.target instanceof Components.interfaces.nsIObjectLoadingContent)) >+ return; >+ >+ aEvent.target.addEventListener("click", >+ gMissingPluginInstaller.managePlugins, >+ false); I'm confused... this is called from the PluginDisabled event handler, which is fired by the XBL when the user clicks on a disabled plugin, no? Shouldn't the code currently in managePlugins be here instead?

This is a P1 blocker for the front end as it now represents the UI we show users when blocking a plugin for security reasons.

(In reply to comment #45) > (From update of attachment 339264 [details] [diff] [review]) > >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > > > missingPluginInstaller.prototype.newMissingPlugin = function(aEvent){ > > >+ var notificationBox = gBrowser.getNotificationBox(browser); > >+ > >+ // If there is already a missing plugin notification then do nothing > >+ if (notificationBox.getNotificationWithValue("missing-plugins")) > >+ return; > >+ > > if (!browser.missingPlugins) > > browser.missingPlugins = {}; > > > > var pluginInfo = getPluginInfo(aEvent.target); > > > > browser.missingPlugins[pluginInfo.mimetype] = pluginInfo; > > Shouldn't you leave this like it was, with the check for the existing > notification after the setting of browser.missingPlugins? That way the > pluginfinder correctly offers to install all of the potential missing plugins > if the user triggers it. Yes totally. > > >+ var bundle_browser = document.getElementById("bundle_browser"); > > I know you didn't change this, but while you're at it, have it use > gNavigatorBundle instead? > > >+missingPluginInstaller.prototype.newBlockedPlugin = function(aEvent){ > > >+ try { > >+ if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin")) > >+ return; > >+ } catch (ex) {} // if the pref is missing, treat it as false, which shows the infobar > > This pref is misnamed I guess, since it applies to both... oh well, not a major > (or new) problem I guess. Yeah. I could add a new pref if you really wanted but figured that an all in one pref is all that is needed even if it is a little badly named now. > The two comments above also apply to this new method. Does splitting these into > two separate methods really simplify things? Looks like it increases code > duplication quite a bit. I found it made more sense to me but I'm not too fussed so I've munged them back together for this patch. > >+missingPluginInstaller.prototype.newDisabledPlugin = function(aEvent){ > >+ // Since we are expecting also untrusted events, make sure > >+ // that the target is a plugin > >+ if (!(aEvent.target instanceof Components.interfaces.nsIObjectLoadingContent)) > >+ return; > >+ > >+ aEvent.target.addEventListener("click", > >+ gMissingPluginInstaller.managePlugins, > >+ false); > > I'm confused... this is called from the PluginDisabled event handler, which is > fired by the XBL when the user clicks on a disabled plugin, no? Shouldn't the > code currently in managePlugins be here instead? So for reasons I have never been totally clear on, the events fired from the XBL binding aren't used by Firefox. When the object/embed tag is first encountered it fires an event, it is this event we listen to. We then register the click handler to handle clicking on the plugin after the initial page load. The click handler prevents the handler on the binding running and re-sending the PluginDisabled event. This is ready for review. I also have a unit test almost ready.

This is a browser chrome test tat verifies that the appropriate notification bars are displayed and the right mimetypes detected and that the add-ons manager is opened when clicking on a disabled plugin. It is currently causing a problem with the testcase from bug 427559 which ends up with the wrong window focused (even though at all points during that test the correct element is focused when compared to running without my test). If I remove the part of the test that opens the add-ons manager then everything is fine.

Per gavin on IRC I've landed the strings for this in time for the string freeze: http://hg.mozilla.org/mozilla-central/rev/eda9801a2f58

(In reply to comment #48) > It is currently causing a problem with the testcase from bug 427559 which ends > up with the wrong window focused (even though at all points during that test > the correct element is focused when compared to running without my test). If I > remove the part of the test that opens the add-ons manager then everything is > fine. The new test makes the testcase from bug 427559 fail on both Windows and OSX. On Linux that testcase is already disabled because it randomly fails in what appears to be the same manner.

Removed the hunk that wasn't relevant to this.

Comment on attachment 340868 [details] [diff] [review] frontend patch rev 3 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+missingPluginInstaller.prototype.managePlugins = function(aEvent){ >+ BrowserOpenAddonsMgr("plugins"); > aEvent.preventDefault(); > } You need aEvent.stopPropagation() here... >+missingPluginInstaller.prototype.newDisabledPlugin = function(aEvent){ >+ // Since we are expecting also untrusted events, make sure >+ // that the target is a plugin >+ if (!(aEvent.target instanceof Components.interfaces.nsIObjectLoadingContent)) >+ return; >+ >+ aEvent.target.addEventListener("click", >+ gMissingPluginInstaller.managePlugins, >+ false); ...and |true| here to capture, so that the managePlugins prevents the XBL's click handler from firing and triggering another event (which in turn triggers this listener). >- var bundle_browser = document.getElementById("bundle_browser"); >- var blockedNotification = notificationBox.getNotificationWithValue("blocked-plugins"); >- const priority = notificationBox.PRIORITY_WARNING_MEDIUM; >- const iconURL = "chrome://mozapps/skin/plugins/pluginGeneric-16.png"; >- >- if (aEvent.type == "PluginBlocklisted" && !blockedNotification) { >- var messageString = bundle_browser.getString("blockedpluginsMessage.title"); >+ if (aEvent.type == "PluginBlocklisted") { >+ if (notificationBox.getNotificationWithValue("blocked-plugins")) >+ return; Seems like it would make sense to keep "blockedNotification" since it's used in both branches. Could hoist up "priority" as well, and use let for messageString/iconURL to make their scope clearer (declaring in one block and using in another is kind of ugly). The return here is better, though. >+ var priority = notificationBox.PRIORITY_WARNING_MEDIUM; >+ var iconURL = "chrome://mozapps/skin/plugins/pluginBlocked-16.png"; >+ var messageString = gNavigatorBundle.getString("blockedpluginsMessage.title"); >- if (aEvent.type == "PluginNotFound") { >+ else { Could you keep |aEvent.type == "PluginNotFound"| in a comment at least, just for clarity? Could also just leave the condition. >+ var messageString = gNavigatorBundle.getString("missingpluginsMessage.title"); See? This is redeclared! :) I haven't tested the "blocked plugin" case, I'm assuming you have. r=me with those changes.

Updated for checkin

Comment on attachment 341066 [details] [diff] [review] frontend patch rev 4 [checked in] Landed as http://hg.mozilla.org/mozilla-central/rev/0020c60ff9f5 Going to hold this bug open until I've got the final testcase submitted but otherwise this feature is complete.

Note, the code (especially the css part of it) refer to different image filenames, but actually they are still the same picture. Any plans for some artwork for disabled and blocked plugins?

(In reply to comment #55) > Note, the code (especially the css part of it) refer to different image > filenames, but actually they are still the same picture. > Any plans for some artwork for disabled and blocked plugins? Yes

(In reply to comment #54) > (From update of attachment 341066 [details] [diff] [review]) > Landed as http://hg.mozilla.org/mozilla-central/rev/0020c60ff9f5 > Check http://hg.mozilla.org/mozilla-central/diff/0020c60ff9f5/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd There's no need to have those strings twice there ;)

(In reply to comment #57) > (In reply to comment #54) > > (From update of attachment 341066 [details] [diff] [review] [details]) > > Landed as http://hg.mozilla.org/mozilla-central/rev/0020c60ff9f5 > > > > Check > http://hg.mozilla.org/mozilla-central/diff/0020c60ff9f5/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd > > There's no need to have those strings twice there ;) Oops. Not sure how that happened, apparently mercurial's merge isn't all that smart. I've removed the duplicates. Thanks for spotting that.

Ok this makes me feel slightly dirty, but it works nonetheless. The test is just renamed so it runs after the test it was causing to fail and so everything passes. Beltzner made me do it. Also the testcase relies on bug 457632 landing which is just waiting for the tree to open. otherwise there would have had to be delays introduced to wait for the notificationbox to clean up properly.

Comment on attachment 341925 [details] [diff] [review] frontent test rev 2 >diff --git a/browser/base/content/test/browser_pluginnotification.js b/browser/base/content/test/browser_pluginnotification.js >+function pageLoad() { >+ // The plugin events are async dispatched and can come after the load event >+ // This just allows the events to fire before we then go on to test the states >+ setTimeout(gNextTest, 0); Can you use executeSoon here instead? Otherwise looks good.

Testcase landed: http://hg.mozilla.org/mozilla-central/rev/0608881ec3a0

Filed follow up bug 462965 for updating the icons.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081208 Shiretoko/3.1b3pre.