Thoughts on the module system









6 thoughts on “Thoughts on the module system”

  1. 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.

  2. 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.

    1. 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.

  3. 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.

  4. 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”.

  5. 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.

Comments are closed.