On Tue, Jan 27, 2009 at 10:04 PM, Brett Wilson <[email protected]> wrote:
> > There are a lot of patches lately with a lot of reviewers on them, > especially related to porting since a lot of people might need to be > in the loop for some changes. > > The problem is that there's no clear responsibility given in these > reviews. If I'm the sole reviewer on a change, I know I have to do a > good job. When there are three other people, I sometimes assume that > somebody else must have looked carefully at some part of the review. > I'm sure sometimes all the reviewers think this and the change isn't > reviewed properly. > > In other cases, some reviewers say LGTM for a patch, while others are > still expecting changes. The author can get confused as to the status > of the review, and I also know some patches have been checked in > lately where at least one reviewer expected further changes before > checkin. > > At the same time, we want to encourage many people to participate in > the review process and keep tabs on what's going on. I propose several > changes to best practices: > > 1. When a patch author requests more than one reviewer, they should > make clear in the review request email what they expect the > responsibility of each reviewer to be. Example: > agl: bitmap changes > evanm: process hacks > everybody else: FYI > In this case, I might be on the review list because I've asked to be > in the loop for multiprocess changes, but I wouldn't be the primary > reviewer and the author and other reviewers wouldn't be expecting me > to review all the diffs in detail. > > 2. If you get a review that includes many other people, and the author > didn't do (1), please ask them what part you're responsible for if you > don't want to review the whole thing in detail. > > 3. The author should wait for approval from everybody on the reviewer > list before checking in. > > 4. People who are on a review without clear review responsibility > should be super responsive and not hold up the review. The patch > author should feel free to ping them mercilessly if they are. > > 5. If you're on "FYI" person on a review and you didn't actually > review in detail (or at all), but don't have a problem with the patch, > note this. You could say something like "rubber stamp" or "ACK" > instead of "LGTM." This way the real reviewers know not to trust that > you did their work for them, but the author of the patch knows they > don't have to wait for further feedback from you. > > Hopefully we can still keep everybody in the loop but have clear > ownership and detailed reviews. It might even speed up some patches > since I can quickly ACK patches I don't care about, and the patch > author knows they don't have to wait for feedback from me. Or do you > think this has too much overhead? > > Comments? > > Brett > This is a great idea! -Darin --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
