839206 - Replace plugin installation notification bar with door hanger

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Christian Sonne [:cers]]

Instead of the very obtrusive notification bar that is currently if we encounter an unknown/uninstalled plugin, we should just show an icon in the awesomebar, which the user can click and get a door hanger with additional information and install possibilities.

Comment 1

[Christian Sonne [:cers]]

Attachment: replaces notification bar with dismissed door hanger

We need new and/or larger icons to display in the door hanger - the in-content UI uses the contentPluginDownload icon, so I stuck with that for now.

Comment 2

[Benjamin Smedberg]

Why are we doing this work? I'd much prefer to just get rid of the notification entirely.

Comment 3

[(not currently active) Ted Mielczarek]

+1

I intentionally don't have Flash installed on my Linux machine, and I would rather not be notified of that constantly.

Comment 4

[Christian Sonne [:cers]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Why are we doing this work? I'd much prefer to just get rid of the
> notification entirely.

So there is some (but much less annoying) indication of why the page the user is viewing might be broken.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> +1
> 
> I intentionally don't have Flash installed on my Linux machine, and I would
> rather not be notified of that constantly.

There are arguably about two types of people who get these notifications:
1) New users who probably do want plugins like flash/java
2) Advanced users who explicitly don't want a specific / all plugins

Not notifying group #1 at all would just lead them to think that Firefox doesn't work, and with the current patch, all group #2 will see is a small icon in the awesomebar. Is that a big enough annoyance to merit making Firefox seem broken to new users?

What if the door hanger included an option to disable the notification for that (or possibly all) plugins?

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 712533 [details] [diff] [review]
replaces notification bar with dismissed door hanger

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

::: browser/base/content/browser-plugins.js
@@ +188,5 @@
>  
>            let installLink = doc.getAnonymousElementByAttribute(plugin, "class", "installPluginLink");
>            this.addLinkClickCallback(installLink, "installSinglePlugin", plugin);
>          }
> +        this.pluginUnavailable(plugin, eventType);

I think we can simplify and untangle some code.

Most of the code in pluginUnavailable() isn't really useful now; by moving the install notification to a doorhanger we don't care if there are already some notification bars showing in content.

So this can basically just be a call to _handlePluginNotFoundEvent(). Might need to cut'n'paste the browser.missingPlugins stuff into there, or maybe we can simplify that too. Need to look again.

And let's also just name that more directly -- showInstallNotification() or something like that. [One of the flaws of the existing code is that "unknown" / "missing" / "not found" is kind of ambiguous as to purpose, because dumb 'ole PFS always offers to install things. I'd like to start making the "install" / "installable" path distinct.]

