Closed Bug 558517 Opened 10 years ago Closed 10 years ago

Cleanup browser.js plugin code

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a5

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (WIP) (obsolete) — Splinter Review
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. :)
Attached patch Patch v.1 (obsolete) — Splinter Review
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".
Attachment #438232 - Attachment is obsolete: true
Attachment #438407 - Flags: review?(gavin.sharp)
(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.
(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.
Attachment #438407 - Flags: review?(gavin.sharp) → review+
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).
Attached patch Patch v.2Splinter Review
Attachment #438407 - Attachment is obsolete: true
Attachment #440090 - Flags: review?(gavin.sharp)
Attachment #440090 - Flags: review?(gavin.sharp) → review+
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
Blocks: 822845
You need to log in before you can comment on or make changes to this bug.