This was a busy week for MozReview! There are a number of changes people need to be aware of.

Support for Specifying Reviewers via Commit Messages

MozReview will now parse r?gps syntax out of commit messages to set reviewers for pushed commits.

Read the docs for more information, including why we are preferring r? to r=.

Since it landed, a number of workflow concerns have been reported. See the bug tree for bug 1142251 before filing a bug to help avoid duplicates.

Thank Dan Minor for the feature!

Review Attachment/Flag Per Commit

Since the beginning of MozReview, there was one Bugzilla attachment / review flag per commit series. This has changed to one attachment / review flag per commit.

Before, you needed to grant Ship It on the parent/root review request in order to r+ the MozReview review request. Now, you grant Ship It on individual commits and these turn into individual r+ on Bugzilla. To reinforce that reviewing the parent/root review request doesn't do anything meaningful any more, the Ship It button and checkbox have been removed from the parent/root review request.

The new model more closely maps to how code review in Bugzilla has worked at Mozilla for ages. And, it is a superior workflow for future workflows we're trying to enable.

We tried to run a one-time migration script to convert existing parent/root attachments/review flags to per-commit attachments/flags. However, there were issues. We will attempt again sometime next week. In the interim, in-flight review requests may enter an inconsistent state if they are updated. If a new push is performed, the old parent/root attachment/review flag may linger and per-commit attachments/flags will be created. This could be confusing. The workaround is to manually clear the r? flag from the parent/root attachment or wait for the migration script to run in a few days.

Mark Côté put in a lot of hard work to make this change happen.

r? Flags Cleared After Review

Before, submitting a review without granting Ship It wouldn't do anything to the r? flag: the r? flag would linger.

Now, submitting review without granting Ship It will clear the r? flag. We think the new default is better for the majority of users. However, we recognize it isn't always wanted. There is a bug open to provide a checkbox to keep the r? flag present.

Metadata Added to Changesets

If you update to the tip of the version-control-tools repository (you should do this every week or so to stay fresh - use mach mercurial-setup to do this automatically), metadata will automatically be added to commits when working with MozReview-enabled repositories.

Specifically, we insert a hidden, unique, random ID into every changeset. This ID can be used to map commits to each other. We don't use this ID yet. But we have plans.

A side-effect of this change is that you can no longer push to MozReview if you have uncommitted local changes. If this is annoying, use hg shelve and hg unshelve to create and undo temporary commits. If this is too annoying, complain by filing a bug and we can look into doing this automatically as part of pushing.

What's Next?

We're actively working on more workflow enhancements to make MozReview an even more compelling experience.

I'm building a web service to query file metadata from moz.build files. This will allow MozReview to automatically file bugs in proper components based on what files changed. Once code reviewer metadata is added to moz.build files, it will enable us to assign reviewers automatically as well. The end goal here is to lower the number of steps needed to turn changed code into a landing. This will be useful when we turn GitHub pull requests into MozReview review requests (random GitHub contributors shouldn't need to know who to flag for review, nor should they be required to file a bug if they write some code). Oh year, we're working on integrating GitHub pull requests.

Another area of focus is better commit tracking and partially landed series. I have preliminary patches for automatically adding review URL annotations to commit messages. This will enable people to easily go from commit (message) to MozReview, without having to jump through Bugzilla. This also enables better commit tracking. If you e.g. perform complicated history rewriting, the review URL annotation will enable the MozReview server to better map previously-submitted commits to existing review requests. Partially landed series will enable commits to land as soon as they are reviewed, without having to wait on the entire series. It's my strong belief that if a commit is granted review, it should land as soon as possible. This helps prevent bit rot of ready-to-land commits. Landings also make people feel better because you feel like you've accomplished something. Positive feedback loops are good.

Major work is also being done to overhaul the web UI. The commit series view (which is currently populated via XHR) will soon be generated on the server and served as part of the page. This should make pages load a bit faster. And, things should be prettier as well.

And, of course, work is being invested into Autoland. Support for submitting pushes to Try landed a few weeks ago. Having Autoland actually land reviewed commits is on the radar.

Exciting times are ahead. Please continue to provide feedback. If you see something, say something.

It is common for commit messages in Firefox to contains strings like Part 1, Part 2, etc. See this push for bug 784841 for an extreme multi-part example.

