Gavin's blog

I recently made a few add-on-relevant changes to Firefox’s tab context menu on the Firefox trunk, and wanted to mention them here. They were triggered by evaluating some of the extension bustage that bug 347930 caused.

Accessing the tab context menu

One of bug 347930‘s changes was moving the tabbrowser context menu from the <tabbrowser>’s anonymous content to the tab container’s anonymous content. The tab container is also no longer a child of the tabbrowser, so some common methods of accessing the context menu <menupopup> element broke. I tried to address this in bug 554165 by making two changes:

1. Adding a gBrowser.mStrip compatibility shim JS object to keep some of the common references working
2. Introducing a gBrowser.tabContextMenu property as a supported tabbrowser API. The hope is that extensions will make use of this property rather than the hacky alternatives they’ve been required to use in the past. We can keep this property working regardless of the underlying implementation details, which could potentially change again in the future.

Adding elements to the tab context menu

MozillaZine community member “onemen” pointed out that putting the tab context menu in anonymous content made it a pain to modify (requires modifying it dynamically from code). So I filed and fixed bug 554991, the net effect of which is that the “tabContextMenu” <menupopup> is now defined in browser.xul, and can be modified using normal XUL overlays. It’s also still accessible via gBrowser.tabContextMenu, of course.

Both of these changes only apply to Firefox trunk builds, which means they don’t apply to 3.6 or earlier.

browser.js is probably the most important file in the Firefox front-end, since it contains most of the JS code used by the main browser window. It’s also one of the biggest, at ~7500 lines. It’s a bit of a dumping ground, since it’s the most obvious place for main-window-scoped JS code to live, and there’s a lot of unrelated code spread throughout the file.

The idea of cleaning it up has been brought up several times in the past. We have made some changes that help – Services.jsm landed recently and helps reduce some of the XPCOM boilerplate overhead (though there’s a bunch of it left!), and Dão, Mano, and others have done some good work with piecemeal cleanup of cruft in various areas. It remains a gigantic file, though, and I’d like to try taking some bigger steps towards improving maintainability and approachability. I suspect those words could mean different things to different people, though, so I’m looking for some feedback or ideas about what exactly should be done. Here what I was thinking:

Split it up into multiple files

Splitting the code into several separate files and combining them at build time could help make things more organized. The idea would be to group pieces of code that are related to each other or that share the same purpose. We already do this for for some of the places code, for example. I’m not sure how much of a benefit we could get out of extending this technique, though, since there are a lot of small unrelated pieces, and hundreds of small scattered files could certainly end up sucking more than one humongous file. Getting the right balance might be tricky.

Move “static” methods to a common JS module

Some code in the file is general enough that it doesn’t need to be loaded for each browser window, and some pieces of code can be re-factored to make this possible. This would have the additional benefit of reducing the amount of JS that needs to be loaded/parsed for subsequent window opens, since JS modules are shared globally and only loaded once.

Just delete stuff!

The most obvious way to make browser.js more manageable is to just outright remove code! We’re always on the lookout for dead code, but I’m sure there’s a bunch of dead(ish) code still present that’s ripe for removal. There’s also not-so-dead code that we need to take a very hard look at. Going down that path can certainly be controversial, but we have bugs filed for several such ideas already, and I think it’s a path we need to explore further.

Have any other ideas?  Let me know in the comments, or even better, file a bug and mark it blocking bug 448669!