Shaver posted about this in dev.apps.firefox last week, but I thought it might bear repeating to the Planet audience: as of last week, the Firefox module has a new module owner (yours truly), and a new review policy.

The new review policy is explained in detail on the Wiki. As part of this change, there is now a larger set of people deemed “Firefox reviewers“, and a new policy for nominating reviewers. Together these changes will hopefully address the review bandwidth issues we’ve been having.

Because the Firefox module is relatively large and covers many different code areas, Firefox reviewers are expected to know their areas of expertise, and not be shy about redirecting requests to a more suitable reviewer, if applicable. The wiki page also goes on in detail about what r+ means in the Firefox module – a handy list to review for new and old reviewers alike.

Please, send me email if anything on that page isn’t clear, or if you think anything (or anyone!) is missing!

I don’t recall precisely how I first got involved with Mozilla, but a large part of my early contributions was triaging bugs. I stumbled across Bugzilla at some point, and I really enjoyed finding duplicates and trying to figure out the root causes of bugs. I would watch the “bugs filed today” query, and try to triage as many bugs as I could. It was a fun challenge to find duplicates quickly, and memorize common issues (this, and later IRC- and bonsai-watching habits and general Mozilla addiction are what led to the coining of the name “gavinbot“). Later, as I took on more responsibility in other areas (e.g. development and code review), I stopped being as actively involved in Bugzilla triage itself; the bugmail hasn’t stopped, though 🙂

Recently Tyler posted about his frustrations with the bug triage effort. He raises some good points, but I think one of his premises is faulty:  I don’t think that focusing on the number of UNCO bugs is useful. We should focus on outcomes, not on driving a number to 0 for the sake of it.

Are we missing bugs that we should have noticed, but failed to? Maybe, but we’re never going to be 100% on top of every bug, and I don’t think the situation here is as dire as “5000 untriaged bugs!” makes it sound – a very large percentage of those are duplicates, or invalid, or minor issues that we shouldn’t be focusing on. Spending contributor time sorting through many of those bugs probably doesn’t have a very good cost/benefit ratio. Important issues still tend to bubble up pretty effectively, in my experience.

Are people being discouraged from reporting problems to us because they get ignored? This is a problem, certainly (though again, I’m not sure about the degree). We should fix this by having better tools for giving feedback (i.e. not Bugzilla), and there are already successful efforts underway to do that (e.g. input.mozilla.com).

It’s important to step back and look at the bigger picture – driving a Bugzilla query of untriaged bugs to 0 should only be a goal insofar as it helps us achieve the primary goal of producing good software. I contend that it probably isn’t the most effective way to contribute to that goal at the moment.

Getting Firefox to support multiple processes is not a small task. To satisfy the goals for going to a multi-process model, we’re going to be making changes to how the front-end Firefox application code interacts with Web page content, since all of those interactions will need to occur via cross-process “messages”.

Felipe has done some great work getting basic e10s browser functionality landed on mozilla-central. He’s added an –enable-e10s-compat build-time switch that both sets the pref that enables putting tabs into separate processes, and then also disables a bunch of code that doesn’t work in that situation. The disabled code includes some pretty major things like “updating the security/location bar UI in response to page navigation”. He’s working on getting those pieces working in bug 666801, and the rest of the currently known work is being tracked in bug fxe10s.

As we continue to identify pieces of code that need Electrolysis-related changes (using both dynamic analysis and static analysis – progress is being made on both of those fronts!), we’ll be adding them to that tracking bug, and poking people to get involved in fixing them. Now that –enable-e10s-compat has landed on the trunk, anyone should feel free to pick some of those bugs up, and land these fixes on mozilla-central (via fx-team, if desired!). Tim Taubert has a great post about the basics of rewriting code to support e10s, and Firefox reviewers should all be getting into the habit of reviewing e10s fixes (and ensuring that newly added code is e10s friendly!).

On Monday I finally landed the new “PopupNotifications” JS module, used to display popup-style notifications (as discussed earlier). Since then I’ve spent some time fixing followups, but there’s still a bunch of work to be done (including fleshing out the MDC documentation and trying to find help making menu-buttons nicer). Dave‘s already got a patch up to use the new notifications for the Addons Manager, and Robert is working on getting them working in SeaMonkey.

I completed some reviews, including some for relatively exciting patches from Dão:

• Turning on and fixing the position of the “Firefox” menu button on Windows Vista/7
• Making tabs-on-top the default on Windows (see Alex’s post for more details)

I also spent a bit of time reviewing fixes for dolske‘s prompting code rewrite that landed last week as well as cleaning up the new HUD panel‘s styling.

Next week, I plan on trying to wrap up as much of the notification-related stuff as possible, and move on to getting some patches up for the Account Manager work. Also need to do a bit of prep for the Summit which is coming up quickly!

I’ve made some progress since my last post:

Still rather ugly, but at least it’s functional!

Some things I’ll be looking into next:

• the dismissal behavior – currently dismissing the notification (i.e. clicking elsewhere on the screen) denies the geolocation request. I think we want to instead keep the request pending and allow you to access it later from the site-identity menu, but that requires some extra work (tracking shown/unshown notifications, etc.)
• make notifications triggered from background tabs/windows behave correctly (right now they’re just ignored)
• multiple-notification stacking
• support for different priorities (not sure this will be needed)
• styling! – Stephen is working on mockups for menu-buttons, which is probably the most obvious issue