When code review is conducted in Bugzilla, these identifiers are necessary because Bugzilla orders attachments/patches in the order they were updated or their patch title (I'm not actually sure!). If part numbers were omitted, it could be very confusing trying to figure out which order patches should be applied in.

However, when code review is conducted in MozReview, there is no need for explicit part numbers to convey ordering because the ordering of commits is implicitly defined by the repository history that you pushed to MozReview!

I argue that if you are using MozReview, you should stop writing Part N in your commit messages, as it provides little to no benefit.

I, for one, welcome this new world order: I've previously wasted a lot of time rewriting commit messages to reflect new part ordering after doing history rewriting. With MozReview, that overhead is gone and I barely pay a penalty for rewriting history, something that often produces a more reviewable series of commits and makes reviewing and landing a complex patch series significantly easier.

A bunch of us were in Toronto last week hacking on MozReview.

One of the cool things we did was deploy a bot for performing Python static analysis. If you submit some .py files to MozReview, the bot should leave a review. If it finds violations (it uses flake8 internally), it will open an issue for each violation. It also leaves a comment that should hopefully give enough detail on how to fix the problem.

While we haven't done much in the way of performance optimizations, the bot typically submits results less than 10 seconds after the review is posted! So, a human should never be reviewing Python that the bot hasn't seen. This means you can stop thinking about style nits and start thinking about what the code does.

This bot should be considered an alpha feature. The code for the bot isn't even checked in yet. We're running the bot against production to get a feel for how it behaves. If things don't go well, we'll turn it off until the problems are fixed.

We'd like to eventually deploy C++, JavaScript, etc bots. Python won out because it was the easiest to integrate (it has sane and efficient tooling that is compatible with Mozilla's code bases - most existing JavaScript tools won't work with Gecko-flavored JavaScript, sadly).

I'd also like to eventually make it easier to locally run the same static analysis we run in MozReview. Addressing problems locally before pushing is a no-brainer since it avoids needless context switching from other people and is thus better for productivity. This will come in time.

Report issues in #mozreview or in the Developer Services :: MozReview Bugzilla component.

Bugzilla has played a major role in the Firefox development process for over 15 years. With upcoming changes to how code changes to Firefox are submitted and reviewed, I think it is time to revisit the central role of Bugzilla and bugs in the Firefox development process. I know this is a contentious thing to say. Please, gather your breath, and calmly read on as I explain why I believe this.

The current Firefox change process defaults to requiring a Bugzilla bug for everything. It is rare (and from my experience frowned upon) when a commit to Firefox doesn't reference a bug number. We've essentially made Bugzilla and a bug prerequisites for changing anything in the Firefox version control repository. For the remainder of this post, I'm going to say that we require a bug for any change, even though that statement isn't technically accurate. Also, when I say Bugzilla, I mean bugzilla.mozilla.org, not the generic project.

Before I go on, let's boil the Firefox change process down to basics.

At the heart of any change to the Firefox source repository is a diff. The diff (a representation of the differences between a set of files) is the smallest piece of data necessary to represent a change to the Firefox code. I argue that anything more than the vanilla diff is overhead and could contribute to process debt. Now, there is some essential overhead. Version control tools supplement diffs with metadata, such as the author, commit message, and date. Mozilla has also instituted a near-mandatory code review policy, where changes need to be signed off by a set of trusted individuals. I view both of these additions to the vanilla diff as essential for Firefox development and non-negotiable. Therefore, the bare minimum requirements for changing Firefox code are a diff plus metadata (a commit/patch) and (almost always) a review/sign-off. That's it. Notably absent from this list is a Bugzilla bug. I argue that a bug is not strictly required to change Firefox. Instead, we've instituted a near-universal policy that we should have bugs. We've chosen to add overhead and process debt - interaction with Bugzilla - to our Firefox change process.

Now, this choice to require all changes be associated with bugs has its merits. Bugs provide excellent anchor points for historical context and for additional information after the change has been committed and is forever set in stone in the repository (commits are immutable in Mercurial and Git and you can't easily attach metadata to the commit after the fact). Bugs are great to track relationships between different problems or units of work. Bugs can even be used to track progress towards a large feature. Bugzilla components also provide a decent mechanism to follow related activity. There's also a lot of tooling and familiar process standing on top of the Bugzilla platform. There's a lot to love here and I don't want diminish the importance of all these things.

When I look to the future, I see a world where the current, central role of Bugzilla and bugs as part of the Firefox change process begin to wane. I see a world where the benefits to maintaining our current Bugzilla-centric workflow start to erode and the cost of maintaining it becomes higher and harder to justify. You actually don't have to look too far into the future: that world is already here and I've already started to feel the pains of it.

A few days ago, I blogged about GitHub and its code first approach to change. That post was spun off from an early draft of this post (as were the posts about Firefox contribution debt and utilizing GitHub for Firefox development). I wanted to introduce the concept of code first because it is central to my justification for changing how we do things. In summary, code first capitalizes on the fact that any change to software involves code and therefore puts code front and center in the change process. (In hindsight, I probably should have used the term code centric, because that's how I want people to think about things.) So how does code first relate to Bugzilla and Firefox development?

Historically, code review has occurred in Bugzilla: upload a patch to Bugzilla, ask for review, and someone will look at it. And, since practically every change to Firefox requires review, you need a bug in Bugzilla to contain that review. Thus, one way to view a bug is as a vehicle for code review. Not every bug is just a code review, of course. But a good number of them are.

The only constant is change. And the way Mozilla conducts code review for changes to Firefox (and other projects) is changing. We now have MozReview, a code review tool that is not Bugzilla. If we start accepting GitHub pull requests, we may perform reviews exclusively on GitHub, another tool that is not Bugzilla.

(Before I go on, I want to quickly point out that MozReview is nowhere close to its final form. Parts of MozReview are pretty bad right now. The maintainers all know this and we have plans to fix it. We'll be in Toronto all of next week working on it. If you don't think you'll ever use it because parts are bad today, I ask you to withhold judgement for a few more months.)

In case you were wondering, the question on whether Bugzilla should always be used for code review for Firefox has been answered and that answer is no. People, including maintainers of Bugzilla, realized that better-than-Splinter/Bugzilla code review tools exist and that continuing to invest time to develop Bugzilla/Splinter into a best-in-class code review tool would be better spent integrating Bugzilla with an existing tool. This is why we now have a Review Board based code review tool - MozReview - integrated with Bugzilla. If you care about code quality and more powerful workflows, you should be rejoicing at this because the implementation of code review in Bugzilla does not maximize optimal outcomes.

The world we're moving to is one where code review occurs outside of Bugzilla. This raises an important question: if Bugzilla was being used primarily as a vehicle for code review, what benefit and/or role should Bugzilla play when code review is conducted outside of Bugzilla?

I posit that there are a class of bugs that won't need to exist going forward because bugs will provide little to no value. Put another way, I believe that a growing number of commits to the Firefox repository won't reference bugs.

Come with me on a journey to the future.

MozReview is purposefully being designed in a code and repository centric way. To initiate the formal process for considering a change to code, you push to a Mercurial (or Git!) repository. This could be directly to Mozilla's review repository. If I have my way, this could even be kicked off by submitting a pull request on GitHub or Bitbucket. No Bugzilla attachment uploading here: our systems talk in terms of repositories and commits. Again, this is by design: we don't want submitting code to Mozilla to be any harder than hg push or git push so as to not introduce process debt. If you have code, you'll be able to send it to us.

In the near future, MozReview will stop cross-posting detailed review updates to Bugzilla. Instead, we'll use Review Board's e-mail feature to send its flavor of emails. These will have rich HTML content (or plain text if you insist) and will provide a better experience than Bugzilla ever will. We'll adopt the model of tools like Phabricator and GitHub and only post summaries or links of activity, not full content, to bugs. You may be familiar with the concept as applied to the web: it's called hyperlinking.

Work is being invested into Autoland. Autoland is an automated landing queue that pushes/lands commits semi-automatically once they are ready (have review, pass automation, etc). Think of Autoland as a bot that does all the labor intensive and menial actions around pushing that you do now. I believe Autoland will eventually handle near 100% of pushes to the Firefox repository. And, if I have my way, Autoland will result in the abolishment of integration branches and merge commits in the Firefox repository. Good riddance.

MozReview and Autoland will be highly integrated. MozReview will be the primary user interface for interacting with Autoland. (Some of this should be in place by the end of the quarter.)

In this world, MozReview and its underlying version control repositories essentially become a database of all submitted, pending, and discarded commits to Firefox. The metaphorical primary keys of this database are not bug numbers: they are code/commits. (Code first!) Some of the flags stored in this database tell Autoland what it should do. And the MozReview user interface (and API) provide a mechanism into controlling those flags.

Landing a change in Firefox will be initiated by a simple action such as clicking a checkbox in MozReview. (That could even be the Grant Review checkbox.) Commits cleared for landing will be picked up by Autoland and eventually automatically pushed to the Firefox repository (assuming the build and test automation is happy, of course). Once Autoland takes control, humans are just passengers. We won't be bothered with menial tasks like updating the commit message to reflect a review was performed: this will happen automatically inside MozReview or Autoland. (Although, there's a chance we may adopt some PGP-based signing to more strongly convey review for some code changes in order to facilitate stronger auditing and trust guarantees. Stay tuned.) Likewise, if a commit becomes associated with a bug, we can add that metadata to the commit before it is landed, no human involvement necessary beyond specifying the link in the MozReview web UI (or API). Autoland/MozReview will close review requests and/or bugs automatically. (Are you excited about performing less work yet?)

When commits are added to MozReview, MozReview will read metadata from the repository they came from to automatically determine an appropriate reviewer. (We plan to leverage moz.build files for this in the case of Firefox.) This should eliminate a lot of process debt around choosing a reviewer. Similar metadata will also be used to determine what Bugzilla component a change is related to, static analysis rules to use to critique the phsyical structure of the change, and even automation jobs that should be executed given the set of files that changed. The use of this metadata will erode significant process debt around the change contribution workflow.

As commits are pushed into MozReview/Autoland, the systems will be intelligent about automatically tracking dependencies and facilitating complex development workflows that people run into on a daily basis.

If I create a commit on top of someone else's commit that hasn't been checked in yet, MozReview will detect the dependency between my changes and the parent ones. This is an advantage of being code first: by interfacing with repositories rather than patch files, you have an explicit dependency graph embedded in the repository commit DAG that can be used to aid machines in their activities.

It will also be possible to partially land a series of commits. If I get review on the first 5 of 10 commits but things stall on commit 6, I can ask Autoland to land the already-reviewed commits so they don't get bit rotted and so you have partial progress (psychological studies show that a partial reward for work results in greater happiness through a sense of accomplishment).

Since initiating actions in MozReview is light weight (just hg push), itch scratching is encouraged. I don't know about you, but in the course of working on the Firefox code base, I frequently find myself wanting to make small, 15-30s changes to fix something really minor. In today's world, the overhead for these small changes is often high. I need to upload a separate patch to Bugzilla. Sometimes I even need to create a new bug to hold that patch. If that patch depends on other work I did, I need to set up bug dependencies then worry about landing everything in the right order. All of a sudden, the overhead isn't worth it and my positive intentions go unacted on. Multiplied by hundreds of developers over many years, and you can imagine the effect on software quality. With MozReview, the overhead for itch scratching like this is minor. Just make a small commit, push, and the system will sort everything out. (These small commits are where I think a bugless process really shines.)

This future world revolves around code and commits and operations on them. While MozReview has review in its name, it's more than a review tool: it's a database and interface to code and its state.

In this code first world, Bugzilla performs an ancillary role. Bugzilla is still there. Bugs are still there. MozReview review requests and commits link to bugs. But it is the code, not bugs, that are king. If you want to do anything with code, you interact with the code tools. And Bugzilla is not one of them.

Another way of looking at this is that nearly everything involving code or commits becomes excised from Bugzilla. This would relegate Bugzilla to, well, an issue/bug tracker. And - ta da - that's something it excels at since that's what it was originally designed to do! MozReview will provide an adequate platform to discuss code (a platform that Bugzilla provides today since it hosts code review). So if not Bugzilla tools are handling everything related to code, do you really need a bug any more?

This is the future we're trying to build with MozReview and Autoland. And this is why I think bugs and Bugzilla will play a less central role in the development process of Firefox in the future.

Yes, there are many consequences and concerns about making this shift. You would be rational to be skeptical and doubt that this is the right thing to do. I have another post in the works that attempts to outline some common conerns and propose solutions to many of them. Before writing a long comment pointing out every way in which this will fail to work, I encourage you to wait for that post to be published. Stay tuned.

You may have noticed the Firefox trees were closed for the better part of yesterday.

Long story short, a file containing URLs for Firefox installers was updated to reference https://ftp.mozilla.org/ from http://download-installer.cdn.mozilla.net/. The original, latter host is a CDN. The former is not. When thousands of clients started hitting ftp.mozilla.org, it overwhelmed the servers and network, causing timeouts and other badness.

The change in question was accidental. It went through code review. From a code change standpoint, procedures were followed.

It is tempting to point fingers at the people involved. However, I want us to consider placing blame elsewhere: on the code review tool.

The diff being reviewed was to change the Firefox version number from 32.0 to 32.0.3. If you were asked to review this patch, I'm guessing your eyes would have glanced over everything in the URL except the version number part. I'm pretty sure mine would have.

Let's take a look at what the reviewer saw in Bugzilla/Splinter (click to see full size):

And here is what the reviewer would have seen had the review been conducted in MozReview:

Which tool makes the change of hostname more obvious? Bugzilla/Splinter or MozReview?

MozReview's support for intraline diffs more clearly draws attention to the hostname change. I posit that had this patch been reviewed with MozReview, the chances are greater we wouldn't have had a network outage yesterday.

And it isn't just intraline diffs that make Splinter/Bugzilla a sub-optimal code review tool. I recently blogged about the numerous ways that using Bugzilla for code revie results in harder reviews and buggier code. Every day we continue using Bugzilla/Splinter instead of investing in better code review tools is a day severe bugs like this can and will slip through the cracks.

If there is any silver lining to this outage, I hope it is that we need to double down on our investment in developer tools, particularly code review.