For a long long time Mozilla has governed its code (and a few other things) as a series of modules. Each module covers an area of code in the source and has an owner and a list of peers, folks that are knowledgeable about that module. The full list of modules is public. In the early days the module system was everything. Module owners had almost complete freedom to evolve their module as they saw fit including choosing what features to implement and what bugs to fix. The folks who served as owners and peers came from diverse areas too. They weren’t just Mozilla employees, many were outside contributors.
Over time things have changed somewhat. Most of the decisions about what features to implement and many of the decisions about the priority of bugs to be fixed are now decided by the product management and user experience teams within Mozilla. Almost all of the module owners and most of the peers are now Mozilla employees. It might be time to think about whether the module system still works for Mozilla and if we should make any changes.
In my view the current module system provides two things that it’s worth talking about. A list of folks that are suitable reviewers for code and a path of escalation for when disagreements arise over how something should be implemented. Currently both are done on a per-module basis. The module owner is the escalation path, the module peers are reviewers for the module’s code.
The escalation path is probably something that should remain as-is. We’re always going to need experts to defer decisions to, those experts often need to be domain specific as they will need to understand the architecture of the code in question. Maintaining a set of modules with owners makes sense. But what about the peers for reviewing code?
A few years ago both the Toolkit and Firefox modules were split into sub-modules with peers for each sub-module. We were saying that we trusted folks to review some code but didn’t trust them to review other code. This frequently became a problem when code changes touched more than one sub-module, a developer would have to get review from multiple peers. That made reviews go slower than we liked. So we dropped the sub-modules. Instead every peer was trusted to review any code in the Firefox or Toolkit module. The one stipulation we gave the peers was that they should turn away reviews if they didn’t feel like they knew the code well enough to review it themselves. This change was a success. Of course for complicated work certain reviewers will always be more knowledgeable about a given area of code but for simpler fixes the number of available reviewers is much larger than it was otherwise.
I believe that we should make the same change for the entire code-base. Instead of having per-module peers simply designate every existing peer as a “Mozilla code reviewer” able to review code anywhere in the tree so long as they feel that they understand the change well enough. They should escalate architectural concerns to the module’s owner.
This does bring up the question of how do patch authors find reviewers for their code if there is just one massive list of reviewers. That is where Bugzilla’s suggested reviewers feature comes it. We have per-Bugzilla lists of reviewers who are the appropriate choices for that component. Patches that cover more than one component can choose whoever they like to do the review.
This really depends on people knowing what they don’t know, which for some areas can be difficult. For instance, I don’t know how well that will work for things like reviewing WebIDL changes that are exposed to the web. I think there are subtle issues for WebIDL that may not be apparent to non-experts. Also, unshipping mistakes in WebIDL is difficult, because websites may have started to depend on the change. WebIDL changes also require finding out if there’s a spec somewhere, if that spec is at a reasonable stage of development, that the spec itself is reasonable, and that an intent-to-ship post has been made to dev.platform.
I could imagine that there are similar issues around defining new data structures using cutting-edge C++ features.
Once you have a list of “Mozilla code reviewers”, maintaining the list is going to be difficult. Right now it’s mostly OK that different modules have rather different standards for who becomes a peer — but if it’s a global list, we’re going to need a good bit of process to maintain it, bring in new people as appropriate, and make sure that we’re not bringing in people who aren’t ready.
I would suggest leaving it at the discretion of the module owner who is allowed to become peers and review code. I assume module owners watch what changes are being made in their component, and if peers abuse their trust and commit bad code, we have processes in place to back that code out and propagate the changes.
Having an OWNERS file in each component doesn’t require so much process for as long as the module owner is aware of his or hers responsibility to delegate commit rights.
At the moment commit rights in any part of the tree is not enforced, so one could argue that this system is a bit fragile. For example, I reviewed code for several years before being made a peer.
What I often miss is a way to subscribe to changes that I’m not directly responsible for reviewing. For example, as someone who cares about E10s or IPC changes to tab browsers, I might be inclined to watch the toolkit parts of tabbrowser.xul, but not necessarily be a reviewer for it. The changes there might be effectual to code in testing/marionette (the remote protocol).
On the topic of reviewers, a greater worry than whether peers are considere+d “trusted” to review any part of the mozilla-central codebase, is my dissatisfaction of having to single out a single reviewer for any given patch. In 90% of the cases, I don’t care who reviews my change, as long as it is someone trusted to know the code. In this sense, your suggestion goes a long way, but we have some catching up to do when it comes to the way code reviewers are flagged. A r? does not scale when might be a group of people.
FWIW, we moved the opposite direction in Chrome. Though the majority of contributors are Google employees and almost all product decisions are made by Googlers, on the code review front we went from a “committers can review anything” system to one with explicit directory ownership requiring signoff from appropriate peers.
We found the free-for-all system led to too much fracturing of style and best practices; explicit owners are a way to ensure that technical culture is passed to each area of the codebase. It was too easy otherwise to have areas where groups of newer employees reviewed code, and while they were competent enough to commit and do some code reviewing, they didn’t bring the history and knowledge of the rest of the codebase into their areas, so “islands” developed.
We also found explicit owners to be helpful in guiding authors to “who are the actual experts here, that you’d really want to consult”.
In my experience the problem of touching multiple modules can fairly reasonably be delt with by just being a bit flexible about what module you consider code to belong to, without running into the sort of problems Andrew is concerned about. I certainly took the position I could review accessibility related things outside of accessible/ and I don’t think that ever bothered anyone or caused problems.
On the other hand I would ask questions about how owners currently work. Maybe that’s fine for the project, but I think you can find yourself in a position where moco has certain goals and so should have particular priorities that don’t necessarily align with what a module owner might think is best for the module when not as concerned about moco’s goals.