Gavin’s blog

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.

I’m going to give weekly blog status updates a shot. I suspect planet already gets inundated with them near the end of the week, so maybe I’ll try adjusting my schedule by a few days. Or maybe I’ll end up just posting them on wikimo instead. Either way I’ll try to keep them interesting!

I didn’t think I was going to end up doing this, so I didn’t take detailed notes of everything I’ve done this week. I’m going to try to get better at that.

Accomplished this week:

• tried to keep the review queue cleanup rolling from last week, but I think I netted out even (or slightly negative). Current state: 33 pending requests.
• reviewed dolske‘s plugin crashing UI patches (bug 539848, bug 538910)
• helped test the 1.9.3 alpha 1 branding changes (bug 543564)
• wrote some additional tests for bug 512784 (consolidated smart getters for common services). ready to land once it gets rs=Mossop
• spent some time reviewing patch for bug 480350 (tab matches in awesomebar)
• wasted a bit of time arguing with RSnake on his blog

Up next (roughly in priority order):

• shorlander‘s going to be filing bugs for proposed theme/UX changes, will need to triage/prioritize those with dao
• need to make progress on browser.js cleanup/simplification – I want to get a list of actionable items by mid-next-week and get patching
• I have some mobile blog post ideas and notes that I really need to turn into posts
• try not to give up too much ground on review queue clearing
• want speak to Ryan about some changes required for bug 511017 (allow default search plugins to be updated, allowing them to get locale-specific search URLs to avoid redirects through google.com)
• would like to resolve bug 479334 (improved en-US spellcheck dictionary, including better merge scripts to ease taking changes from upstream hunspell and chromium). just needs a final test run and a couple of tweaks before landing, I think.

Ideally I think my “upcoming” tasks would be as granular and clearly defined as my “completed” tasks, I guess, but they’ll probably be easier to split out into specific tasks once some of the planning/exploratory work is done.

Curtis Bartley wrote an update for Planet Firefox, but his blog isn’t hooked up yet. My blog is hooked up already, but I haven’t written any updates.

So we’re going to combine forces to bring you the latest in status updates. (I will also shamelessly use this opportunity to bump my blog’s latest post into this year – welcome to 2009, readers!).

Here’s a teaser:

“We want Firefox to override [...] the user [...] bad news”

Check out Curtis’ update over on his blog to get the full scoop.

Firefox 3 is quickly nearing release, and shaver has prompted me to exercise my blog-posting muscles and post about changes relevant to Firefox theme developers. Alex Faaborg has already done a great job explaining the goals of the new default Firefox 3 themes (with screenshots!). I wanted to dive in a bit further into the details of the platform changes relevant to theme developers looking to update or create new themes for Firefox 3.

A large part of Firefox 3 theme work on Windows was making it possible to give Firefox a platform native look on both Vista and XP. In order to allow a given Firefox build to change it’s appearance based on the version of Windows it was running on, changes were made to our chrome registration code to allow selecting different theme packages at run-time based on operating system version. This functionality is also available to theme developers, and isn’t specific to Windows – more details can be found on MDC’s chrome registration page.

Windows theme developers might also want to make use of the new ::-moz-system-metric(windows-default-theme) pseudo-class, which allows different styling based on whether the user is currently using one of the default Windows themes (Luna/Royale/Zune/Aero, not including Classic). This feature was added to allow the Firefox 3 themes to use hard-coded colors not available in the system color set, without negatively affecting third party system themes that might specify different clashing colors. This pseudo-class therefore allows a fallback to potentially less-appealing, but more compatible, system colors for non-default themes.

Firefox 3′s layout engine, Gecko 1.9, has also received many fixes that are likely to be useful for theme developers:

• David Baron‘s patch in bug 401291 made several improvements to dynamic selector matching, which means that matching of selectors that include pseudo-classes like :first-child, nly-child, and :last-child will now be updated correctly when the DOM changes. Similar fixes were made to the :empty pseudo-class and the “+” combinator, and support was added for the “~” combinator (see CSS3 for more details).
• Support was added for Animated PNG, which allows animation with 8-bit transparency. The Firefox throbber is an APNG image, for example.
• Thanks to Cairo, Gecko 1.9 supports rgba and hsla colors in CSS, which allows specifying an alpha channel in CSS colors, to allow translucency. The Firefox themes use this in several different places for better integration with platform native colors.
• Another win from Cairo is that border radii drawn using -moz-border-radius are now anti-aliased, which improves their appearance (bug 16380).
• On Mac, Gecko now supports transparent windows, thanks to work from Colin Barrett and Håkan Waara (bug 307204). This brings Mac up to par with Windows (Linux still doesn’t support partial transparency).
• On Windows, new -moz-appearance values have been introduced that map to Vista toolbar appearances (-moz-win-browsertabbar-toolbox, -moz-win-communications-toolbox, -moz-win-media-toolbox). Also new is support for system color values for “communications text” and “media text” (-moz-win-communicationstext, -moz-win-mediatext).

Lastly, and perhaps most importantly, I wanted to encourage theme developers to check out Themes changes in Firefox 3 on MDC. It’s not a complete guide quite yet, but it already contains some great tips to help you update your theme for Firefox 3, and it’s a great place for theme developers to share information about common pitfalls.