@@ +420,5 @@
>    openHelpPage: function () {
>      openHelpLink("plugin-crashed", false);
>    },
>  
> +  _handlePluginNotFoundEvent: function PH_handlePluginNotFoundEvent(aPlugin) {

This code should check for an existing |plugins-not-found| notification, and bail out if there's already one. We don't want multiple plugins in the page triggering multiple prompts.

@@ +697,5 @@
>          overlay.style.visibility = "hidden";
>      }
>    },
>  
> +  showPluginsMissing: function PH_showPluginsMissing() {

We should rename this to "startPluginInstall" or something like that. Also, since we're moving this, drop it next to _handlePluginNotFoundEvent, since that's the only place that calls it. Better yet, make it a nested function there.

@@ -764,5 @@
>                                           popup     : null,
>                                           callback  : showOutdatedPluginsInfo
>                                        }],
>                            },
> -      PluginNotFound    : {

Since this is going away as a notification bar, there more cleanup to do here in pluginUnavailable()... |missingNotification| goes away, since it'll never exist as a notification bar, as do the checks involving it.

@@ +840,5 @@
>        if (missingNotification)
>          missingNotification.close();
>      }
> +    
> +    if (eventType == "PluginNotFound") {

This block can just go away entirely now.

I'll file a quick bug to kill off the last code triggering it:

(for npapi-carbon-event-model-failure)

846       // if this is not a Universal build, just follow PluginNotFound path
847       if (!macutils.isUniversalBinary)
848         eventType = "PluginNotFound";

This case really should just revert to "no plugin available, sorry". Showing a plugin-install prompt seems unproductive.

@@ -830,5 @@
>        if (missingNotification)
>          missingNotification.close();
>      }
> -    else if (eventType == "PluginNotFound") {
> -      if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin"))

Ideally we could remove this pref from firefox.js, but I see that the PluginBlocklisted case is also using it. Undecided about just removing that pref for both cases, or fixing the "XXX add a new pref?" comment for the blocklist case.

::: browser/base/content/browser.xul
@@ +579,5 @@
>              <image id="blocked-plugins-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="mixed-content-blocked-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="webRTC-shareDevices-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="webRTC-sharingDevices-notification-icon" class="notification-anchor-icon" role="button"/>
> +            <image id="plugins-not-found-icon" class="notification-anchor-icon" role="button"/>

"plugin-install-icon"

::: browser/themes/gnomestripe/browser.css
@@ +1153,5 @@
>  
> +#plugins-not-found-icon {
> +  list-style-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
> +}
> +@media (min-resolution: 2dppx) {

Don't need the @2x styles for gnomestripe.

::: browser/themes/winstripe/browser.css
@@ +2201,5 @@
>  
> +#plugins-not-found-icon {
> +  list-style-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
> +}
> +@media (min-resolution: 2dppx) {

also don't need the @2x styles for winstripe.

::: toolkit/themes/winstripe/mozapps/jar.mn
@@ +63,5 @@
>          skin/classic/mozapps/plugins/contentPluginCrashed.png      (plugins/contentPluginCrashed.png)
>          skin/classic/mozapps/plugins/contentPluginDisabled.png     (plugins/contentPluginDisabled.png)
>          skin/classic/mozapps/plugins/contentPluginDownload.png     (plugins/contentPluginDownload.png)
> +        skin/classic/mozapps/plugins/contentPluginDownload-64.png  (plugins/contentPluginDownload-64.png)
> +        skin/classic/mozapps/plugins/[e-mail] (plugins/[e-mail])

Don't need the @2x icon for windows.

You'll also need to add an "aero" equivalent further down in this file (see how, say, contentPluginDownload.png is treated)

Comment 7

[Christian Sonne [:cers]]

Attachment: replaces notification bar with dismissed door hanger v2

Now the door hanger appears only for flash and java, and for java it is dismissed by default. For flash, you get the option to not show the door hanger automatically.

(In reply to Justin Dolske [:Dolske] from comment #6)
> Comment on attachment 712533 [details] [diff] [review]
> replaces notification bar with dismissed door hanger
> 
> Review of attachment 712533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 846       // if this is not a Universal build, just follow PluginNotFound
> path
> 847       if (!macutils.isUniversalBinary)
> 848         eventType = "PluginNotFound";
> 
> This case really should just revert to "no plugin available, sorry". Showing
> a plugin-install prompt seems unproductive.

Do we have a "no plugin available, sorry" in-content UI?

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 719296 [details] [diff] [review]
replaces notification bar with dismissed door hanger v2

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

::: browser/base/content/browser-plugins.js
@@ +195,5 @@
>  
>            let installLink = doc.getAnonymousElementByAttribute(plugin, "class", "installPluginLink");
>            this.addLinkClickCallback(installLink, "installSinglePlugin", plugin);
>          }
> +        this.showInstallNotification(plugin, eventType);

The code immediately preceding this is setting the download icon and click handler, but now we're adding code in showInstallNotification to limit the types we offer to install...

So we should probably fixup this part too. I suspect you'll want a canInstallThisMimeType() helper, do you can do:

  if (canInstallThisMimeType()) {
    /* ... current goop above to set "install" icon ... */
    this.showInstallNotification(plugin, eventType);
  } else {
    /* do we need to fiddle with installStatus / iconStatus here? */
    /* mostly-unused-today case that shows contentPluginMissing.png */
  }

@@ +462,5 @@
> +    if (["application/x-shockwave-flash",
> +         "application/futuresplash",
> +         "application/x-java-vm",
> +         "application/x-java-applet",
> +         "application/x-java-bean"].indexOf(mimetype) == -1) {

Pull these mimetypes out into a separate array. The PFS removal is going to replace & extend it, so the diffs will be a littler clearer. (IE, the array will disappear in favor of the post-PFS data source, and the loop here will just be tweaked to iterate over the new thing)

I guess we're gonna have to decide the contents of this list now. Or, well, we could keep the current braindead yes-we-can-install-everything behavior for now, and deal with that entirely in the PFS removal bug. I suppose it's better to start doing it here, to balance the sizes of each.

I'll look at this a bit closer in the next review pass, too.

@@ +473,5 @@
> +      var missingPlugins = gBrowser.selectedBrowser.missingPlugins;
> +      if (missingPlugins) {
> +        openDialog("chrome://mozapps/content/plugins/pluginInstallerWizard.xul",
> +                   "PFSWindow", "chrome,centerscreen,resizable=yes",
> +                   {plugins: missingPlugins, browser: gBrowser.selectedBrowser});

This (existing) function is pretty dumb now that I look at it. :)

The caller already has |browser| (and it's free as a closure now), so using the selected browser is a little sketchy. And because we know |browser.missingPlugins| was created above, that check can go away.

Which means this is really just a 1-line openDialog() call, which means this entire thing can just be a tiny anonymous function glued onto |callback| for the doorhanger.

@@ +479,5 @@
> +    }
> +
> +    let doc = aPlugin.ownerDocument;
> +    let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +    let contentWindow = browser.contentWindow;

You already have |browser| in scope here, and neither |contentWindow| nor |doc| are used. So all 3 lets can go away.

@@ +482,5 @@
> +    let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +    let contentWindow = browser.contentWindow;
> +
> +    let messageString = gNavigatorBundle.getString("missingpluginsMessage.title");
> +    let mainAction, secondaryActions, options;

Hmm. I think we'll want to talk with UX about what the exact strings in this prompt should be before landing, but this is fine for the moment.

@@ +517,5 @@
> +      options = { dismissed: true };
> +    }
> +    let icon = "plugin-install-icon";
> +    PopupNotifications.show(browser, "plugins-not-found",
> +                            messageString, icon,

I'd just inline the |icon| value here.

@@ +926,5 @@
>      }
> +    
> +    else {
> +      let notify = notifications[eventType];
> +      notificationBox.appendNotification(notify.message, notify.barID, notify.iconURL,

This code shouldn't move into an else block. In fact since I think every eventType is checked, this code would never run to add the notifications for the other types!

::: browser/base/content/browser.xul
@@ +578,5 @@
>              <image id="blocked-plugins-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="mixed-content-blocked-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="webRTC-shareDevices-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="webRTC-sharingDevices-notification-icon" class="notification-anchor-icon" role="button"/>
> +            <image id="plugin-install-icon" class="notification-anchor-icon" role="button"/>

I'd put this next to the plugins-notification-icon above. We might start thinking about (for a followup bug?) what to do if there are blocked and missing plugins on a page, such that you could potentially have 2 doorhangers here.

Comment 9

[Christian Sonne [:cers]]

Attachment: Replace notification bar with door hanger

Comment 10

[Christian Sonne [:cers]]

Attachment: Replace notification bar with door hanger

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 723988 [details] [diff] [review]
Replace notification bar with door hanger

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

New Binary File: toolkit/themes/osx/mozapps/plugins/contentPluginDownload-64.png
New Binary File: toolkit/themes/osx/mozapps/plugins/[e-mail]
New Binary File: toolkit/themes/windows/mozapps/plugins/contentPluginDownload-64.png

Need shorlander versions of all these, since they're scaled from the current 48x48 icon in-tree.

Also need a proper icon for the urlbar (anchor icon for the doorhanger), since using contentPluginDownload.png is both being scaled and the white-on-white isn't really suitable.

And I suspect we should use a different chrome:// URI for these (even if it's the same underlying icon), because the existing ones are intended specifically for the in-content UI, not the UI in browser chrome. Not sure if we're already breaking this elsewhere, though.


Finally, I think it would be good to have a couple tests for this, since I'm pretty sure the existing PFS stuff has zero-coverage.

I think 2 basic tests should suffice:

1) A test with an utterly unknown plugin type, to ensure we're not showing any UI.

2) A test with a known plugin type, to ensure we show the doorhanger.

The PFS removal bug should, in turn, expand upon #2 to make sure that the install button is actually functional.

Might help for the above to add "known" plugin data for application/x-mozilla-test-test-test-denmark-rocks or whatever.

::: browser/base/content/browser-plugins.js
@@ +151,5 @@
>    },
>  
> +  supportedPlugins: {
> +    "mimetypes": {
> +      "application/x-shockwave-flash": "flash",

PFS also has application/futuresplash. Let's just add that too and not think too hard about if it's still relevant or not.

@@ +157,5 @@
> +    },
> +
> +    "plugins": {
> +      "flash": {
> +        "displayName": "Adobe Flash",

"Flash" here, and similarly s/Oracle Java/Java/.

@@ +160,5 @@
> +      "flash": {
> +        "displayName": "Adobe Flash",
> +        /* Link directly to Windows installer */
> +        "installWINNT": {
> +          "installInfoURL": "http://adobe-cdn...",

The installInfoURL/downloadURL stuff isn't used in this patch, so should be removed. Maybe just make it simple true/false fields.

Future patches (like the Kill PFS one?) can add this info back, in a final & working form -- when they're ready.

@@ +245,5 @@
>  
>            let installLink = doc.getAnonymousElementByAttribute(plugin, "class", "installPluginLink");
>            this.addLinkClickCallback(installLink, "installSinglePlugin", plugin);
>          }
> +        this.showInstallNotification(plugin, eventType);

This should shift up so we can show the proper ui (missing/installable).

@@ +491,5 @@
>    openHelpPage: function () {
>      openHelpLink("plugin-crashed", false);
>    },
>  
> +  showInstallNotification: function PH_showInstallNotification(aPlugin) {

Please no PH_foo stuff. It's no longer needed.

@@ +538,5 @@
> +          let installTab = gBrowser.addTab(this.supportedPlugins.plugins[pluginIdentifier].about);
> +          gBrowser.selectedTab = installTab;
> +        }).bind(this)
> +      };
> +    }

Hmm. I don't think we should have this |else| case at all. Either we're helping to install something (because it's a major plugin and we're trying to make your life easy), or we're not (and it should end up falling back to the general unknown-plugin-sorry case).

@@ +540,5 @@
> +        }).bind(this)
> +      };
> +    }
> +
> +    if (pluginIdentifier == "flash") {

I suppose there's no point in showing this menuitem once it's been invoked. So,

  if (pluginIdentifier == "flash" && pref == true) { ... }

::: browser/base/content/browser.xul
@@ +575,5 @@
>              <image id="password-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="webapps-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="blocked-plugins-notification-icon" class="notification-anchor-icon" role="button"/>
> +            <image id="plugin-install-icon" class="notification-anchor-icon" role="button"/>

The ID here should match the pattern used by other images (-notification-icon).

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +96,5 @@
>  popupWarningDontShowFromLocationbar=Don't show info bar when pop-ups are blocked
>  popupShowPopupPrefix=Show '%S'
>  
>  # missing plugin installer
>  missingpluginsMessage.title=Additional plugins are required to display all the media on this page.

Let's update this string while we're here, since it's not really great for the doorhanger context. I'm not feeling particularly creative at the moment, so as a first shot:

"Would you like to install the additional software needed to display the media on this page?"

@@ +101,5 @@
> +missingpluginsMessage.button.label=Learn more about %S
> +missingpluginsMessage.button.accesskey=L
> +missingpluginsMessage.installButton.label=Install %S
> +missingpluginsMessage.installButton.accesskey=I
> +missingpluginsMessage.ignoreButton.label=I don't want Flash, never notify me again

I think "Don't ask again" will be sufficient. And shorter. :)

