922107 - Add pref to hide the "missing plugin" notifications

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Per bug 917713 there are relevant use-cases for hiding all missing-plugin notifications.
Adding, say, plugins.hideMissingPluginsNotification is rather easy and we supported something similar previously (plugins.hide_infobar_for_missing_plugin).

Any objections?

Comment 1

[Georg Fritzsche [:gfritzsche]]

To reproduce this:
* don't have Java, Flash or Quicktime installed
* navigate to a page that use the plugin (e.g. [1], [2])
* notice the doorhanger popping up asking you to install

To implement this, it should be enough to return false from showInstallNotification() [3] if the preference is set. See e.g. [4] for how to check a bool preference.

[1] http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
[2] http://benjamin.smedbergs.us/tests/ctptests/java-solo.html
[3] http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/browser/base/content/browser-plugins.js#l506
[4] http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/browser/base/content/browser-plugins.js#l545

Comment 2

[Willian Gustavo Veiga]

Attachment: 922107-add-pref-to-hide-the-missing-plugin-notifications.patch

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8380240 [details] [diff] [review]
922107-add-pref-to-hide-the-missing-plugin-notifications.patch

Review of attachment 8380240 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, this looks good in my local testing!

Note that the commit message doesn't need a '#' in front of the bug number, just "Bug 922107" is enough.

::: browser/base/content/browser-plugins.js
@@ +504,5 @@
>      openHelpLink("plugin-crashed", false);
>    },
>  
>    showInstallNotification: function (aPlugin) {
> +    let hideMissingPluginsNotification = Services.prefs.getBoolPref(this.PREF_HIDE_MISSING_PLUGINS_NOTIFICATION);

You could add a linebreak after the '=' and indent to stay under 80 character linelength here.
Also, i don't think the empty line before the if-statement is needed.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Add test for the pref

This should have a test as well.

https://tbpl.mozilla.org/?tree=Try&rev=0b957b3fe588

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8380240 [details] [diff] [review]
922107-add-pref-to-hide-the-missing-plugin-notifications.patch

Review of attachment 8380240 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know how I feel about adding new hidden prefs, but this is pretty simple.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8380567 [details] [diff] [review]
Add test for the pref

Review of attachment 8380567 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding a test for this.

Comment 7

[Willian Gustavo Veiga]

Attachment: 922107-add-pref-to-hide-the-missing-plugin-notifications.patch

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> To reproduce this:
> * don't have Java, Flash or Quicktime installed
> * navigate to a page that use the plugin (e.g. [1], [2])
> * notice the doorhanger popping up asking you to install

Nit: that's not quite correct. We only show the doorhanger for Flash (unless plugins.notifyMissingFlash has been disabled, by the UI available in that doorhanger). Other plugin types only get an icon added to the URL bar -- no notification is shown unless you click it.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Justin Dolske [:Dolske] from comment #8)
> Nit: that's not quite correct. We only show the doorhanger for Flash (unless
> plugins.notifyMissingFlash has been disabled, by the UI available in that
> doorhanger). Other plugin types only get an icon added to the URL bar -- no
> notification is shown unless you click it.

Ah, indeed. The patch here disables the notification icon (and stops Flash popups as well).

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8380987 [details] [diff] [review]
922107-add-pref-to-hide-the-missing-plugin-notifications.patch

Thanks, this looks good to me now!

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45dda3851224
https://hg.mozilla.org/integration/mozilla-inbound/rev/b43455151bca

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45dda3851224
https://hg.mozilla.org/mozilla-central/rev/b43455151bca