The current working API is very similar to the existing notificationbox API. It’s definitely not set in stone. Right now it looks like this:

var mainAction = {
  label: "Share Location",
  accessKey: "S",
  callback: function() {
    request.allow();
  },
};

var secondaryActions = [
  {
    label: "Always Share",
    accessKey: "A",
    callback: function () {
      Services.perms.add(requestingURI, "geo", ALLOW_ACTION);
      request.allow();
    }
  },
  {
    label: "Never Share",
    accessKey: "N",
    callback: function () {
      Services.perms.add(requestingURI, "geo", DENY_ACTION);
      request.cancel();
    }
  }
];

var dismissalAction = {
  callback: function() {
    request.cancel();
  }
};

Notification.show("geolocation", "title", message,
                  "chrome://browser/skin/Geo.png",
                  mainAction, secondaryActions,
                  dismissalAction, browser);

Just recently I started working on implementing a new notification system for Firefox. It’s been discussed and blogged about before, and both Neil and MattN have worked on it before, so the task mostly involves just updating, cleaning up and completing their work. It’s being tracked by bug 398776, and I’m still in the early stages. Here’s what I threw together just so that I could test the API:

That still needs some CSS tweaking, obviously (in general, and for the button specifically: bug 509642), and Neil’s work in bug 554937 should make showing the actual popup even easier.

I also did a few other things of note this week:

• worked on some search service changes to enable changes to the Google search plugin that should make the search experience better
• did a bit of experimenting with moving some browser.js code to a JS module – nothing real yet
• worked on a security bug
• did some code reviews

The topic of code style guidelines has come up again just recently in the newsgroups. This isn’t the first time, and probably won’t be the last. Everyone has an opinion – some strongly held – about where control statement braces should go (K&R baby!), how you should wrap multi-line conditions (logical operators at EOL!), and how many spaces (or tabs! ew!) should be used for indentation (2!).

Usually the justification that fuels long debates about determining the One True Mozilla Code Style is that well-defined, strictly enforced, project-wide guidelines will help new contributors and make the code easier to maintain. Some people seem to also be suggesting that the guidelines be applied equally to all of Mozilla’s C++ and JavaScript code, since the two languages share much of their syntax. I don’t buy it.

I think that the benefits to a common style are often oversold. Consistency tends to be highly valued by us software developer types, sometimes to an unhealthy degree. I may find a particular bracing or indentation style unappealing when I approach new code, but I think it very rarely significantly impacts my ability to understand or contribute to it, assuming it’s at least internally consistent. I’m sympathetic to the idea that a common style can be beneficial at the file- (or maybe even module-) level, even if only for aesthetic reasons, but I think that trying to create and enforce more ambitious policies usually ends up being more trouble than it’s worth, especially for a project the size of Mozilla. The benefits to cross-module style guidelines just seem mostly theoretical to me, even more so for policies that attempt to span Mozilla’s loosely defined front-end/platform and JS/C++ boundaries.

Finding a single policy that covers all aspects of code style while satisfying even just a plurality of developers in a project this large can be costly just in terms of time spent debating. Add to that the time spent attempting to normalize the code base with large style-only cleanup patches, which would be necessary given our current state of affairs, and I think the scales are already tipped in favor of more tightly scoped code-style policies.

In practice, the project has settled into an fairly stable equilibrium already – we have a style guide, but the economics of making all of our code follow it, which would require both carefully auditing new code and fixing up old code, just haven’t worked out. I think we should stop kidding ourselves into thinking that they someday will, or that they should.

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!

I just landed the patch for bug 512784 on mozilla-central. It adds a new module to the toolkit whose sole purpose is to expose memoized getters for common XPCOM services on a simple “Services” JS object. The first pass involved adding getters for the prefs service, observer service, window mediator, permission manager, IO Service, prompt service, and search service. This patch also updates Firefox’s browser.js to make use of the module, which means that trunk-based extensions that run code in Firefox chrome windows can start making use of it if they’d like.

Here’s an example of one of the changes:

+// Services = object with smart getters for common XPCOM services
+Components.utils.import("resource://gre/modules/Services.jsm");

 function getTopWin()
 {
-  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1']
-                                .getService(Components.interfaces.nsIWindowMediator);
-  return windowManager.getMostRecentWindow("navigator:browser");
+  return Services.wm.getMostRecentWindow("navigator:browser");
 }

I expect to add some other services to it as I look at expanding use of the module (Mossop has some suggestions in the bug). We might also add an equivalent in Firefox for browser-specific services, if that proves to be useful. The end goal is to remove a lot of the XPCOM boilerplate junk we see spread around our front-end JS code, and a side benefit is the reduction of unnecessary getService() calls for services that are already guaranteed to be otherwise instantiated and kept around for the app’s lifetime. This necessarily implies that JS scopes where the module was imported will now have permanent references to services after their first use, which is worth keeping in mind, both when making use of of the module and when adding additional getters to it.