Let's also call these all installPlugin.*

::: browser/themes/windows/browser.css
@@ +2204,5 @@
> +}
> +
> +.popup-notification-icon[popupid="plugins-not-found"] {
> +  list-style-image: url(chrome://mozapps/skin/plugins/contentPluginDownload-64.png);
> +}

You'll need to make these changes for the linux theme too. (But not jar.mn, since that's in the toolkit theme and Linux inherits from Windows there.)

Also, these should be place where similar rules already exist.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Because I was both slow with this review and playing around with it, here's an updated patch.

I think this fixes most of my previous review comments. Still needs:
* Updated icons artwork & chrome:// URIs
* Tests

Comment 13

[Justin Dolske [:Dolske]]

Oh. And, uhm, the application/lolcat stuff can be removed. I was testing with some old stuff in http://dolske.net/mozilla/tests/plugin/

Comment 14

[Christian Sonne [:cers]]

Attachment: Patch v.6

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 729355 [details] [diff] [review]
Patch v.6

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

* Still needs updated icons.
* Update the supportedPlugins data with the info from bug 848160...
  - Add "Quicktime", steal the ugly mimetype regex PFS is using, all platforms
  - Add "Shockwave", application/x-director, for Windows/Mac only

Other than that looks good still.

Comment 16

[Christian Sonne [:cers]]

