Cleanup browser.js plugin code

RESOLVED FIXED in Firefox 3.7a5

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Firefox 3.7a5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 438232 [details] [diff] [review]
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. :)
(Assignee)

Comment 1

7 years ago
Created attachment 438407 [details] [diff] [review]
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".
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.
(Assignee)

Comment 3

7 years ago
(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).
(Assignee)

Comment 5

7 years ago
Created attachment 440090 [details] [diff] [review]
Patch v.2
Attachment #438407 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #440090 - Flags: review?(gavin.sharp)
Attachment #440090 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 6

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5

Updated

5 years ago
Blocks: 822845
You need to log in before you can comment on or make changes to this bug.