January 13, 2017
In 2017, Engineering Productivity is starting on a new project that
we’re calling “Conduit”, which will improve the automation around
submitting, testing, reviewing, and landing commits. In many ways,
Conduit is an evolution and course correction of the work on MozReview
we’ve done in the last couple years.
Before I get into what Conduit is exactly, I want to first clarify
that the MozReview team has not really been working on a review tool
per se, aside from some customizations requested by users (line
support for inline diff comments). Rather, most of our work was
building a whole pipeline of automation related to getting code
landed. This is where we’ve had the most success: allowing
developers to push commits up to a review tool and to easily land them
on try or mozilla-central. Unfortunately, by naming the project
“MozReview” we put the emphasis on the review tool (Review Board)
instead of the other pieces of automation, which are the parts unique
to Firefox’s engineering processes. In fact, the review tool should
be a replaceable part of our whole system, which I’ll get to shortly.
We originally selected Review Board as our new review tool for a few
The back end is Python/Django, and our team has a lot of experience
working with both.
The diff viewer has a number of fancy features, like clearly
indicating moved code blocks and indentation changes.
A few people at Mozilla had previously contributed to the Review
Board project and thus knew its internals fairly well.
However, we’ve since realized that Review Board has some big
downsides, at least with regards to Mozilla’s engineering needs:
The UI can be confusing, particularly in how it separates the Diff
and the Reviews views. The Reviews view in particular has some
Loading large diffs is slow, but also conversely it’s unable to
depaginate, so long diffs are always split across pages. This
restricts the ability to search within diffs. Also, it’s impossible
to view diffs file by file.
Bugs in interdiffs and even occasionally in the diffs themselves.
No offline support.
In addition, the direction that the upstream project is taking is not
really in line with what our users are looking for in a review tool.
So, we’re taking a step back and evaluating our review-tool
requirements, and whether they would be best met with another tool or
by a small set of focussed improvements to Review Board. Meanwhile,
we need to decouple some pieces of MozReview so that we can accelerate
improvements to our productivity services, like Autoland, and ensure
that they will be useful no matter what review tool we go with.
Project Conduit is all about building a flexible set of services that
will let us focus on improving the overall experience of submitting
code to Firefox (and some other projects) and unburden us from the
restrictions of working within Review Board’s extension system.
In order to prove that our system can be independent of review tool,
and to give developers who aren’t happy with Review Board access to
Autoland, our first milestone will be hooking the commit repo (the
push-to-review feature) and Autoland up to BMO. Developers will be
able to push a series of one or more commits to the review repo, and
reviewers will be able to choose to review them either in BMO or
Review Board. The Autoland UI will be split off into its own service
and linked to from both BMO and Review Board.
(There’s one caveat: if there are multiple reviewers, the first one
gets to choose, in order to limit complexity. Not ideal, but the
problem quickly gets much more difficult if we fork the reviews out to
As with MozReview, the push-to-BMO feature won’t support confidential
bugs right away, but we have been working on a design to support
them. Implementating that will be a priority right after we finish
We have an aggressive plan for Q1, so stay tuned for updates.
January 13, 2017 05:56 PM
October 13, 2016
In Q3 the MozReview team started to focus on tackling various
usability issues. We started off with a targetted effort on the
“Finish Review” dialog, which was not only visually unappealing but
difficult to use. The talented David Walsh compressed the nearly
full-screen dialog into a dropdown expanded from the draft banner, and
he changed button names to clarify their purpose. We have some
ideas for further improvements as well.
David has now embarked on a larger mission: reworking the main
review-request UI to improve clarity and discoverability. He came up
with some initial designs and discussed them with a few MozReview
users, and here’s the result of that conversation:
This design provides some immediate benefits, and it sets us up for
some future improvements. Here are the thoughts behind the changes:
The commits table, which was one of the first things we added to stock
Review Board, was never in the right place. All the surrounding text
and controls reflect just the commit you are looking at right now.
Moving the table to a separate panel above the commit metadata is a
better, and hopefully more intuitive, representation of the
hierarchical relationship between commit series and individual commit.
The second obvious change is that the commit table is now collapsed to
show only the commit you are currently looking at, along with its
position (e.g. “commit 3 of 5”) and navigation links to previous and
next commits. This places the emphasis on the selected commit, while
still conveying the fact that it is part of a series of commits.
(Even if that series is actually only one commit, it is still
important to show that MozReview is designed to operate on series.)
To address feedback from people who like always seeing the entire
series, it will be possible to expand the table and set that as a
The commit title is still redundant, but removing it from the second
panel left the rest of the information there looking rather abandoned
and confusing. I’m not sure if there is a good fix for this.
The last functional change is the addition of a “Quick r+” button.
This fixes the annoying process of having to select “Finish Review”,
set the dropdown to “r+”, and then publish. It also removes the need
for the somewhat redundant and confusing “Finish Review” button, since
for anything other than an r+ a reviewer will most likely want to
leave one or more comments explaining their action. The “Quick r+”
button will probably be added after the other changes are deployed, in
part because we’re not completely satisfied with its look and position.
The other changes are cosmetic, but they make various data and
controls look much slicker while also being more compact.
We are also noodling around with a further enhancement:
This is a banner containing data about the current commit, which will
appear when the user scrolls past the commits table. It provides a
constant reminder of the current commit, and we may put in a way to
skip up to the commits table and/or navigate between commits. We may
also fold the draft/“Finish Review” banner into this as well, although
we’re still working out what that would look like. In any case, this
should help avoid unnecessary scrolling while also presenting a “you
are here” signpost.
As I mentioned, these changes are part of an on-going effort to
improve general usability. This refactoring gets us into position to
tackle more issues:
Since the commits table will be clearly separated from the commit
metadata, we can move the controls that affect the whole series
(e.g. autoland) up to the first panel, and leave controls that affect
only the current commit (right now, only “Finish Review”/“Quick r+”)
with the second panel. Again this should make things more intuitive.
Similarly, this gives us a better mechanism for moving the remaining
controls that exist only on the parent review request (“Review
Summary”/“Complete Diff”) onto the individual commit review
requests, alongside the other series controls. This in turns means
that we’ll be able to do away with the parent review request, or at
least make some radical changes to it.
MozReview usage is slowly ticking upwards, as more and more Mozillians
are seeing the value of splitting their work up into a series of
small, atomic commits; appreciating the smooth flow of pushing commits
up for review; and especially digging the autoland functionality.
We’re now hard at work to make the whole experience delightful.
October 13, 2016 01:20 AM
July 04, 2016
One of the first projects I worked on when I moved to the MozReview team was “review delegation”. The goal was to add the ability for a reviewer to redirect a review request to someone else. It turned out to be a change that was much more complicated than expected.
MozReview is a Mozilla developed extension to the open source Review Board product; with the primary focus on working with changes as a series of commits instead of as a single unified diff. This requires more than just a Review Board extension, it also encompasses a review repository (where reviews are pushed to), as well as Mercurial and Git modules that drive the review publishing process. Autoland is also part of our ecosystem, with work started on adding static analysis (eg. linters) and other automation to improve the code review workflow.
I inherited the bug with a patch that had undergone a single review. The patch worked by exposing the reviewers edit field to all users, and modifying the review request once the update was OK’d. This is mostly the correct approach, however it had two major issues:
- According to Review Board and Bugzilla, the change was always made by the review’s submitter, not the person who actually made the change
- Unlike other changes made in Review Board, the review request was updated immediately instead of using a draft which could then be published or discarded
Review Board has a simple permissions model – the review request’s submitter (aka patch author) can make any change, while the reviewers can pretty much only comment, raise issues, and mark a review as “ready to ship” / “fix it”. As you would expect, there are checks within the object layer to ensure that these permission boundaries are not overstepped. Review Board’s extension system allows for custom authorisation and permissions check, however the granularity of these are course: you can only control if a user can edit a review request as a whole, not on a per-field level.
In order to allow the reviewer list to be changed, we need to tell Review Board that the submitter was making the change.
Performing the actions as the submitter instead of the authenticated user is easy enough, however when the changes were pushed to Bugzilla they were attributed to the wrong user. After a few false starts, I settled on storing the authenticated user in the request’s meta data, performing the changes as the submitter, and updating the routines that touch Bugzilla to first check for a stored user and make changes as that user instead of the submitter. Essentially “su”.
This worked well except for on the page where the meta data changes are displayed – here the change was incorrectly attributed to the review submitter. Remember that under Review Board’s permissions model only the review submitter can make these changes, so the name and gravatar were hard-coded in the django template to use the submitter. Given the constraints a Review Board extension has to operate in, this was a problem, and developing a full audit trail for Review Board would be too time consuming and invasive. The solution I went with was simple: hide the name and gravatar using CSS.
Drafts and Publishing
How Review Board works is, when you make a change, a draft is (almost) always created which you can then publish or discard. Under the hood this involves duplicating the review request into a draft object, against which modifications are made. A review request can only have one draft, which isn’t a problem because only the submitter can change the review.
Of course for reviewer delegation to work we needed a draft for each user. Tricky.
The fix ended up needing to take a different approach depending on if the submitter was making the change or not.
When the review submitter updates the reviewers, the normal review board draft is used, with a few tweaks that show the draft banner when viewing any revision in the series instead of just the one that was changed. This allows us to correctly cope with situations where the submitter makes changes that are broader than just the reviewers, such as pushing a revised patch, or attaching a screenshot.
When anyone else updates the reviewers, a “draft” is created in local storage in their browser, and a fake draft banner is displayed. Publishing from this fake draft banner calls the API endpoint that performs the permissions shenanigans mentioned earlier.
Overall it has been an interesting journey into how Review Board works, and highlighted some of the complications MozReview hits when we need to work against Review Board’s design. We’ve been working with the Review Board team to ease some of these issues, as well as deviating where required to deliver a Mozilla-focused user experience.
Filed under: mozreview
July 04, 2016 07:20 AM
April 22, 2016
A great post on code review is making its rounds. It’s started
some discussion amongst Mozillians, and it got me thinking about how
MozReview helps with the author’s points. It’s particularly
interesting because apparently Twitter uses Review Board for code
reviews, which is a core element of the whole MozReview system.
The author notes that it’s very important for reviewers to know what
reviews are waiting on them, but also that Review Board itself doesn’t
do a good job of this. MozReview fixes this problem by piggybacking
on Bugzilla’s review flags, which have a number of features built
around them: indicators, dashboards, notification emails, and reminder
emails. People can even subscribe to the reminders for other
reviewers; this is a way managers can ensure that
their teams are responding promptly to review requests. We’ve also
toyed around with the idea of using push notifications to notify
people currently using Bugzilla that they have a new request (also
relevant to the section on being “interrupt-driven”).
On the submitter side, MozReview’s core support for microcommits—a
feature we built on top of Review Board, within our extensions—helps
“keep reviews as small as possible”. While it’s impossible to enforce
small commits within a tool, we’ve tried to make it as painless as
possible to split up work into a series of small changes.
The MozReview team has made progress on automated static analysis
(linters and the like), which helps submitters verify that their
commits follow stylistic rules and other such conventions. It will
also shorten review time, as the reviewer will not have to spend time
pointing out these issues; when the review bots have given their r+s,
the reviewer will be able to focus solely on the logic. As we
continue to grow the MozReview team, we’ll be devoting some time to
finishing up this feature.
April 22, 2016 03:39 PM
March 23, 2016
when a comment is left on a review in review board/mozreview it is currently displayed as a small square in the left column.
our top reviewers have strongly indicated that this is suboptimal and would prefer to match what most other code review systems do in displaying comments as an inline block on the diff. i agree — review comments are important and deserve more attention in the user interface; they should be impossible to miss.
while the upstream review board team have long said that the current display is in need of fixing, there is minimal love for the inline comments approach.
recently we worked on a plan of attack to appease both our reviewers and upstream’s requirements. :smacleod, :mconley and i talked through a wide assortment of design goals and potential issues. we have a design document and i’ve started mocking up the approach in html:
we also kicked off discussions with the upstream review board development team, and are hopeful that this is a feature that will be accepted upstream.
Filed under: mozreview
March 23, 2016 03:39 PM
February 08, 2016
MozReview - Mozilla's Review Board based code review tool - now
supports ingestion from Git. Previously, it only supported Mercurial.
for configuring Git with MozReview are available. Because blog posts
are not an appropriate medium for documenting systems and processes, I
will not say anything more here on how to use Git with MozReview.
Somewhat related to the introduction of Git support is an improved
mechanism for mapping commits to existing review requests.
When you submit commits to MozReview, MozReview has to decide how
those commits to review requests in Review Board. It has to choose
whether to recycle an existing review request or create a new one.
When recycling, is has to pick an appropriate one. If it chooses
incorrectly, wonky things can happen. For example, a review request
could switch to tracking a new and completely unrelated commit. That's
Up until today, our commit mapping algorithm was extremely simple. Yet
it seemed to work 90% of the time. However, a number of people
found the cracks and complained. With Git support coming online,
I had a feeling that Git users would find these cracks with higher
frequency than Mercurial users due to what I perceive to be
variations in the commit workflows of Git versus Mercurial. So,
I decided to proactively improve the commit mapping before the Git
users had time to complain.
Both the Git and Mercurial MozReview client-side extensions now insert
a MozReview-Commit-ID metadata line in commit messages. This line
effectively defines a (likely) unique ID that identifies the commit
across rewrites. When MozReview maps commits to review requests,
it uses this identifier to find matches. What this means is that
history rewriting (such as reordering commits) should be handled
well by MozReview and should not confuse the commit mapping
I'm not claiming the commit mapping mechanism is perfect. In fact,
I know of areas where it can still fall apart. But it is much
better than it was before. If you think you found a bug in the
commit mapping, don't hesitate to
file a bug.
Please have it block bug 1243483.
A side-effect of introducing this improved commit mapping is that
commit messages will have a MozReview-Commit-ID line in them. This
may startle some. Some may complain about the spam. Unfortunately,
there's no better alternative. Both Mercurial and Git do support a
hidden key-value dictionary for each commit object. In fact, the
MozReview Mercurial extension has been storing the very commit IDs
that now appear in the commit message in this dictionary for months!
Unfortunately, actually using this hidden dictionary for metadata
storage is riddled with problems. For example, some Mercurial
commands don't preserve all the metadata. And accessing or setting
this data from Git is painful. While I wish this metadata (which
provides little value to humans) were not located in the commit
message where humans could be bothered by it, it's really the only
practical place to put it. If people find it super annoying, we
could modify Autoland to strip it before landing. Although, I think
I like having it preserved because it will enable some useful
scenarios down the road, such as better workflows for uplift
requests. It's also worth noting that there is precedent for storing
unique IDs in commit messages for purposes of commit mapping in the
code review tool: Gerrit uses Change-ID lines.
I hope you enjoy the Git support and the more robust commit to review
request mapping mechanism!
February 08, 2016 11:05 AM
January 13, 2016
When I first started writing web services, I was a huge RESTful fan
boy. The architectural properties - especially the parts related to
caching and scalability - really jived with me. But as I've grown
older and gained experienced, I've realized that RESTful design,
like many aspects of software engineering, is more of a guideline
or ideal than a panacea. This post is about one of those experiences.
Review Board's Web API
is RESTful. It's actually one of the better examples of a RESTful API
I've seen. There is a very clear separation between resources. And
everything - and I mean everything - is a resource. Hyperlinks are
used for the purposes described in Roy T. Fielding's dissertation.
I can tell the people who authored this web API understood RESTful
design and they succeeded in transferring that knowledge to a web API.
Mozilla's MozReview code review tool
is built on top of Review Board. We've made a number of customizations.
The most significant is the ability to submit a series of commits
as one logical review series. This occurs as a side-effect of a
hg push to the code review repository. Once your changesets
are pushed to the remote repository, that server issues a number of
Review Board Web API HTTP requests to reviewboard.mozilla.org to
create the review requests, assign reviewers, etc. This is mostly all
built on the built-in web API endpoints offered by Review Board.
Because Review Board's Web API adheres to RESTful design principles
so well, turning a series of commits into a series of review requests
takes a lot of HTTP requests. For each commit, we have to perform
something like 5 HTTP requests to define the server state. For
series of say 10 commits (which aren't uncommon since we try to
encourage the use of microcommits), this can add up to dozens of
HTTP requests! And that's just counting the HTTP requests to
Review Board: because we've integrated Review Board with Bugzilla,
events like publishing result in additional RESTful HTTP requests from
Review Board to bugzilla.mozilla.org.
At the end of the day, submitting and publishing a series of 10
commits consumes somewhere between 75 and 100 HTTP requests! While
the servers are all in close physical proximity (read: low network
latencies), we are reusing TCP connections, and each HTTP request
completes fairly quickly, the overhead adds up. It's not uncommon
for publishing commit series to take over 30s. This is unacceptable
to developers. We want them to publish commits for review as quickly
as possible so they can get on with their next task. Humans should not
have to wait on machines.
Over in bug 1220468,
I implemented a new batch submit web API for Review Board and converted
the Mercurial server to call it instead of the classic, RESTful Review
Board web APIs. In other words, I threw away the RESTful properties
of the web API and implemented a monolith API doing exactly what we
need. The result is a drastic reduction in net HTTP requests. In our
tests, submitting a series of 20 commits for review reduced the HTTP
request count by 104! Furthermore, the new API endpoint performs
all modifications in a single database transaction. Before, each HTTP
request was independent and we had bugs where failures in the middle of
a HTTP request series left the server in inconsistent and unexpected
state. The new API is significantly faster and more atomic as a bonus.
The main reason the new implementation isn't yet nearly instantaneous
is because we're still performing several RESTful HTTP requests to
Bugzilla from Review Board. But there are plans for Bugzilla to
implement the batch APIs we need as well, so stay tuned.
(I don't want to blame the Review Board or Bugzilla maintainers for
their RESTful web APIs that are giving MozReview a bit of scaling pain.
MozReview is definitely abusing them almost certainly in ways that
weren't imagined when they were conceived. To their credit, the
maintainers of both products have recognized the limitations in their
APIs and are working to address them.)
As much as I still love the properties of RESTful design, there are
practical limitations and consequences such as what I described
above. The older and more experienced I get, the less patience I have
for tolerating architecturally pure implementations that sacrifice
important properties, such as ease of use and performance.
It's worth noting that many of the properties of RESTful design are
applicable to microservices as well. When you create a new service
in a microservices architecture, you are creating more overhead for
clients that need to speak to multiple services, making changes less
transactional and atomic, and making it difficult to consolidate
multiple related requests into a higher-level, simpler, and performant
API. I recommend
for more on this subject.
There is a place in the world for RESTful and microservice
architectures. And as someone who does a lot of server-side engineering,
I sympathize with wanting scalable, fault-tolerant architectures. But
like most complex problems, you need to be cognizant of trade-offs. It is
also important to not get too caught up with architectural purity if
it is getting in the way of delivering a simple, intuitive, and fast
product for your users. So, please, follow me down from the ivory tower.
The air was cleaner up there - but that was only because it was so
distant from the swamp at the base of the tower that surrounds every
January 13, 2016 03:25 PM
January 04, 2016
A few weeks ago, mdoglio found an article from six years ago comparing
Review Board and Splinter in the context of GNOME engineering. This was a
fascinating read because, without having read this article in advance, the
MozReview team ended implementing almost everything the author talked about.
Firstly, I admit the comparison isn’t quite fair when you replace
bugzilla.gnome.org with bugzilla.mozilla.org. GNOME doesn’t use attachment
flags, which BMO relies heavily on. I haven’t ever submitted a patch to
GNOME, but I suspect BMO’s use of review flags makes the review
process at least a bit simpler.
The first problem with Review Board that he points out is that the
“post-review command-line leaves a lot to be desired when compared to
git-bz”. This was something we did early on in MozReview, all be it
with Mercurial instead: the ability to push patches up to MozReview
hg command. Admittedly, we need an extension, mainly
because of interactions with BMO, but we’ve automated that setup with
mach mercurial-setup to reduce the friction. Pushing commits is the
area of MozReview that has seen the fewest complaints, so I think the
team did a great job there in making it intuitive and easy to use.
Then we get to what the author describes as “a more fundamental
problem”: “a review in Review Board is of a single diff”. As he
continues, “More complex enhancements are almost always done as
patchsets [emphasis his], with each patch in the set being kept as
standalone as possible. … Trying to handle this explicitly in the
Review Board user interface would require some substantial changes”.
This was also an early feature of MozReview, implemented at the same
hg push support. It’s a core philosophy baked into
MozReview, the single biggest feature that distinguishes MozReview
from pretty much every other code-review tool out there. It’s
interesting to see that people were thinking about this years
before we started down that road.
An interesting aside: he says that “a single diff … [is] not very
natural to how people work with Git”. The article was written in
2009, as GitHub was just starting to gain popularity. GitHub users
tend to push fix-up commits to address review points rather than
editing the original commits. This is at least in part due to
limitations present early on in GitHub: comments would be lost if the
commit was updated. The MozReview team, in fact, has gotten some push
back from people who like working this way, who want to make a bunch
of follow-up commits and then squash them all down to a single commit
before landing. People who strongly support splitting work into
several logical commits and updating them in place actually tend to be
Mercurial users now, especially those that use the evolve extension,
which can even track bigger changes like commit reordering and insertion.
Back to Review Board. The author moves onto how they’d have to
integrate Review Board with Bugzilla: “some sort of single-sign-on
across Bugzilla and Review Board”, “a bugzilla extension to link to
reviews”, and “a Review Board extension to update bugs in Bugzilla”.
Those are some of the first features we developed, and then later
There are other points he lists that we don’t have, like an “automated
process to keep the repository list in Review Board in sync with the
600+ GNOME repositories”. Luckily many people at Mozilla work on just
one repo: mozilla-central. But it’s true that we have to add others manually.
Another is “reduc[ing] the amount of noise for bug reporters”, which
you get if you confine all patch-specific discussion to the review
tool. We don’t have this yet; to ease the transition to Review Board,
we currently mirror pretty much everything to Bugzilla. I would
really like to see us move more and more of code-related discussion to
Review Board, however. Hopefully as more people transition to using
MozReview full time, we can get there.
Lastly, I have to laugh a bit at “it has a very slick and well developed web
interface for reviewing and commenting on patches”. Clearly we
thought so as well, but there are those that prefer the simplicity of
Splinter, even in 2015, although probably mostly from habit. Trying
to reconcile these two views is very challenging.
January 04, 2016 10:19 PM
October 22, 2015
for the next couple of quarters (at least) i’ll be shifting my attention full time from bugzilla to mozreview. this switch involves a change of language, frameworks, and of course teams. i’m looking forward to new challenges.
one of the first things i’ve done is sketch out a high level architectural diagram of mozreview and its prime dependencies:
mozreview exists as an extension to reviewboard, using bugzilla for user authentication, ldap to check commit levels, with autoland pushing commits automatically to try (and to mozilla-central soon). there’s mecurial extensions on both the client and server to make pushing things easer, and there are plans to perform static analysis with bots.
Filed under: mozilla
October 22, 2015 07:53 PM
As mentioned in my previous post on MozReview, one of the biggest
sources of confusion is the way we present the “squashed” diffs, that is,
the diff that show all of the changes in a commit series, the sum
of all the proposed changes. We also refer to these as “parent”
review requests, since they function as something to hold all the
commits together. They are stored in MozReview as separate review
requests, similar to the individual commits.
The confusion results from several things:
The links to the parent are confusing: they are currently labelled
“Complete diff” and “Review summary”. “Complete diff” doesn’t
clearly indicate that it is the complete diff of all commits
together, and “Review summary” is almost totally meaningless, since
it doesn’t include all the reviews left on the commits
themselves—only reviews left on the overview diff.
There is nothing in the UI that clearly indicates that you are
viewing the squashed diff. The only indication is that none of the rows in
the commit table are highlighted. This is particularly confusing
when there is only one commit, since the squashed diff is
identical to the commit diff.
You can leave reviews on the squashed diff, but you can’t leave a
“ship it”. This is because we are enforcing reviewers to
review individual commits. However, because there isn’t much to
distinguish parent review requests from commit review requests, it
can look like the review dialog is just broken.
There are a few simple things we can do to fix these problems: use
better link names, put a big “This is an overview of the commit
series” message, and/or put a warning “You must review individual commits” on
the review dialog. But really, we need to step back and think about the
way we present the squashed diffs, and if they even make sense as
a concept in MozReview.
To reiterate, squashed diffs provide a complete view of a whole commit
series. The concept of a commit series doesn’t exist in core Review
Board (nor does it exist in many other code-review tools), but it’s
central to the idea of the repository-centric approach (like in GitHub
pull requests). We added this concept by storing metadata resulting
from pushes to tie commit series together with a parent, and we added
UI elements like the commits table.
There are three broad ways we can deal with squashed diffs
going forward. We need to settle on one and make the associated UI
changes to make our model clear to users.
Remove squashed diffs altogether.
This is the simplest option. Squashed diffs aren’t actually
technically necessary, and they can distract reviewers from the
individual commits, which is where they should be spending most of
their time, since, in most cases, this is how the code will be
landing in the main repository. Some other repository-centric
review tools, like Critic, don’t have the concept of an overview
diff, so there are precedents. However, it might be a bit heavy
handed to tell reviewers that they can’t view all the commits as a
single diff (at least, without pulling them down locally).
Continue to allow reviews, of some sort, on squashed diffs.
This is what we have now: reviewers can leave reviews (at the
moment, comments only) on squashed diffs. If we decide we want to
continue to allow users to leave reviews on squashed diffs, we’ll
need to both figure out a better UI to distinguish them from the
individual commits and also settle several open questions:
Should reviewers be able to grant ship its (i.e. r+s) on
squashed diffs? This would imply that the commits probably
haven’t been reviewed individually, which would defeat the
purpose of a commit-centric system. That said, reviewer time is
very important, so we could have a trade off to support more
Conversely, should reviewers be able to leave comments on the
parent diff? For simplicity, we could allow reviewers to leave
a “ship it” review on a squashed diff that would apply to all
commits but force them to leave any comments on diffs on the
commits themselves. This would essentially remove the ability
to review squashed diffs themselves but would leave the
convenience of saying “this is all good”.
If we do want to allow review comments on squashed diffs, how
should they be consolidated with the reviews on individual
commits? Right now, reviews (general comments and comments on
diffs) for the squashed diff and all commits are all on separate
pages/views. Giving one view into all activity on a commit
series would be ideal if we want to support squashed-diff
reviews. Arguably, this would be valuable even if we didn’t have
reviews on squashed diffs.
For comparison, GitHub pull requests support this model. There
are three tabs in a pull request: “Files changed”, which is the
squashed diff; “Commits”, which is a list of commits with links to
the individual commit diffs; and “Conversation”, which shows
comments on the commits and on the squashed diff (along with other
events like updates to the commits). The way they are presented
is a little confusing (comments on the squashed diff are just
labelled “<user> commented on the diff”, whereas comments on the
diffs are of the form “<user> commented on <file> in <commit
hash>”), but it is a useful single view. However, note that pull
requests do not have the concept of a “ship it” or “r+”, which
makes the GitHub interface simpler.
This approach would support multiple reviewer work flows, but it
is also the most complicated, both in terms of UX and technical
implementation, and it waters down the philosophy behind
Provide read-only overview diffs.
The third approach is to keep squashed diffs but make them read
only. They could be used as reference, to get a big picture of
the whole series, but since they are read only, they would be
easily distinguishable from commits and would force reviewers to
look at the individual commits. This is really just option 1
above, with a reference view of the whole series. It would be
more work than option 1 but less than option 2, and would preserve
The MozReview team has been leaning towards option 3. We have a mock-up
that strips away a lot of the UI that would be useless in this
scenario and makes the intention clear. It’s not the prettiest, but
it wouldn’t take too much work to get here:
However, we’d like to hear user feedback before making any decisions.
Whichever option we go with, we’ll come up with a plan to get there
that ideally will have incremental improvements, depending on the
complexity of the full solution, so that we can start to fix things
October 22, 2015 06:29 PM
October 14, 2015
Starting today, a Mozilla LDAP account with Mercurial SSH access is no
longer required to hg push into
MozReview to initiate code review
with Mozilla projects.
The instructions for configuring your client to use MozReview
have been updated to reflect how you can now push to MozReview over HTTP
using a Bugzilla API Key for authentication.
This change effectively enables first-time contributors to use MozReview
for code review. Before, you had to obtain an LDAP account and configure
your SSH client, both of which could be time consuming processes and
therefore discourage people from contributing. (Or you could just use
Bugzilla/Splinter and not get the benefits of MozReview, which many
I encourage others to update contribution docs to start nudging people
towards MozReview over Bugzilla/patch-based workflows (such as
tracked this feature.
October 14, 2015 12:30 PM
September 30, 2015
MozReview was intentionally released early, with a fairly minimal
feature set, and some ugly things bolted onto a packaged code-review
tool. The code-review process at Mozilla hasn’t changed much since
the project began—Splinter, a slightly fancier UI than dealing with
raw diffs, notwithstanding. We knew this would be a controversial
subject, with a variety of (invariably strong) opinions. But we also
knew that we couldn’t make meaningful progress on a number of
long-desired features, like autolanding commits and automatic code
analysis, without moving to a modern repository-based review system.
We also knew that, with the incredible popularity of GitHub, many
developers expect a workflow that involves them pushing up commits for
review in a rich web UI, not exporting discrete patches and looking at
almost raw diffs.
Rather than spending a couple years off in isolation developing a
polished system that might not hit our goals of both appealing to
future Mozillians and increasing productivity overall, we released a
very early product—the basic set of features required by a push-based,
repository-centric code-review system. Ironically, perhaps, this has
decreased the productivity of some people, since the new system is
somewhat raw and most definitely a big change from the old. It’s our
sincere belief that the pain currently experienced by some people,
while definitely regrettable and in some cases unexpected, will be
balanced, in the long run, by the opportunities to regularly correct
our course and reach the goal of a world-class code
review-and-submission system that much faster.
And so, as expected, we’ve received quite a bit of feedback. I’ve
been noticing a pattern, which is great, because it gives us insight
into classes of problems and needs. I’ve identified four categories,
which interestingly correspond to levels of usage, from basic to
Understanding MozReview’s (and Review Board’s) models
Some users find MozReview very opaque. They aren’t sure what many of
the buttons and widgets do, and, in general, are confused by the
interface. This caught us a little off-guard but, in retrospect, is
understandable. Review Board is a big change from Splinter and much
more complex. I believe one of the sources of most confusion is the
overall review model, with its various states, views, entry points,
and exit points. Splinter has the concept of a review in progress,
but it is a lot simpler.
We also had to add the concept of a series of related commits to
Review Board, which on its own has essentially a patch-based model,
similar to Splinter’s, that’s too limited to build on. The
relationship between a parent review request and the individual
“child” commits is the source of a lot of bewilderment.
Improving the overall user experience of performing a review is a top
priority for the next quarter. I’ll explore the combination of the
complexity of Review Board and the commit-series model we added in a
Inconveniences and lack of clarity around some features
For users who are generally satisfied by MozReview, at least, enough
to use it without getting too frustrated, there are a number of paper
cuts and limitations that can be worked around but generate some
annoyance. This is an area we knew we were going to have to improve.
We don’t yet have parity with Splinter/Bugzilla attachments,
e.g. reviewers can’t delegate review requests, nor can they mark
specific files as reviewed. There are other areas that we can go
beyond Bugzilla, such as being able to land parts of a commit series
(this is technically possible in Bugzilla by having separate patches,
but it’s difficult to track). And there are specific things that
Review Board has that aren’t as useful for us as they could be, like
This will also be a big part of the work in the next quarter (at least).
Inability to use MozReview at all due to technological limitations
The single biggest item here is lack of support for git, particularly
a git interface for hg repos like mozilla-central. There are many
people interested in using MozReview, but their work flows are based
around git using git-cinnabar. gps and kanru did some initial work
around this in bug 1153053; fleshing this out into a proper
solution isn’t a small task, but it seems clear that we’ll have to
finish it regardless before too long, if we want MozReview to be the
central code-review tool at Mozilla. We’re still trying to decide how
this fits into the above priorities; more users is good, but making
existing users happier is as well.
As mentioned at the beginning of this post, the main reason we’re
building a new review tool is to make it repository-centric, that is,
based around commits, not isolated patches. This makes a lot of
long-desired tools and features much more feasible, including
autoland, automatic static analysis, commit rewriting to automatically
include metadata like reviewers, and a bunch of other things.
This has been a big focus for the last few months. We’ve had
autoland-to-try for a little while now, and autoland-to-inbound is
nearly complete. We have a generic library for static analysis with
which we’ll be able to build various review bots. And, of course, the
one big feature we started with, the ability push commits to MozReview
instead of exporting standalone patches, which by itself is both more
convenient and preserves more information.
After autoland-to-inbound we’ll be putting aside other big features
for a little while to concentrate on general user experience so that
people enjoy using MozReview, but rest assured we’ll be back here to
build more powerful workflows for everyone.
September 30, 2015 06:15 PM
September 14, 2015
Two weeks ago the MozReview developers got together to do some focussed
hacking. It was a great week, we got a lot of stuff done, and we
clarified our priorities for the coming months. We deployed several
new features and improvements during the week, and we made good
progress on several other goals.
For this week, we actually opted to not go to a Mozilla office and
instead rented an AirBNB in Montreal—our own little hacker house! It
was a great experience. There was no commuting (except for me, since
I live here and opted to sleep at my house) and no distractions. One
evening when we were particularly in the zone, we ordered a bunch of
pizzas instead of going out, although we made sure to take breaks and
go outside regularly on all the other days, in order to refresh our
minds. Five out of five participants said they’d do it again!
See also dminor’s post about the week, with a bit more detail on
what he worked on.
What we pushed out
My main contribution to the week was finally switching MozReview to
Bugzilla’s API-key-based authentication-delegation system. I’d been
working on this for the last couple months when I found time, and it
was a big relief to finally see it in action. I won’t go into detail
here, since I’ve already blogged about it and announced it to
gps, working amazingly fast as always, got API-key support working
on the command line almost immediately after UI support was deployed.
No more storing credentials or login cookies!
Moving on, we know the MozReview’s UI could… stand some improvement,
to say the least. So we split off some standalone work from Autoland
around clarifying the status of reviews. Now in the commit table
you’ll see something like this:
This warrants a bit of explanation, since we’re reusing some terms
from Bugzilla but changing them a little.
r+ indicates at least one ship-it, subject to the following:
- If the author has L3 commit access, the ship-it review can be
- If the author does not have L3 commit access, at least one ship-it
is from a reviewer with L3 access.
The reason for the L3 requirement is for Autoland. Since a human
won’t necessarily be looking at your patch between the time that a
review is granted and the commit landing in tree, we want some checks
and balances. If you have L3 yourself, you’re trusted enough to not
require an L3 reviewer, and vice versa. We know this is a bit
different from how things work right now with regards to the
checkin-needed flag, so we’re going to open a discussion on
mozilla.governance about this policy.
If one or more reviewers have left any issues, the icon will be the
number of open issues beside an exclamation mark on a yellow
backgroud. If that or any other reviewer has also left a ship-it (the
equivalent of an “r+ with minor issues”), the issue count will switch
to an r+ when all the issues have been marked as fixed or dropped.
If there are no open issues nor any ship-its, a grey r? will be
We’ve also got some work in progress to make it clearer who has left what
kind of review that should be live in a week or two.
We also removed the ship-it button. While convenient if you have
nothing else to say in your review, it’s caused confusion for new
users, who don’t understand the difference between the “Ship It!” and
“Review” buttons. Instead, we now have just one button, labelled
“Finalize Review”, that lets you leave general comments and, if
desired, a ship-it. We plan on improving this dialog to make it
easier to use if you really just want to leave just a ship-it and no
Finally, since our automation features will be growing, we moved the
Autoland-to-try button to a new Automation menu.
Where we’re going
As alluded to above, we’re actively working on Autoland and trying to
land supporting features as they’re ready. We’re aiming to have this
out in a few weeks; more posts to come.
Much of the rest of our plan for the next quarter or two is around
user experience. For starters, MozReview has to support the same
feature set as Splinter/Bugzilla. This means implementing things like
marking files as reviewed and passing your review onto someone else.
We also need to continue to improve the user interface by further
differentiating between parent review requests, which are intended only
to provide an overview of the whole commit series, and child review
requests, which is where the actual reviews should happen.
Particularly confusing is when there’s only one child, which means the
parent and the child are nearly indistinguishable (of course in this
case the parent isn’t really needed, but hiding or otherwise doing
away with the parent in this case could also be confusing).
And then we have some other big-ticket items like static analysis,
which we started a while back; release-management tools; enabling Review
Board’s rich emails; partial-series landing (being able to land
earlier commits as they are reviewed without confusing MozReview in
the next push); and, of course, git support, which is going to be
tricky but will undoubtedly make a number of people happy.
Our priorities are currently documented on our new road map, which
we’ll update at least once or twice a quarter. In particular, we’ll
be going over it again soon once we have the results from our
September 14, 2015 07:26 PM
July 16, 2015
As of today, ~15.6% of commits landing in Firefox in July have gone
through MozReview or have been produced
on machines that have used MozReview. This is still a small percentage
of overall commits. But, signs are that the percentage is going up. Last
month, about half as many commits exhibited the same signature. It's
only July 16 and we've already passed the total from June.
What I find interesting is the differences between commits that have
gone through MozReview versus the rest. When you look at the diff
statistics (a quick proxy of change size), we find that MozReview
commits tend to be smaller. The median adds as reported by diff stat
(basically lines that were changed) is 12 for MozReview versus 17
elsewhere. The average is 58 for MozReview versus 100 elsewhere. For
number of files modified, MozReview averages 2.59 versus elsewhere's
2.71. (These numbers exclude some specific large commits that appeared to
be bulk imports of external projects and drove up the non-MozReview
It's entirely possible the root cause behind the discrepancy is a
side-effect of the population of MozReview users: perhaps MozReview
users just write smaller commits. However, I'd like to think it's because
MozReview makes it easier to manage multiple commits and people are taking
advantage of that (this is an explicit design goal of MozReview). Whatever
the root cause, I'm glad diffs are smaller.
As I've written about before,
smaller commits are easier to review and land, thus enabling projects to
I have a quarterly goal to remove the requirement for a Mozilla LDAP
account to push to MozReview. That will allow first time contributors to
use MozReview. This will be a huge win, as we can do much more magic in
the MozReview world than we can from vanilla Bugzilla (automatic bug
filing, automatic reviewer assignment, etc). Unofficially, I'd like to
have more than 50% of Firefox commits go through MozReview by the end of
July 16, 2015 02:00 PM
July 07, 2015
A lot of people contributed some really great feedback about MozReview
at Whistler. One of the most frequent requests was for the ability to
publish submitted review requests without having to open a browser.
I'm pleased to report that as of yesterday, this feature is implemented!
If reviewers have been assigned to all your review requests, Mercurial
will now prompt you to publish the review requests during hg push.
It should just work.
As part of this change, we also introduced more advanced feature
negotiation into the handshake between client and server. This means
we now have a mechanism for detecting out-of-date client installations.
This will enable us to more aggressively drop backwards compatibility
(making server-side development easier) while simultaneously ensuring
that more people are running modern and hopefully better versions of the
client code. This should translate to moving faster and a better
experience for everyone.
July 07, 2015 02:55 PM
May 29, 2015
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
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.
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
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.
May 29, 2015 04:20 PM
January 27, 2015
It is common for commit messages in Firefox to contains strings like
Part 1, Part 2, etc. See
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
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.
January 27, 2015 08:17 PM
January 24, 2015
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.
because it was the easiest to integrate (it has sane and efficient
tooling that is compatible with Mozilla's code bases - most existing
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
Report issues in #mozreview or in the Developer Services :: MozReview
January 24, 2015 11:30 PM
January 16, 2015
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
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
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
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
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
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
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
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
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.
January 16, 2015 10:50 AM
December 04, 2014
You may have noticed the Firefox trees
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
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
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
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
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
December 04, 2014 08:40 AM