Last Comment Bug 558517 - Cleanup browser.js plugin code
: Cleanup browser.js plugin code
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 3.7a5
Assigned To: Justin Dolske [:Dolske]
Depends on:
Blocks: 822845
  Show dependency treegraph
Reported: 2010-04-09 20:45 PDT by Justin Dolske [:Dolske]
Modified: 2012-12-18 14:42 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (WIP) (23.13 KB, patch)
2010-04-09 20:45 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.1 (23.57 KB, patch)
2010-04-11 20:12 PDT, Justin Dolske [:Dolske] review+
Details | Diff | Review
Patch v.2 (23.05 KB, patch)
2010-04-19 17:23 PDT, Justin Dolske [:Dolske] review+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2010-04-09 20:45:01 PDT
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. :)
Comment 1 Justin Dolske [:Dolske] 2010-04-11 20:12:01 PDT
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".
Comment 2 Dão Gottwald [:dao] 2010-04-14 12:09:26 PDT
(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] 2010-04-14 14:10:22 PDT
(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:] 2010-04-19 15:01:44 PDT
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] 2010-04-19 17:23:59 PDT
Created attachment 440090 [details] [diff] [review]
Patch v.2
Comment 6 Justin Dolske [:Dolske] 2010-04-20 12:27:44 PDT

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

Note You need to log in before you can comment on or make changes to this bug.