Let's say you've created your very first Mozilla patch for some bug, for instance making Control+left and Control+right work properly (By the way, if this bug isn't fixed soon, Firefox is doomed to fail). You're fairly new to Mozilla development so you aren't very familiar with the whole process. You do know however that you need to create a patch and attach it to the necessary bug in Bugzilla. So you click on the handy link that says "Create a New Attachment". You fill in the bits about the file path, the description and check the "patch" checkbox indicating that you are submitting a patch. Then you get to a bit with a bunch of flags. It looks something like this:
You've read somewhere in a place you can no longer find that Mozilla code needs to be reviewed by someone before being applied, so you assume by the labels that the flags have something to do with reviewing the bug or patch. There are a bunch of flags, each with a dropdown menu, and two of those have a grey box beside them. You look in the menu of the "review" line and see that your choices are "+", "-" and "?". Confused, you wonder why there are just symbols and no descriptive text. You reload the page just to make sure the page loaded properly, and then, being a curious sort, you look at the source code to see that indeed, yes, those labels are what is intended to be there. Hmmm. Do the labels mean "Addition", "Subtraction" and "Whaaa..?". There's no description of what the flags mean, nor any link to any help.
But you aren't going to give up just yet. You try choosing the items from the list. The only thing of note is that the neighbouring grey box turns white when "?" is selected. Perhaps that's what you should choose, but it's the last item on the menu, so that seems unlikely. You'd imagine that the item you are supposed to choose is first on the list, or better, chosen by default. You choose "?" and notice that you can now type something into the now white box. But what should you enter? There is the label "Requestee" above the box. Since "Requestee" isn't a real word, it doesn't tell you much, although it implies from the form of this "word" that it is referring to some person. Maybe you are supposed to enter your name there. That would be strange though, since Bugzilla should know your name since you already logged in. Perhaps, you are supposed to enter someone else's name in there, such as the person who should look at your patch. Sadly, you're new here, and the only Mozilla person you are familar with is Asa, and you don't think he does anything with code. You aren't sure how to find out who the real Mozilla developers are that might be able to help you or how to contact them.
You look at the other flags but they are all similar in nature. No descriptive text and the same choices in the dropdown. Resigned, you just press Submit and hope somebody knows what to do. Two months later, you get a flurry of messages pertaining to the bug created by someone named Boris Zbarsky; although you don't understand the messages, after a while the bug somehow gets fixed. You wonder what it all meant, but you try downloading a new Firefox build and note with joy that the bug is indeed fixed, and Firefox isn't doomed after all.
In case you're wondering, to request a code review for a patch, you're supposed to choose "?" from the list beside "review", and then enter the email address of the person who you want to do the review in the "Requestee" field. It's usually fairly difficult to determine who you should enter unless you have been working with Mozilla code for a while, usually over a year or so, and it's usually about 2% easier when you have. You can also set the superreview flag in a similar way, and set various approval flags. The difference between a review and a superreview can be difficult to understand -- I certainly don't really know exactly -- but generally a superreview is just a regular review but done by someone wearing a cape. Approvals can only be done by people with cars.
Then there's a bug that I filed that has an entirely different set of flags, in this case 'first-review' instead of 'review' and 'second-review' instead of 'superreview'. I don't know why this is different, nor what it means. The first person to correctly answer this will receive a free box of gummy worms.
The + and - values for a review flag mean code reviewed and accepted versus rejected respectively. Now, it really doesn't make sense to be able to mark a patch as reviewed by the person submitting the patch, yet the UI still provides this option. The other thing the UI lets the submitter do is decide whether a patch should be reviewed or not, which would only be of value to an experienced developer -- these people sometimes submit works in progress as patches.
The additional more complex concern is what to enter in the "Requestee" field. There's no indication of who might be allowed to do such a review. There's actually a finite number of people from a list that can do a superreview, so the list might just be presented there for someone to choose from. For review, it could just select from a set of reviewers associated with the component the bug is filed in. Naturally, that won't work if the bug is misfiled, but the bug should actually be recategorized anyway. For instance, a bug in View Source would allow selection from one of the reviewers associated with the View Source component.
To go further, there's no real reason to be selecting a specific person to do a review. Instead, a suitable reviewer should be selected automatically either based on the component, or for the brave, by analyzing the patch to see which files are modified. For most components, this would generally work since there's only one reviewer doing reviews of that code anyway. For other things like layout bugs, the patches could just go into a queue for triage by someone who knows better who should be reviewing it. Of course, I'm not suggesting that the ability to select a specific reviewer be removed, for those people that know what they're doing, they should be able to select a specific person.
When selecting a reviewer, Bugzilla should provide an indication of how active a reviewer is and how many patches are in that person's queue. For instance, if a reviewer has fifty patches and hasn't reviewed any for two months, it probably isn't a good idea to choose that person. In fact, any automated reviewer selection should take this into account and select the reviewer who is most likely to actually do a review.
Thus, the UI for review should just have three options: Request code review from the default reviewer, Request code review from a specific person, and Don't request code review. The superreview could be similar, or for simplicity, only available to be specified if the code review option was set to 'specific person'. The approval flags should either be removed, if it's the case that unreviewed patches can't be approved anyway, or if approving unreviewed patches is allowed, the labels changed to clarify what you're actually requesting. For instance, instead of 'approval1.8rc2', there should be a checkbox that reads "Request approval for Mozilla 1.8 Release Candidate 2" with a link that describes what requesting approval actually means.
If you think these are good ideas, let's discuss and file enhancement bugs to improve Bugzilla's review request UI.