Attachment: Patch v. 7

The contentPluginInstall icons are for now a copy of contentPluginDownload, and will be replaced in bug 856828

Comment 17

[Justin Dolske [:Dolske]]

Still need an updated patch with the icons from bug 856828...

Comment 18

[Christian Sonne [:cers]]

Attachment: Patch v. 8

Added icons from bug 856828

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 743714 [details] [diff] [review]
Patch v. 8

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

::: browser/base/content/browser-plugins.js
@@ +500,5 @@
>    openHelpPage: function () {
>      openHelpLink("plugin-crashed", false);
>    },
>  
> +  showInstallNotification: function PH_showInstallNotification(aPlugin) {

PH_foo naming isn't needed any more.

::: browser/themes/osx/browser.css
@@ +3104,5 @@
>    }
>  }
>  
> +#plugin-install-notification-icon {
> +  list-style-image: url(chrome://mozapps/skin/plugins/contentPluginInstall.png);

These icons (the anchor icon and icon in the panel) should live in browser.

@@ +3108,5 @@
> +  list-style-image: url(chrome://mozapps/skin/plugins/contentPluginInstall.png);
> +}
> +@media (min-resolution: 2dppx) {
> +  #plugin-install-notification-icon {
> +    list-style-image: url(chrome://mozapps/skin/plugins/contentPluginInstall.png);

Forgot the @2x here.

::: toolkit/themes/osx/mozapps/jar.mn
@@ +62,5 @@
>    skin/classic/mozapps/plugins/contentPluginDownload.png          (plugins/contentPluginDownload.png)
> +  skin/classic/mozapps/plugins/contentPluginInstall.png           (plugins/contentPluginInstall.png)
> +  skin/classic/mozapps/plugins/[e-mail]        (plugins/[e-mail])
> +  skin/classic/mozapps/plugins/contentPluginInstall-64.png        (plugins/contentPluginInstall-64.png)
> +  skin/classic/mozapps/plugins/[e-mail]     (plugins/[e-mail])

The non @2x ones should have been added to the jar.mn for windows too.

::: toolkit/themes/windows/mozapps/plugins/pluginProblem.css
@@ +45,5 @@
>  :-moz-type-unsupported .icon,
>  :-moz-type-unsupported-platform .icon {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);
>  }
> +:-moz-type-unsupported .icon[installable] {

This change is needed in the osx version of this file too.

Comment 20

[Justin Dolske [:Dolske]]

Attachment: Patch v.9

Last version + fixes.

Pushed to try to verify test and make sure nothing is unexpected:

https://tbpl.mozilla.org/?tree=Try&rev=58528d7210aa

Comment 21

[Justin Dolske [:Dolske]]

Try run is looking green. r+, please land. :)

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b2233c10069

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 24

[u279076]

Adding this to the list of sign-offs for Firefox 23.0b1.

Comment 25

[Paul Silaghi, QA [:pauly]]

Hello everyone,
I tested this on FF 23.0a2 (2013-06-20).
Couple of issues:
1. If Flash is missing, the doorhanger has "Don't ask again" and "Not now" options.
If Java, Shockwave, Quicktime are missing, the doorhanger has only the "Not now" option.
2. If Flash is missing, the doorhanger and a pop-up are displayed.
If Java, Shockwave, Quicktime are missing, only the doorhanger is displayed.
3. Drag and drop the tab to open a new window => the doorhanger disappears (similar issue in bug 841350)
4. What about Silverlight, Adobe Reader ?
5. Open a page with multiple missing plugins: http://mozqa.com/data/firefox/plugins/plugin-test.html
The doorhanger says only Java is missing.
6.(In reply to Justin Dolske [:Dolske] from comment #8)
> We might start
> thinking about (for a followup bug?) what to do if there are blocked and
> missing plugins on a page, such that you could potentially have 2
> doorhangers here.
It seems to work fine, both the CTP and the missing plugins icons are displayed.
7. Do you have testcase for an unknown plugin to test with ?

Comment 26

[u279076]

Philor or Dolske, can either of you answer Paul's questions in comment 25?

Comment 27

[Justin Dolske [:Dolske]]

That all sounds expected. Did you read this bug and dependencies?

Comment 28

[u279076]

Thanks Justin, marking this verified fixed.