558517 - Cleanup browser.js plugin code

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch (WIP)

The plugin related code in browser.js (gMissingPluginInstaller) has been giving me a rash since I started working on it, now that Lorentz is more-or-less done I feel an overpowering need to scratch at this.

The code is overall kind of crufty, dating back to when it just handled missing plugins. Other stuff has been tacked on since then, and the code is often confusing/misleading as a result.

As a first pass, this shuffles around and renames some stuff, but should be functionally equivalent. Untested because I just felt like hacking on it, and now it's MFBT. :)

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Some notes that might help follow the patch:

* The stuff about untrusted events is moot now, all the events we listen for are fired by nsObjectLoadingContent's nsPluginErrorEvent::Run(), and they're trusted. We don't need to listen for untrusted events. [Well, the NewPluginInstalled event is fired by the plugin wizard, but that's handled as a separate case.]

* s/gMissingPluginInstaller/gPluginHandler/. I left an alias in for backwards-compt.

* s/newMissingPlugin()/pluginUnavailable()/. Also clarified by some event-type specific stuff up into handleEvent and made it mode direct about what events do what.

* Consolidated strings into notifyStrings, so it's easier to read through the last half of pluginUnavailable() and actually see what it's doing.

* The old refreshBrowser() moves up, and is basically the new newPluginInstalled() listener.

In the future I intend to increase commonality between the plugin-crashed code and the other parts... Eg, the other plugin issues should suppress their UI when it's too small, and the notification bar stuff could be more common. But I'll do this as a separate step/bug/patch, so it's not a single massive "cleanup".

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> * s/gMissingPluginInstaller/gPluginHandler/. I left an alias in for
> backwards-compt.

gMissingPluginInstaller seems to be used by only one add-on. MXR finds two, but they're essentially the same, just for different Firefox versions.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #2)

> gMissingPluginInstaller seems to be used by only one add-on. MXR finds two, but
> they're essentially the same, just for different Firefox versions.

I did the MXR search too, but apparently misremembered the frequency... No need to keep this for backwards compat.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 438407 [details] [diff] [review]
Patch v.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  pluginUnavailable: function (plugin, eventType) {

>+    let notifyStrings = {

I think having buttonLabel/buttonAccess in this object makes things harder to follow. How about something more like:

var notifications = {
  PluginBlocklisted: {
    message: "",
    id: "",
    iconURL: "",
    buttons: [],
  },
  ...
}

with all of the buttons defined in the single object, and then do the if/else if thing (or a switch()?) for the behavior differences, and then just:

>+    notificationBox.appendNotification(notifications[eventType].message,
>+                                       notifications[eventType].id,
>+                                       notifications[eventType].iconURL,
>+                                       notificationBox.PRIORITY_WARNING_MEDIUM,
>+                                       notifications[eventType].buttons);

>+  pluginInstanceCrashed: function (plugin, aEvent) {

>+    let isObjectTooSmall = this.isTooSmall(plugin, overlay);
>     if (isObjectTooSmall) {

nit: omit the temp variable?

r=me with those considered (but I'll take a look at the changes if you decide to go with my suggestion).

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 6

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0afded9fa753

Note that this does not need to land on Lorentz (3.6.x), it's just a cleanup and isn't fixing any bugs.