Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Idea: No patch left behind

100 views
Skip to first unread message

Taras Glek

unread,
Aug 10, 2011, 5:33:18 PM8/10/11
to
Hi,
I find that a lot of contributors(myself included) get discouraged due
to having their patches rot in the review queue.
It's not fair to the contributors have a patch rot without any feedback.
It's also not great to r+ patch a month after it was submitted.

I propose a two step solution to this

1) Every bugzilla account should have bugzilla
whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
on to remind reviewers about outstanding r? requests.

2) Institute a policy where reviewers have to reassign r? to someone
else if they know they wont be able to review something within a week.
It might turn into a game of hot potato, or it might end up with r?
flags being removed from attachments. I think that's better than quietly
ignoring the problem.

I realize that some people do a lot more reviews than others, but
letting patches languish in the queue doesn't make those queues smaller.
We should have more fallback reviewers instead.


Taras

smaug

unread,
Aug 10, 2011, 6:34:00 PM8/10/11
to Taras Glek
On 08/11/2011 12:33 AM, Taras Glek wrote:
> Hi,
> I find that a lot of contributors(myself included) get discouraged due
> to having their patches rot in the review queue.
> It's not fair to the contributors have a patch rot without any feedback.
> It's also not great to r+ patch a month after it was submitted.
>
> I propose a two step solution to this
>
> 1) Every bugzilla account should have bugzilla
> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
> on to remind reviewers about outstanding r? requests.

Yes, I'd like to get reminded if I haven't reviewed a patch in a week or so.


>
> 2) Institute a policy where reviewers have to reassign r? to someone
> else if they know they wont be able to review something within a week.
> It might turn into a game of hot potato, or it might end up with r?
> flags being removed from attachments. I think that's better than quietly
> ignoring the problem.

Sounds ok to me.

>
> I realize that some people do a lot more reviews than others, but
> letting patches languish in the queue doesn't make those queues smaller.
> We should have more fallback reviewers instead.

I do quite a few reviews, and I'd like to have tools to help my work.
One thing is to get reminders.


Hmm, I have bz_canusewhines. Why haven't I created such reminder yet :/


-Olli

>
>
> Taras

Robert O'Callahan

unread,
Aug 11, 2011, 12:35:42 AM8/11/11
to Taras Glek, dev-pl...@lists.mozilla.org
On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek <tg...@mozilla.com> wrote:

> I find that a lot of contributors(myself included) get discouraged due to
> having their patches rot in the review queue.
> It's not fair to the contributors have a patch rot without any feedback.
> It's also not great to r+ patch a month after it was submitted.
>

Are patches accidentally falling through the cracks, or being deliberately
ignored?

If the latter, I think the only effective approach is to tackle the
delinquent reviewers directly. If one-on-one engagement doesn't work,
escalate.

Rob
--
"If we claim to be without sin, we deceive ourselves and the truth is not in
us. If we confess our sins, he is faithful and just and will forgive us our
sins and purify us from all unrighteousness. If we claim we have not sinned,
we make him out to be a liar and his word is not in us." [1 John 1:8-10]

Mark Banner

unread,
Aug 11, 2011, 5:42:26 AM8/11/11
to
On 10/08/2011 22:33, Taras Glek wrote:
> Hi,
> I find that a lot of contributors(myself included) get discouraged due
> to having their patches rot in the review queue.
> It's not fair to the contributors have a patch rot without any feedback.
> It's also not great to r+ patch a month after it was submitted.
>
> I propose a two step solution to this

One of the things I started doing at the beginning of the year was
having an aim of getting reviews in the Thunderbird product down to
something like an average of 2 weeks, and MailNews Core to 3 weeks (iirc).

Unfortunately this effort has been sidetracked a bit with me getting
lots of other things to do, however, I think it has reasonable success.

We started sometime towards the end of last year with patches that had
been in the review queue for several years. We looked at those, we
review and landed patches if appropriate (sometimes just finishing the
work ourselves rather than waiting to see if the original contributor
was still around); we cancelled/denied reviews if a new patch was
required etc.

Once we'd gotten rid of the majority of the backlog, we then started
pushing reviewers to get patches reviewed quickly, at one point we
almost hit 0 outstanding review requests.

Recently we've had more turn over of patches and reviews, and it is
holiday time, so we've got more in progress at any one time, but without
re-running the stats I'm guessing most patches are generally reviewed
within 2-3 weeks at the outside, and probably averaging a week. Our
oldest request in Thunderbird product is currently from the mid July,
which is way better than the several years we used to have.

One of the favourite ones for clearing old reviews was looking at open
review requests on closed bugs - if the bug is closed, why is it
there... (I even have a whine set up for the products I watch).

The advantage of getting rid of the backlog first was that it gave us a
much clearer view of the more recent reviews and we can easily see when
some are getting left behind.

We haven't been so successful in mailnews core yet as that had a larger
backlog and we're still trying to get that sorted out.

> 1) Every bugzilla account should have bugzilla
> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
> on to remind reviewers about outstanding r? requests.

I think that's reasonable, but I don't think that stops people with a
large review queue from ignoring it - getting a whine email every week
(say) doesn't mean you'll do something about it.

Generally I think reviewers need to be getting into a habit of looking
at their queues on a frequent basis. Obviously some methods will work
better for some folks than others.

> 2) Institute a policy where reviewers have to reassign r? to someone
> else if they know they wont be able to review something within a week.
> It might turn into a game of hot potato, or it might end up with r?
> flags being removed from attachments. I think that's better than quietly
> ignoring the problem.

One of the problems with bugzilla with this approach is that you then
loose easy visibility when review on the patch was first requested, and
how long that's been going on (for example, My Reviews, just shows the
date the request was created/reassigned, not when the flag was first
requested).

> I realize that some people do a lot more reviews than others, but
> letting patches languish in the queue doesn't make those queues smaller.
> We should have more fallback reviewers instead.

I think what we need to have is module owners/team leads frequently
looking at queues and what's happening with them. If patches are getting
left behind, why is that, which of course leads onto do we need to
reassign those reviews to someone else or do we need to get more reviewers.

Standard8

Brad Lassey

unread,
Aug 11, 2011, 12:51:09 PM8/11/11
to
I was recently told of a company that escalates any expense report that
hasn't been approved in 24 hours to the next level of management up. It
might be helpful to something similar in bugzilla. If a review hasn't
been processed in say a week, bugzilla could automatically re-assign it
to the module owner (based on the product and component).

The problem I'm trying to solve here is reviews requested of people who
are traveling, on PTO, paternity/maternity leave etc. and thus not
paying attention to bugzilla.


Zack Weinberg

unread,
Aug 11, 2011, 1:01:04 PM8/11/11
to

I'm not sure that would help; it's been my experience (particularly in
dusty corners of gecko core) that the module owners are the people who
don't do their reviews.

zw

Taras Glek

unread,
Aug 11, 2011, 1:26:30 PM8/11/11
to rob...@ocallahan.org
On 08/10/2011 09:35 PM, Robert O'Callahan wrote:
> On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek<tg...@mozilla.com> wrote:
>
>> I find that a lot of contributors(myself included) get discouraged due to
>> having their patches rot in the review queue.
>> It's not fair to the contributors have a patch rot without any feedback.
>> It's also not great to r+ patch a month after it was submitted.
>>
>
> Are patches accidentally falling through the cracks, or being deliberately
> ignored?
>
> If the latter, I think the only effective approach is to tackle the
> delinquent reviewers directly. If one-on-one engagement doesn't work,
> escalate.

I think it's a bit of both. We need to make it easier to keep track of
own review queue and to find people with delinquent reviews.

Perhaps someone can cook up a bugzilla search to track stale r? requests
and publicize that on some mailing list?

"Developer XYZ hasn't reviewed bug #### in a week" -> then someone else
could pick up the review.

Taras

Christian Legnitto

unread,
Aug 11, 2011, 1:34:44 PM8/11/11
to Taras Glek, dev-pl...@lists.mozilla.org

FWIW I think it would be pretty trivial to write an extension to do the email whine by default (with cc'ing a mailing list or whatever) followed by reassigning the review via a per-module list of prioritized fallback reviewers.

Bugzilla extension docs are here:

http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Extension.html

And there are staging installs to test during development:

http://landfill.bugzilla.org/

Sounds like a good intern or new contributor project to me (assuming that this is a real problem, I don't see any data presented to back up the assertion yet).

Thanks,
Christian

Asa Dotzler

unread,
Aug 11, 2011, 1:47:52 PM8/11/11
to Taras Glek, rob...@ocallahan.org
Taras Glek wrote:
> Perhaps someone can cook up a bugzilla search to track stale r? requests
> and publicize that on some mailing list?
>
> "Developer XYZ hasn't reviewed bug #### in a week" -> then someone else
> could pick up the review.
>
> Taras


A quick skim of the pending requests gets you pretty close, though it's
not automatic.

https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=Firefox&type=review&requestee=&component=&group=requestee

for example.

- A

Mark Banner

unread,
Aug 11, 2011, 1:53:41 PM8/11/11
to
On 11/08/2011 18:26, Taras Glek wrote:
> Perhaps someone can cook up a bugzilla search to track stale r? requests
> and publicize that on some mailing list?

Easy:

https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=&type=review&requestee=&component=&group=type

Goes back to 2002.

Just ignore the ones at the bottom of the list.

Standard8

gavin

unread,
Aug 11, 2011, 2:06:44 PM8/11/11
to
On Aug 11, 10:26 am, Taras Glek <tg...@mozilla.com> wrote:
> Perhaps someone can cook up a bugzilla search to track stale r? requests
> and publicize that on some mailing list?
>
> "Developer XYZ hasn't reviewed bug #### in a week" -> then someone else
> could pick up the review.

That's a good idea. A good portion of the patches that sit in my
review queue are patches to code that no one really "owns" - I ended
up being picked as a reviewer because I'm a peer of the module that
the code is nominally part of, not because I'm particularly familiar
with the code. These never get very high on the priority list because
they're usually not very important fixes, and performing the review
requires a lot of familiarizing yourself with old/unknown code. A more
formalized way to request review "from the wind" for a given patch
would let me indicate, by forwarding the request, that anyone who
feels like reviewing the patch should feel free to (even if they're
not officially "peers") - I might want to double check the review
after the fact, but it doesn't need to block on me in particular. That
would also be an effective way to develop ownership of unowned areas
of code.

Gavin

Brad Lassey

unread,
Aug 11, 2011, 2:14:48 PM8/11/11
to
On 08/11/2011 01:01 PM, Zack Weinberg wrote:
> I'm not sure that would help; it's been my experience (particularly in
> dusty corners of gecko core) that the module owners are the people who
> don't do their reviews.
>
> zw

That, IMO, is a another problem to be solved. Absentee module owners
should be replaced.

Boris Zbarsky

unread,
Aug 11, 2011, 2:20:12 PM8/11/11
to
On 8/11/11 2:14 PM, Brad Lassey wrote:
> That, IMO, is a another problem to be solved. Absentee module owners
> should be replaced.

How much time are we expecting module owners to spend on reviews? How
much time do reviews take?

My data from my personal experience is that I tend to have lots of
review requests that are more than a week old in my queue, and that I'm
spending at least 50% of my work time on reviews, fwiw.

-Boris

Philip Chee

unread,
Aug 11, 2011, 2:31:54 PM8/11/11
to

How would you deal with people like, um, timeless, with review requests
going back to 2004.

Phil

--
Philip Chee <phi...@aleytys.pc.my>, <phili...@gmail.com>
http://flashblock.mozdev.org/ http://xsidebar.mozdev.org
Guard us from the she-wolf and the wolf, and guard us from the thief,
oh Night, and so be good for us to pass.

Mike Shaver

unread,
Aug 11, 2011, 2:32:40 PM8/11/11
to Brad Lassey, dev-pl...@lists.mozilla.org
On Thu, Aug 11, 2011 at 2:14 PM, Brad Lassey <bla...@mozilla.com> wrote:
> That, IMO, is a another problem to be solved. Absentee module owners should
> be replaced.

With...

If there were more-suitable module owners around, they'd probably
already be the module owner, right? If there's a module owner that's
not keeping up with their reviews, and you offer to take over the
module from them, I bet they'll kiss you.

Mike

Brad Lassey

unread,
Aug 11, 2011, 2:41:39 PM8/11/11
to
On 08/11/2011 02:32 PM, Mike Shaver wrote:
> If there were more-suitable module owners around, they'd probably
> already be the module owner, right? If there's a module owner that's
> not keeping up with their reviews, and you offer to take over the
> module from them, I bet they'll kiss you.

More saying that we should make an effort to find suitable module owners
(or active peers to pick up the slack) for modules that are falling
behind on reviews. It could be the case that absentee module owners
aren't aware that others view them as absentee.

Also, my goal for re-assigning reviews to module owners isn't to make
module owners do the reviews, but to rather give them the responsibility
to find someone to do the reviews.

Robert Kaiser

unread,
Aug 11, 2011, 2:50:16 PM8/11/11
to
Mark Banner schrieb:

Wow, some impressive requester/requestee combinations around near the
top... people who aren't working on Mozilla stuff any more, etc.
And I wonder how many patches even still apply somewhat.

Robert Kaiser


--
Note that any statements of mine - no matter how passionate - are never
meant to be offensive but very often as food for thought or possible
arguments that we as a community should think about. And most of the
time, I even appreciate irony and fun! :)

Robert Kaiser

unread,
Aug 11, 2011, 2:55:03 PM8/11/11
to
Philip Chee schrieb:

> How would you deal with people like, um, timeless, with review requests
> going back to 2004.

Timeless at least tends to still be around somewhat - and then, I don't
see a lot of requests *to* him, just *from* him, the oldest one where
he's the requestee is 2006, and it's a rare case. The other way round
isn't. People like leaf and a number of others are much more of a
problem, as they don't work on Mozilla stuff any more and haven't been
seen around anywhere for a long time.

L. David Baron

unread,
Aug 11, 2011, 2:34:53 PM8/11/11
to dev-pl...@lists.mozilla.org
On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote:
> I'm not sure that would help; it's been my experience (particularly
> in dusty corners of gecko core) that the module owners are the
> people who don't do their reviews.

Yeah, that's probably me. I've been spending as much time as I can
on reviews lately (it's my primary job right now; I basically
haven't been writing code), but there's also a limit to the number
of hours a day I can spend doing code reviews and maintain my
sanity.


The stuff in my review queue is really a mix of things (to use some
extreme examples):

* patches with well-defined goals that the author is, with good
reason, reasonably confident in

* like above, except from new contributors who need much more
careful review and mentoring

* patches that redesign a subsystem without previous discussion
with the other maintainers of the code (of, e.g., what the
constraints that led to the current design are or what other
changes need to happen soon in that area of code), in other
words, where the review request essentially requests either
rubber-stamping of the new design, or that a replacement design
be offered (which is a lot of work)

* patches that fix the case that the patch author cares about but
break a lot of others -- in other words, patches where the author
has done the first 20% of the work, is asking me to do the next
50%, and is quite willing to do the typing at the end to finish
the last 30%

These aren't really clear divisions: many patches are in between
these extremes.

Should things in or near the latter two categories be treated the
same as things in the former categories? Should whether I think the
patch is important (or whether I think it's even worth the time
being spent on it) influence how quickly I review patches in any or
all of these categories? I tend not to treat things in the latter
categories the same, but all the same I would like to get to them
eventually, so I'd prefer not to kick them out of my review queue.
They're also frequently design tasks that are hard to delegate to
people who aren't experts in the code.

Perhaps what would help the most is if anything that risks falling
into the latter two categories had an explicit design review stage
sometime before a 100K patch appears in somebody's review queue.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla Corporation http://www.mozilla.com/ 𝄂

Mike Shaver

unread,
Aug 11, 2011, 3:11:36 PM8/11/11
to L. David Baron, dev-pl...@lists.mozilla.org
On Thu, Aug 11, 2011 at 2:34 PM, L. David Baron <dba...@dbaron.org> wrote:
> Perhaps what would help the most is if anything that risks falling
> into the latter two categories had an explicit design review stage
> sometime before a 100K patch appears in somebody's review queue.

IMO it's perfectly reasonable to bounce those sorts of reviews with
"changes of this magnitude need some more discussion before I can
review a patch against them; can you start a thread in .platform about
it and put the link in here?"

Mike

Zack Weinberg

unread,
Aug 11, 2011, 5:41:54 PM8/11/11
to
On 2011-08-11 11:34 AM, L. David Baron wrote:
> On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote:
>> I'm not sure that would help; it's been my experience (particularly
>> in dusty corners of gecko core) that the module owners are the
>> people who don't do their reviews.
>
> Yeah, that's probably me. I've been spending as much time as I can
> on reviews lately (it's my primary job right now; I basically
> haven't been writing code), but there's also a limit to the number
> of hours a day I can spend doing code reviews and maintain my
> sanity.

I actually wasn't thinking of either you or Boris; I trust both of you
to come through with reviews eventually, and I know how swamped you are.
(In fact, I have the impression that Boris is swamped partially
_because_ he's the only person who will review patches in a timely
fashion in large areas of the codebase.) You also usually tell people
(or anyway, you usually tell me :) when you're not going to get to
something soon.

My experience with other module owners has not been so good. To pick
four examples not at random:

https://bugzilla.mozilla.org/show_bug.cgi?id=563192
Review requested from, AFAIK, the *only person* who can review changes
to NSPR; no reaction whatsoever for over a year. (Have I mentioned
recently that we need to get rid of NSPR?)

https://bugzilla.mozilla.org/show_bug.cgi?id=643041
Super-reviewer asked a third person for additional review; no reaction
from that third person for five months. Super-reviewer also raised some
fuzzy concerns; I asked some questions in response; no reply for five
months.

https://bugzilla.mozilla.org/show_bug.cgi?id=305206
Not originally my patch, may fall into one of your problematic
categories, definitely needs significant additional work - but without
any feedback for over two years, I'm not motivated to do any of that work.

https://bugzilla.mozilla.org/show_bug.cgi?id=230675
Also not originally my patch, and may not even be a good idea, but
illustrates the difficulty of finding anyone other than Boris who will
admit to being qualified to review design-level changes to Necko.

> * [patches] from new contributors who need much more
> careful review and mentoring

So I mostly don't have the authority to approve patches, but I *do* have
the knowledge to mentor new contributors through getting all the rough
edges off their first several patches. I would be happy to take on some
of that effort, and I bet there are lots of other people who are also
able and willing. Would it help to have a formal system for delegating
that kind of patch review, or at least an informal (but written down)
list of people who can do that?

> * patches that redesign a subsystem without previous discussion
> with the other maintainers of the code (of, e.g., what the
> constraints that led to the current design are or what other
> changes need to happen soon in that area of code), in other
> words, where the review request essentially requests either
> rubber-stamping of the new design, or that a replacement design
> be offered (which is a lot of work)
>
> * patches that fix the case that the patch author cares about but
> break a lot of others -- in other words, patches where the author
> has done the first 20% of the work, is asking me to do the next
> 50%, and is quite willing to do the typing at the end to finish
> the last 30%

I think that when a patch falls into one of these categories, reviewers
should say so quickly and r-, but then offer a way forward for the
author. In the first case, that could be something like "This redesign
doesn't account for X, Y, Z constraints that may not have been obvious
to you, and might interfere with A, B, C future plans in this area.
Bugzilla is an awkward place to discuss redesigns; let's start a
conversation on dev.platform (link to instructions for use of
dev.platform)." In the second case, "This patch fixes this bug but
introduces a whole bunch of other bugs -- please do the next 50% of the
work" seems like a reasonable thing to ask.

People don't like being rejected, but they *really* don't like being
ignored.

> Perhaps what would help the most is if anything that risks falling
> into the latter two categories had an explicit design review stage
> sometime before a 100K patch appears in somebody's review queue.

I've found it difficult to get any traction on bugs that ask for design
(or review of design) ahead of there being any code to review - see for
instance https://bugzilla.mozilla.org/show_bug.cgi?id=650966 which is a
follow-up from an in-person discussion that was very productive, but
despite that, has gone precisely nowhere.

Maybe that's also something we need new process for.

zw

Robert O'Callahan

unread,
Aug 11, 2011, 6:17:14 PM8/11/11
to Zack Weinberg, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 9:41 AM, Zack Weinberg <za...@panix.com> wrote:

> My experience with other module owners has not been so good. To pick four
> examples not at random:
>

Thanks for turning the discussion to specifics!


> https://bugzilla.mozilla.org/show_bug.cgi?id=563192
> Review requested from, AFAIK, the *only person* who can review changes to
> NSPR; no reaction whatsoever for over a year. (Have I mentioned recently
> that we need to get rid of NSPR?)
>

Fork NSPR and make khuey own it!


> https://bugzilla.mozilla.org/show_bug.cgi?id=643041
> Super-reviewer asked a third person for additional review; no reaction from
> that third person for five months. Super-reviewer also raised some fuzzy
> concerns; I asked some questions in response; no reply for five months.
>
https://bugzilla.mozilla.org/show_bug.cgi?id=305206
>
Not originally my patch, may fall into one of your problematic categories,
> definitely needs significant additional work - but without any feedback for
> over two years, I'm not motivated to do any of that work.
>

Send a nag, if no response escalate to manager.

https://bugzilla.mozilla.org/show_bug.cgi?id=230675
> Also not originally my patch, and may not even be a good idea, but
> illustrates the difficulty of finding anyone other than Boris who will admit
> to being qualified to review design-level changes to Necko.
>

That should change now that an actual Necko team exists.

If these reviewers were nagged, then they're being irresponsible (except
maybe for NSPR which is just a black hole). (Not condemning those
individuals; I'm guilty too.) I doubt we can fix that with process tweaks.
It would be good to know the reviewer's side of the story.


> * [patches] from new contributors who need much more
>> careful review and mentoring
>>
>
> So I mostly don't have the authority to approve patches, but I *do* have
> the knowledge to mentor new contributors through getting all the rough edges
> off their first several patches. I would be happy to take on some of that
> effort, and I bet there are lots of other people who are also able and
> willing. Would it help to have a formal system for delegating that kind of
> patch review, or at least an informal (but written down) list of people who
> can do that?


I think it would be great if people like you could curate the review queue
by looking for stale review requests, especially from people you don't
recognize, figuring out what needs to be done (nag, polish, reassign, review
it yourself, etc), and doing it?


> Perhaps what would help the most is if anything that risks falling
>> into the latter two categories had an explicit design review stage
>> sometime before a 100K patch appears in somebody's review queue.
>>
>
> I've found it difficult to get any traction on bugs that ask for design (or
> review of design) ahead of there being any code to review - see for instance

> https://bugzilla.mozilla.org/**show_bug.cgi?id=650966<https://bugzilla.mozilla.org/show_bug.cgi?id=650966>which is a follow-up from an in-person discussion that was very productive,


> but despite that, has gone precisely nowhere.
>

There's no actual design in that bug. From the outside it's not clear to me
what the next step in that bug was supposed to be.

I think a post to dev.something saying "I am working on a patch that does
XYZ in the following way: ... does anyone have any feedback?" would be
adequate. If there's no feedback, go ahead and do it; if someone pushes back
on the design after that, you have the moral high ground!

"I want to do XYZ, can anyone tell me how it should be done?" is a little
less likely to get feedback but also worth trying if needed.

Taras Glek

unread,
Aug 11, 2011, 6:30:10 PM8/11/11
to Philip Chee
On 08/11/2011 11:31 AM, Philip Chee wrote:
> On Wed, 10 Aug 2011 14:33:18 -0700, Taras Glek wrote:
>> Hi,
>> I find that a lot of contributors(myself included) get discouraged due
>> to having their patches rot in the review queue.
>> It's not fair to the contributors have a patch rot without any feedback.
>> It's also not great to r+ patch a month after it was submitted.
>>
>> I propose a two step solution to this
>>
>> 1) Every bugzilla account should have bugzilla
>> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
>> on to remind reviewers about outstanding r? requests.
>>
>> 2) Institute a policy where reviewers have to reassign r? to someone
>> else if they know they wont be able to review something within a week.
>> It might turn into a game of hot potato, or it might end up with r?
>> flags being removed from attachments. I think that's better than quietly
>> ignoring the problem.
>>
>> I realize that some people do a lot more reviews than others, but
>> letting patches languish in the queue doesn't make those queues smaller.
>> We should have more fallback reviewers instead.
>
> How would you deal with people like, um, timeless, with review requests
> going back to 2004.

I would declare bankruptcy and unset r? flags from those patches. There
is no point in pretending that patches will get reviewed.

Taras

Mark Banner

unread,
Aug 11, 2011, 6:34:24 PM8/11/11
to
On 11/08/2011 19:34, L. David Baron wrote:
> On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote:
>> I'm not sure that would help; it's been my experience (particularly
>> in dusty corners of gecko core) that the module owners are the
>> people who don't do their reviews.
>
> Yeah, that's probably me. I've been spending as much time as I can
> on reviews lately (it's my primary job right now; I basically
> haven't been writing code), but there's also a limit to the number
> of hours a day I can spend doing code reviews and maintain my
> sanity.

So you probably have already thought about this, but is there one or
more people working in your areas that you could see if they want to do
reviews? Maybe just throw some small patches in their direction and see
how they cope?

This isn't easy to apply everywhere, as we may not have people in
appropriate areas, but it is something I'm thinking about very frequently.

Standard8

Mark Banner

unread,
Aug 11, 2011, 6:35:45 PM8/11/11
to
On 11/08/2011 17:51, Brad Lassey wrote:
> I was recently told of a company that escalates any expense report that
> hasn't been approved in 24 hours to the next level of management up. It
> might be helpful to something similar in bugzilla. If a review hasn't
> been processed in say a week, bugzilla could automatically re-assign it
> to the module owner (based on the product and component).

I don't think it should have to be re-assigned to the module owner, I
think they should be the ones actively triaging/keeping an eye on the
lists in their module.

Standard8

Zack Weinberg

unread,
Aug 11, 2011, 6:55:29 PM8/11/11
to
On 2011-08-11 3:17 PM, Robert O'Callahan wrote:
> On Fri, Aug 12, 2011 at 9:41 AM, Zack Weinberg<za...@panix.com> wrote:
>> https://bugzilla.mozilla.org/show_bug.cgi?id=563192
>> Review requested from, AFAIK, the *only person* who can review changes to
>> NSPR; no reaction whatsoever for over a year. (Have I mentioned recently
>> that we need to get rid of NSPR?)
>
> Fork NSPR and make khuey own it!

Can we start with a C++0x standard library instead, please? ;-)

>> https://bugzilla.mozilla.org/show_bug.cgi?id=305206
>> Not originally my patch, may fall into one of your problematic categories,
>> definitely needs significant additional work - but without any feedback for
>> over two years, I'm not motivated to do any of that work.
>
> Send a nag, if no response escalate to manager.

I hear you, but I am generally reluctant to nag people whose
overcommitment level I don't know, and even more reluctant to nag
people's managers. (And honestly, I don't know how to find out who any
given person's manager is, other than the org chart on ldap.m.o which is
inaccessible to non-MoCo employees.)

I bring this up not to whine but to illustrate why, IMO, nagging
reviewers doesn't seem like something casual contributors should have to do.

>> [...] difficulty of finding anyone other than Boris who will admit


>> to being qualified to review design-level changes to Necko.
>
> That should change now that an actual Necko team exists.

This is good to hear.

> If these reviewers were nagged, then they're being irresponsible (except
> maybe for NSPR which is just a black hole). (Not condemning those
> individuals; I'm guilty too.) I doubt we can fix that with process tweaks.
> It would be good to know the reviewer's side of the story.

I would like to hear it too.

> I think it would be great if people like you could curate the review queue
> by looking for stale review requests, especially from people you don't
> recognize, figuring out what needs to be done (nag, polish, reassign, review
> it yourself, etc), and doing it?

I don't have a lot of free time until approximately the end of the
month, but after that I will try this and report on how it went.

>> I've found it difficult to get any traction on bugs that ask for design (or
>> review of design) ahead of there being any code to review - see for instance
>> https://bugzilla.mozilla.org/**show_bug.cgi?id=650966<https://bugzilla.mozilla.org/show_bug.cgi?id=650966>which is a follow-up from an in-person discussion that was very productive,
>> but despite that, has gone precisely nowhere.
>
> There's no actual design in that bug. From the outside it's not clear to me
> what the next step in that bug was supposed to be.

That was maybe not a great example. The next step I expected was for
Limi to write up a description of what print preview should be like, and
then we could have gone from there working out the technical side.

zw

Kyle Huey

unread,
Aug 11, 2011, 7:24:42 PM8/11/11
to rob...@ocallahan.org, dev-pl...@lists.mozilla.org, Zack Weinberg
On Thu, Aug 11, 2011 at 6:17 PM, Robert O'Callahan <rob...@ocallahan.org>wrote:

>
> > https://bugzilla.mozilla.org/show_bug.cgi?id=563192
> > Review requested from, AFAIK, the *only person* who can review changes to
> > NSPR; no reaction whatsoever for over a year. (Have I mentioned recently
> > that we need to get rid of NSPR?)
> >
>

> Fork NSPR and make khuey own it!
>

Wait, what!?!

- Kyle

Robert O'Callahan

unread,
Aug 11, 2011, 7:44:36 PM8/11/11
to Kyle Huey, dev-pl...@lists.mozilla.org, Zack Weinberg
On Fri, Aug 12, 2011 at 11:24 AM, Kyle Huey <m...@kylehuey.com> wrote:

> On Thu, Aug 11, 2011 at 6:17 PM, Robert O'Callahan <rob...@ocallahan.org>wrote:
>
>>

>> > https://bugzilla.mozilla.org/show_bug.cgi?id=563192
>> > Review requested from, AFAIK, the *only person* who can review changes
>> to
>> > NSPR; no reaction whatsoever for over a year. (Have I mentioned
>> recently
>> > that we need to get rid of NSPR?)
>> >
>>

>> Fork NSPR and make khuey own it!
>>
>
> Wait, what!?!
>

I was joking. Well, half-joking.

Reducing NSPR usage and replacing it with C++0x stuff sounds great. Maybe
mwu should own that :-).

Robert O'Callahan

unread,
Aug 11, 2011, 7:50:54 PM8/11/11
to Zack Weinberg, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 10:55 AM, Zack Weinberg <za...@panix.com> wrote:

> I hear you, but I am generally reluctant to nag people whose overcommitment
> level I don't know, and even more reluctant to nag people's managers.


That is largely a matter of phrasing the nag email appropriately. E.g. phase
1 can be

"I realize you're very busy, but this patch has been waiting for review for
a while; can you give me an estimate of when you'll be able to review it, or
recommend another reviewer? Thanks."

(And honestly, I don't know how to find out who any given person's manager
> is, other than the org chart on ldap.m.o which is inaccessible to non-MoCo
> employees.)
>

Ask on IRC or ask someone else you know.

I bring this up not to whine but to illustrate why, IMO, nagging reviewers
> doesn't seem like something casual contributors should have to do.
>

Of course, no-one should have to deal with slow reviewers and casual
contributors are going to find it hard to do. That makes it especially
important that those of us more experienced and connected contributors, who
are more able to "encourage" people into good habits, actually do so.


> by looking for stale review requests, especially from people you don't
>> recognize, figuring out what needs to be done (nag, polish, reassign,
>> review
>> it yourself, etc), and doing it?
>>
>
> I think it would be great if people like you could curate the review queue
> I don't have a lot of free time until approximately the end of the month,
> but after that I will try this and report on how it went.
>

Sounds great.

Taras Glek

unread,
Aug 12, 2011, 1:17:29 PM8/12/11
to
On 08/10/2011 02:33 PM, Taras Glek wrote:
> Hi,
> I find that a lot of contributors(myself included) get discouraged due
> to having their patches rot in the review queue.
> It's not fair to the contributors have a patch rot without any feedback.
> It's also not great to r+ patch a month after it was submitted.
>
> I propose a two step solution to this
>
> 1) Every bugzilla account should have bugzilla
> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
> on to remind reviewers about outstanding r? requests.
>
> 2) Institute a policy where reviewers have to reassign r? to someone
> else if they know they wont be able to review something within a week.
> It might turn into a game of hot potato, or it might end up with r?
> flags being removed from attachments. I think that's better than quietly
> ignoring the problem.
>
> I realize that some people do a lot more reviews than others, but
> letting patches languish in the queue doesn't make those queues smaller.
> We should have more fallback reviewers instead.
>
>
> Taras

Thanks for all the feedback on this.

I was too shy to state above that I would like to see 24hour review
turn-around times. It's even more ridiculous in our faster release cycle
if a patch takes half(or more) of the cycle doing nothing in the review
queue.

As far as I can tell there are 3 main reasons(*) that lead to long
review times

1) People like gavin, bz, dbaron having disproportionally high review
loads. We need a process to handoff patches to other reviewers here. A
mailing list, rss feed?
Suggestions?

2) Bugzilla-fobic people(like myself) loosing track of bugzilla r?
requests due to not having bugzilla whines setup.
Is anyone opposed to having bugzilla r? whines setup by default (either
for new accounts or all of existing active accounts)

3) Bad review habits.
I've met a number of MoCo developers that like to batch their reviews up
and then do them all on a single weekday. Please stop, you are killing
all kinds of coding momentum/fun/etc. Lets make it our policy to set
aside time every day to clear the review queue.
Clearly people with existing backlogs will take a while to catch up, but
most MoCo employees should be capable of this.
Are there any good reasons against daily reviews?

I think 24-hours should be our ideal turnaround time and if reviews sit
for a week without signs of progress, managers should get automatically
involved.


Taras

* There are also other reasons (eg community reviewers have limited
times when they can contribute, etc)

Mike Shaver

unread,
Aug 12, 2011, 1:24:45 PM8/12/11
to rob...@ocallahan.org, dev-pl...@lists.mozilla.org, Zack Weinberg, Brendan Eich
On Thu, Aug 11, 2011 at 6:17 PM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> Send a nag, if no response escalate to manager.

I don't think that's an appropriate path. It doesn't handle cases of
ownership by non-employees (we have some, and could certainly stand to
have more), and it entangles inconstant Corporation org-chart
artifacts into project governance.

Sub-module issues should be escalated to the broader module owner (me
for Firefox, f.e.). If it's a top-level module then it should be
escalated to either Brendan (as he's asked in the past) or the module
ownership group.

Mike

Mike Shaver

unread,
Aug 12, 2011, 1:26:08 PM8/12/11
to Taras Glek, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 1:17 PM, Taras Glek <tg...@mozilla.com> wrote:
> As far as I can tell there are 3 main reasons(*) that lead to long review
> times
>
> 1) People like gavin, bz, dbaron having disproportionally high review loads.
>
> 2) Bugzilla-fobic people(like myself) loosing track of bugzilla r? requests
> due to not having bugzilla whines setup.
>
> 3) Bad review habits.

4) patches that are too huge or confusing to review in a single
session; see dbaron's comments earlier in this thread.

They need to be bounced more aggressively, IMO.

Mike

David Humphrey

unread,
Aug 12, 2011, 1:35:16 PM8/12/11
to Taras Glek, dev-pl...@lists.mozilla.org
> I was too shy to state above that I would like to see 24hour review
> turn-around times. It's even more ridiculous in our faster release cycle
> if a patch takes half(or more) of the cycle doing nothing in the review
> queue.

Some of the projects on which I work have daily review built into the
culture. Whether or not that could work for Mozilla-proper (I think it
could), I'll tell you that my experience with it has been really good.

One of the things I've noticed with daily review is that contributors
who aren't full-time on the code tend to stay around more, since their
stuff gets looked at more quickly. It also reduces bit rot, which is
often more than just annoying. Another thing I've noticed is that when
there is a culture of daily review, the discussions within the community
tends to be about reviewing, which is helpful because it helps people
learn how to do it by watching the experts at work. Reviewing and
coding aren't the same thing, and learning to do both is key.

This thread has caused a few patches I wrote last year to suddenly get a
review, which is good; but it's also bad because I've since moved on
from that part of Mozilla's code, in part because there was no point
working on things people didn't seem to care enough about to review.
I'm not going to quit Mozilla over it, but I wonder if some people will,
and have.

It might be interesting to ask which barriers are in the way of a daily
review culture for Mozilla. If bz says he's already doing 50% of his
time on reviews, it sounds like some people do it now. Maybe more could...

Dave

fantasai

unread,
Aug 12, 2011, 1:45:19 PM8/12/11
to
On 08/12/2011 10:17 AM, Taras Glek wrote:
> I was too shy to state above that I would like to see 24hour review turn-around times.
> It's even more ridiculous in our faster release cycle if a patch takes half(or more)
> of the cycle doing nothing in the review queue.

The intent of the new release cycle is to get things that are done out faster.
It's not about compressing the time it takes to get things done.

I think expecting a 24hour review turn-around is counter-productive to expecting
a good quality review and having not-stressed-out reviewers. I'd rather have
quality over speed. It saves time in the long run.

> 3) Bad review habits.
> I've met a number of MoCo developers that like to batch their reviews up and then do
> them all on a single weekday. Please stop, you are killing all kinds of coding
> momentum/fun/etc. Lets make it our policy to set aside time every day to clear the
> review queue. Clearly people with existing backlogs will take a while to catch up,
> but most MoCo employees should be capable of this. Are there any good reasons against
> daily reviews?

Context switching costs? I imagine that's the reason most people batch things.
I don't think batching reviews up is a bad habit. Not getting to them regularly
is a bad habit, and not communicating to the patch contributor how long it's
going to take, if it's going to take longer than a week, is discourteous.

~fantasai

Robert Kaiser

unread,
Aug 12, 2011, 1:56:16 PM8/12/11
to
Taras Glek schrieb:

> I think 24-hours should be our ideal turnaround time

Ideally, yes, but you can't expect everyone to have time set aside for
reviews in every work day, let alone non-work days, and you can't expect
everyone to be paid for doing reviews or even the work on Mozilla code.

Taras Glek

unread,
Aug 12, 2011, 2:07:02 PM8/12/11
to fantasai
On 08/12/2011 10:45 AM, fantasai wrote:
> On 08/12/2011 10:17 AM, Taras Glek wrote:
>> I was too shy to state above that I would like to see 24hour review
>> turn-around times.
>> It's even more ridiculous in our faster release cycle if a patch takes
>> half(or more)
>> of the cycle doing nothing in the review queue.
>
> The intent of the new release cycle is to get things that are done out
> faster.
> It's not about compressing the time it takes to get things done.

I think it highlights an old problem.

>
> I think expecting a 24hour review turn-around is counter-productive to
> expecting
> a good quality review and having not-stressed-out reviewers. I'd rather
> have
> quality over speed. It saves time in the long run.

I'm not saying people should r+ things as quickly as possible. It's one
thing to minimize the amount of time that the patch spends idling in the
queue and quite another to minimize the amount of time spent reviewing
the patch.

>
>> 3) Bad review habits.
>> I've met a number of MoCo developers that like to batch their reviews
>> up and then do
>> them all on a single weekday. Please stop, you are killing all kinds
>> of coding
>> momentum/fun/etc. Lets make it our policy to set aside time every day
>> to clear the
>> review queue. Clearly people with existing backlogs will take a while
>> to catch up,
>> but most MoCo employees should be capable of this. Are there any good
>> reasons against
>> daily reviews?
>
> Context switching costs? I imagine that's the reason most people batch
> things.
> I don't think batching reviews up is a bad habit. Not getting to them
> regularly
> is a bad habit, and not communicating to the patch contributor how long
> it's
> going to take, if it's going to take longer than a week, is discourteous.

Let me flip this around. What about the context-switching costs for the
person who spent the time writing the patch? Having your patch r+ed
late, having to catch up to code changes, rebase, etc is a lot worse
than "loosing" a few minutes due to batching your reviews daily.

If it's a crap patch where the person clearly didn't put in the time to
make it as easy as possible to review, r- it and move on.

Taras

Mike Shaver

unread,
Aug 12, 2011, 2:13:17 PM8/12/11
to Robert Kaiser, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 1:56 PM, Robert Kaiser <ka...@kairo.at> wrote:
> Taras Glek schrieb:
>>
>> I think 24-hours should be our ideal turnaround time
>
> Ideally, yes, but you can't expect everyone to have time set aside for
> reviews in every work day, let alone non-work days, and you can't expect
> everyone to be paid for doing reviews or even the work on Mozilla code.

Not all reviews need 24-hour turnaround, either. Some pieces of data
that might help reviewers prioritize their reviews to get the fast
ones fast but schedule the less time-sensitive ones more comfortably:

- how many different bugs has this patch's author attached patches to?
how long have they been doing so?

- if this isn't the 1st patch on this bug, what were the previous
latencies for patch->review and review->patch? If people aren't
revving the patches for 2 weeks after a quick review, it's probably
not as critical that they be fast.

- what did the requester say the urgency was? I don't always need
patches quickly, so I would be happy to say "within 2 weeks is fine",
where others might say "tomorrow if possible"

Mike

L. David Baron

unread,
Aug 12, 2011, 2:36:08 PM8/12/11
to dev-pl...@lists.mozilla.org
On Friday 2011-08-12 13:26 -0400, Mike Shaver wrote:
> On Fri, Aug 12, 2011 at 1:17 PM, Taras Glek <tg...@mozilla.com> wrote:
> > As far as I can tell there are 3 main reasons(*) that lead to long review
> > times
> >
> > 1) People like gavin, bz, dbaron having disproportionally high review loads.
> >
> > 2) Bugzilla-fobic people(like myself) loosing track of bugzilla r? requests
> > due to not having bugzilla whines setup.
> >
> > 3) Bad review habits.
>
> 4) patches that are too huge or confusing to review in a single
> session; see dbaron's comments earlier in this thread.
>
> They need to be bounced more aggressively, IMO.

A few other thoughts:

Most of the times that I've explicitly tried to distribute review
load to newer contributors who I think are qualified to review the
patches, the person I've tried to distribute it to has declined, for
reasons such as "I don't know that code well enough". I think it
would be useful to make it harder for regular contributors who the
module owner thinks can review the patch to decline on that basis; I
probably should have applied more pressure in these cases, or
probably just not asked permission at all (which, after all, is the
way I end up with reviews). In many cases, most of the time I spend
doing a review is time spent learning or re-learning about the code
that it's modifying; newer contributors might have a slight
disadvantage there, but not a huge one.


In terms of distributing review load, I think there are three sets
of patches:
* patches I feel I need to review myself
* patches I'd be happy for others to review entirely
* patches where I'd like to review a description of what the patch
is changing but I'd be happy for others to review the code

The third category brings up another issue, which is that people
often don't describe what their patches are trying to do. I'm
becoming more aggressive about minusing patches that do this. If I
can't figure out in the first sixty seconds of looking at a patch
what it is intending to change about our behavior, I will minus the
review request on that basis alone without going further (with
leniency for new contributors). This information belongs in the
commit message (which should be in the patch when review is
requested), and in many cases the commit message should follow the
standard multiline commit message format where the first line
contains a short summary (unwrapped, because most Mercurial UI shows
only the first line) and then the remaining (properly wrapped) lines
have details. I'd note that the amount of description needed is not
proportional to the size of the patch: "implement 'foo' CSS
property" is quite sufficient, whereas other patches really do need
descriptions like these:
https://hg.mozilla.org/mozilla-central/rev/830111e10951
https://hg.mozilla.org/mozilla-central/rev/225a79ce27bc
(However, in many cases details belong not in the commit message but
in code comments; it's fine, and in fact often better, for the
commit message to point to *specific* comments introduced or
modified in the patch.)

Mike Shaver

unread,
Aug 12, 2011, 3:00:41 PM8/12/11
to L. David Baron, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 2:36 PM, L. David Baron <dba...@dbaron.org> wrote:
> This information belongs in the
> commit message (which should be in the patch when review is
> requested), and in many cases the commit message should follow the
> standard multiline commit message format where the first line
> contains a short summary (unwrapped, because most Mercurial UI shows
> only the first line)

We need bugzilla to give people this guidance when they attach, if not
check it client side and offer warnings. ("This isn't a patch with
commit, but it's more than 100 lines!")

Let's make a list:

> https://hg.mozilla.org/mozilla-central/rev/830111e10951
> https://hg.mozilla.org/mozilla-central/rev/225a79ce27bc
> (However, in many cases details belong not in the commit message but
> in code comments; it's fine, and in fact often better, for the
> commit message to point to *specific* comments introduced or
> modified in the patch.)

Long lists of things in commit messages often point to things that
should have been split into different patches, IME.

Mike

Mike Shaver

unread,
Aug 12, 2011, 3:01:03 PM8/12/11
to L. David Baron, dev-pl...@lists.mozilla.org
On Fri, Aug 12, 2011 at 3:00 PM, Mike Shaver <mike....@gmail.com> wrote:
> Let's make a list:

Ahem.

Let's make a list: https://wiki.mozilla.org/Bugzilla/Patch_Submitter_Feedback

Mike

Steve Fink

unread,
Aug 12, 2011, 4:35:42 PM8/12/11
to L. David Baron, dev-pl...@lists.mozilla.org
What's a review?

No, seriously, what is a review supposed to accomplish? I'm not
convinced that everyone has a shared understanding of what reviews are
for, and I'm *certain* that new reviewers are unclear on what is
expected of them. Or more precisely, what giving an r+ is supposed to mean.

There's a huge range of possibilities, and I've seen reviews range
across the spectrum from "nothing here is obviously wrong" to "I applied
your patch and measured the cache misses with 150 different web sites
and maybe you should consider using an iterator pattern here and I think
this might introduce a race condition once we switch to e10s..."

What sort of review it is seems to depend on the reviewer's opinions
about what a review ought to mean, the area that the code is modifying,
the impact of the code in question, the reputation of the author, and
the length of the reviewer's review queue, among many other things.

I suspect some people take reviews much more seriously than others in
this thread, and that's where some of the discrepancy in opinions about
reasonable timeframes comes from.

Without a more specific agreement about what reviews mean, I don't see
how we can agree on an appropriate timeframe for reviews. "Within 24
hours" seems reasonable if you're looking at only the patch itself and
considering mainly cosmetic issues, or if you are intimately familiar
with the area that is being modified. (In past lives when working with
no more than half a dozen other developers on a recent codebase, I'd
just subscribe to a feed of all changes, read every one, and give
feedback after they had already landed. It worked great, and I could go
through half a dozen patches in a few minutes.) 24 hours is way too
little if I'm certifying that the patch won't cause major performance
regressions, works for its intended purpose, is free of race conditions,
uses the best APIs available, and is in an unfamiliar area of the code
to begin with. Trying to reach a 24-hour target for most reviews is
implicitly declaring that reviews are generally intended to be fairly
superficial; do we want that? (It's an honest question; I'm struggling
to decide how much effort I put into each review. Though I have the
liberty of making that decision mainly because I'm an infrequent target
of review requests.)

Reviews also vary in terms of who is more of an expert on the subject
matter: the reviewer or the reviewee. Reviews are often teaching
exercises, and most of the learning flows one way or the other.

Can someone tell me what I am agreeing to by giving my r+?

- I don't believe this patch will do harm
- This patch is the best possible way I can envision of achieving its
purpose
- If this patch causes any problems, whether correctness or perf or
memory usage, it's my fault
- The patched code reads as if it were all written by a single person
- ...?

I've given and received reviews of various flavors. I've sometimes
posted two patches for review and asked the reviewer to pick the
preferred approach and r- the other. Is that an abuse of the review
system? I have gotten a couple of r+'s that weren't deserved. Is that
bad, or is that a necessary degree of flexibility to keep things moving?
I have a sense for who's going to give me an intense scrutiny and who
will probably r+ at a quick glance, and target my review requests
accordingly. (Not so much to maximize my chances of an r+, but more to
try to balance review load because I only want to take up the thorough
reviewers' time on things that matter and where I feel the patch needs
the feedback.) Should we strive to make everyone's review mean exactly
the same thing?

r? might mean "please rubberstamp this" or "is this the right way to do
it?" or "do you think this is a good idea?" r+ might mean "looks great!"
or "doesn't matter, don't care, it can't break too much" or "this'll
work -- it's not the best way, but I don't have the time or inclination
to do it right at the moment". We attempt to convey more in the
comments, but concern for reviewee's feelings, the overhead of opening
followup bugs, and the difficulty of precisely conveying
design/implementation reactions and advice all lead to nuances being
lost in a blanket r+/r-.

Steve Fink

unread,
Aug 12, 2011, 4:35:42 PM8/12/11
to L. David Baron, dev-pl...@lists.mozilla.org

Mike Shaver

unread,
Aug 12, 2011, 4:46:57 PM8/12/11
to Steve Fink, L. David Baron, dev-pl...@lists.mozilla.org
IMO, it should be:

>  - I don't believe this patch will do harm

> - If this patch causes any problems, whether correctness or perf or memory
> usage, it's my fault

Where by "fault" I mostly mean "I will assist in making it right".

>  - This patch is the best possible way I can envision of achieving its
> purpose

>  - The patched code reads as if it were all written by a single person

I don't think these are necessarily barriers, though I've worked for a
long time in a module where they were both part of usual rigor. I
think the delay and energy cost (and slightly-adversarial
presentation) outweighed the usually tiny improvements to
implementation, many of which could have been follow-up material.

That's how I'd like to see Firefox reviews done, at least, and that's
the only area in which I really have a say.

Mike

Nicholas Nethercote

unread,
Aug 12, 2011, 6:12:30 PM8/12/11
to Steve Fink, L. David Baron, dev-pl...@lists.mozilla.org
On Sat, Aug 13, 2011 at 6:35 AM, Steve Fink <sf...@mozilla.com> wrote:
>
> No, seriously, what is a review supposed to accomplish? I'm not convinced
> that everyone has a shared understanding of what reviews are for, and I'm
> *certain* that new reviewers are unclear on what is expected of them. Or
> more precisely, what giving an r+ is supposed to mean.

Really? I think "common sense" is the answer to most of your
questions. You can try to codify this stuff, but people are going to
do what they're going to do. Different people will have different
standards, that's inevitable. For example, I'm hopeless at pointing
out minor style variations, I don't care about them/notice them enough
to point them out. Other people (e.g. Brendan, Waldo) are amazing at
this.

I think 24-hour turn-around is a reasonable aim for full-time MoCo
employees at the least. At least, for people like me who spend maybe
5% of their time reviewing; it's different for the 50%-ers.
(Actually, what is the review distribution like? How many people are
like me in the 5% zone, and how many are like bz in the 50% zone?) If
I know I'll take more than 2 or 3 days I try to say so in the bug. I
also have a tendency to ask reviews of people that I know are fast,
and avoid ones that I know are slow.

Detailed descriptions of patches are really helpful too, I always try
to do that.

In terms of reviewer responsibility for flaws in a patch, I personally
don't think there's much. If I landed a patch that had bad effects, I
would not blame the reviewer for failing to catch something, unless I
had specifically asked them something that I expected them to know.
(E.g. if I ask "Mr race expert, will line X have a race?" and they say
"definitely not" and then it has a race.)

Nick

L. David Baron

unread,
Aug 12, 2011, 9:48:53 PM8/12/11
to Steve Fink, dev-pl...@lists.mozilla.org
On Friday 2011-08-12 13:35 -0700, Steve Fink wrote:
> What's a review?
>
> No, seriously, what is a review supposed to accomplish? I'm not
> convinced that everyone has a shared understanding of what reviews
> are for, and I'm *certain* that new reviewers are unclear on what is
> expected of them. Or more precisely, what giving an r+ is supposed
> to mean.

So, for me, this is very context-dependent, where context is both
the person who's writing the code (and what I know of their
abilities and expertise) and the code being changed.

I think the most important thing to review is whether the proposed
change is a change we want to make: that is, for a bug fix, is the
thing it's fixing actually a bug? (The other option is that the
thing it's fixing was actually correct, but there's something else
leading to a problem.) This depends on knowledge of relevant
standards or of the code design. Doing this generally involves
understanding what the patch is trying to do. (In one of my earlier
messages in this thread, I commented than in many cases I'd be happy
to review a description of the patch and leave the details to
somebody else. That's because of exactly this point: I want to
review whether the bug being fixed is actually a bug, but I'm not so
picky about the details.)

But then a lot of reviewing is very contextual. For example, when I
review CSS parser changes, I tend to review the error handling
carefully, because CSS has well-defined (though not necessarily
clearly-defined) error recovery rules that (at least in our parser
architecture) are easy to get wrong and we don't have good tests for
them (though, actually, I just realized I might be able to write a
pretty robust test for them by going through every value in
property_database.js, chopping it at random points in the middle,
inserting various types of garbage, and making sure that the correct
sequence of closing braces/quotes/etc. leads to the parser returning
to the expected state).

When I review new CSS properties or values I look at the items
tested in property_database.js closely to make sure that there's
adequate test coverage, and then I read/skim through the code,
basically looking for anything that seems out of place. (I review a
lot of these, so I have a pretty good sense of what they look like.)

When I review layout code, a lot of what I'm thinking about is
feature interactions -- in other words, following conventions and
invariants correctly so that all the different features in layout
work right together. For example, making sure that not only does
this box get the right size, but does it still get the right size if
it has border or padding (a rather common feature interaction
error)? Or, say, is this code using the correct method for getting
the current font, or is it digging straight into graphics code to do
that and thus bypassing the set of downloadable fonts for the
current page? In other words, is this code calling the right
method, or calling some other method deeper in that bypasses the
code implementing some other feature that it, in reality, does need
to interact with?

Another thing that I review for with newer contributors is pointing
out things that are entirely unnecessary, out-of-place, or typically
done in a simpler way. When there's no tradeoff other than "would
have been faster if you wrote it this way the first time around" I
am much more likely to require changes as a condition of review
(i.e., replace these 15 lines with 5 lines using this function over
here, which will be faster too).

I try to keep an eye out for new code that could have instead been
reuse of existing code (e.g., "this new function looks exactly like
this other function we already have"), though I'm sure I miss plenty
of cases because I don't go searching for such things beyond the
existing code I'm aware of.

Depending on who's writing the code, I may review with different
levels of care for memory management bugs (leaks), crashes, security
bugs, etc.

I almost never apply or test a patch as part of a code review, and I
generally don't review the automated tests in it all that carefully
(if at all), but I often do look at the test coverage in the tests.


But as Nick said, I think this is *very* context-dependent.

Philip Chee

unread,
Aug 13, 2011, 5:59:40 AM8/13/11
to
On Fri, 12 Aug 2011 13:35:42 -0700, Steve Fink wrote:

> r? might mean [....] or "is this the right way to do

> it?" or "do you think this is a good idea?"

Surely the feedback? flag was implemented for exactly this particular
purpose? Aren't people using this?

Phil

--
Philip Chee <phi...@aleytys.pc.my>, <phili...@gmail.com>
http://flashblock.mozdev.org/ http://xsidebar.mozdev.org
Guard us from the she-wolf and the wolf, and guard us from the thief,
oh Night, and so be good for us to pass.

Robert O'Callahan

unread,
Aug 13, 2011, 8:48:57 AM8/13/11
to Steve Fink, L. David Baron, dev-pl...@lists.mozilla.org
On Sat, Aug 13, 2011 at 8:35 AM, Steve Fink <sf...@mozilla.com> wrote:

> 24 hours is way too little if I'm certifying that the patch won't cause
> major performance regressions, works for its intended purpose, is free of
> race conditions, uses the best APIs available, and is in an unfamiliar area
> of the code to begin with. Trying to reach a 24-hour target for most reviews
> is implicitly declaring that reviews are generally intended to be fairly
> superficial; do we want that?
>

No.

A "24-hour review expectation" could not be an expectation that no patch
will take more than 24 hours (more like 8 hours, I guess) to review. That is
impossible.

I think a more reasonable expectation, for someone who works full-time on
Mozilla, would be that reviews pending at the beginning of a work day are
done by the end of the day, unless one simply runs out of time due to
higher-priority work (which is a narrow category, in my view) or because the
reviews simply took too long.

A very big point here is that trying to do your reviews every day shouldn't
mean you spend more time doing reviews (except for context-switching costs).
You should end up doing the same amount of review work (unless you're
shedding load by delaying reviews until the reviewee switches the review
request or gives up in disgust ... which is a very poor way to shed load).
However, I expect reviewees, QA, and other people to end up doing less work.

Robert O'Callahan

unread,
Aug 13, 2011, 8:56:38 AM8/13/11
to fantasai, dev-pl...@lists.mozilla.org
On Sat, Aug 13, 2011 at 5:45 AM, fantasai <fantasa...@inkedblade.net>wrote:

>
> 3) Bad review habits.
>> I've met a number of MoCo developers that like to batch their reviews up
>> and then do
>> them all on a single weekday. Please stop, you are killing all kinds of
>> coding
>> momentum/fun/etc. Lets make it our policy to set aside time every day to
>> clear the
>> review queue. Clearly people with existing backlogs will take a while to
>> catch up,
>> but most MoCo employees should be capable of this. Are there any good
>> reasons against
>> daily reviews?
>>
>
> Context switching costs? I imagine that's the reason most people batch
> things.
> I don't think batching reviews up is a bad habit. Not getting to them
> regularly
> is a bad habit, and not communicating to the patch contributor how long
> it's
> going to take, if it's going to take longer than a week, is discourteous.
>

I agree with Taras that batching things up weekly is undesirable.

Most humans have forced context switches at least once a day. You can
probably do reviews next to those to avoid extra costs.

Gervase Markham

unread,
Aug 15, 2011, 7:39:55 AM8/15/11
to
On 11/08/11 19:32, Mike Shaver wrote:
> With...
>
> If there were more-suitable module owners around, they'd probably
> already be the module owner, right?

Given the until-recently dysfunction of our technical systems for making
this change, not necessarily. :-| I've happily noticed an uptick in the
information being updated since we moved to a wiki-based system:
https://wiki.mozilla.org/Modules
but it's probably still not where it should be in terms of reflecting
the reality of who actually calls the shots on what.

Gerv

Gervase Markham

unread,
Aug 15, 2011, 7:58:30 AM8/15/11
to
On 10/08/11 22:33, Taras Glek wrote:
> Hi,
> I find that a lot of contributors(myself included) get discouraged due
> to having their patches rot in the review queue.
> It's not fair to the contributors have a patch rot without any feedback.
> It's also not great to r+ patch a month after it was submitted.

You are quite right. Some of this is my fault, in that I've had a
Governance item to do something about this for some time now:
https://wiki.mozilla.org/GovernanceIssues#Triage_Stale_Reviews
but it's never made it to the top of the list such that I've managed to
execute on it properly.

The idea was to map Bugzilla components to modules (and we have a
crowd-sourced spreadsheet which contains some of that info, which would
migrate to the Modules wiki system), and then ask the module owner to do
the triage - either:

- We don't actually want this change
- This is bitrotted; our apologies; please can you renew it?
- I've just reviewed this
- I have assigned this to a better reviewer

The aim was to get on top of the problem, then have a monitoring system
to make sure that we didn't regress. One pre-requisite was the new
Modules wiki system: https://wiki.mozilla.org/Modules , but that's now done.

However, it could be that the prerequisites necessary for this scheme
made it too heavyweight, and that's why nothing ended up getting done. :-|

> I propose a two step solution to this
>
> 1) Every bugzilla account should have bugzilla
> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned
> on to remind reviewers about outstanding r? requests.

This can be done. The relevant query (for "review" only) is:

https://bugzilla.mozilla.org/buglist.cgi?type0-1-0=equals&field0-1-0=requestees.login_name&field0-0-0=flagtypes.name&resolution=---&query_format=advanced&value0-1-0=gerv%40mozilla.org&type0-0-0=equals&value0-0-0=review%3F

We could write a DB-altering script to give everyone a saved search of
this type, and set up a whine for it.

Although I am under the impression that Bugzilla may send a mail even if
the query is empty; we would need to change that!

Gerv

Robert Kaiser

unread,
Aug 15, 2011, 10:23:06 AM8/15/11
to
Steve Fink schrieb:

> Can someone tell me what I am agreeing to by giving my r+?

Someone already stated it's "common sense" what that is - I think it can
best be described you you agreeing that "this is functionality-wise and
quality-wise good to go into the production code". If you don't know
what functionality and quality is needed for the production code of
Mozilla (and/or this code area) or are unable to help someone else get
to that level, you are not a fitting reviewer.

Ehsan Akhgari

unread,
Aug 15, 2011, 2:21:33 PM8/15/11
to Gervase Markham, dev-pl...@lists.mozilla.org
On 11-08-15 7:58 AM, Gervase Markham wrote:
> Although I am under the impression that Bugzilla may send a mail even if
> the query is empty; we would need to change that!

It won't. I have had a whine setup for my own outstanding review
requests for quite a while, and Bugzilla smartly skips sending the email
if the query doesn't produce any results.

Cheers,
Ehsan

Taras Glek

unread,
Aug 15, 2011, 3:21:42 PM8/15/11
to Christian Legnitto
On 08/11/2011 10:34 AM, Christian Legnitto wrote:
>
> On Aug 11, 2011, at 10:26 AM, Taras Glek wrote:
>
>> On 08/10/2011 09:35 PM, Robert O'Callahan wrote:

>>> On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek<tg...@mozilla.com> wrote:
>>>
>>>> I find that a lot of contributors(myself included) get discouraged due to
>>>> having their patches rot in the review queue.
>>>> It's not fair to the contributors have a patch rot without any feedback.
>>>> It's also not great to r+ patch a month after it was submitted.
>>>>
>>>
>>> Are patches accidentally falling through the cracks, or being deliberately
>>> ignored?
>>>
>>> If the latter, I think the only effective approach is to tackle the
>>> delinquent reviewers directly. If one-on-one engagement doesn't work,
>>> escalate.
>>
>> I think it's a bit of both. We need to make it easier to keep track of own review queue and to find people with delinquent reviews.
>>
>> Perhaps someone can cook up a bugzilla search to track stale r? requests and publicize that on some mailing list?
>>
>> "Developer XYZ hasn't reviewed bug #### in a week" -> then someone else could pick up the review.
>>
>
> FWIW I think it would be pretty trivial to write an extension to do the email whine by default (with cc'ing a mailing list or whatever) followed by reassigning the review via a per-module list of prioritized fallback reviewers.

This sounds great.

>
> Bugzilla extension docs are here:
>
> http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Extension.html
>
> And there are staging installs to test during development:
>
> http://landfill.bugzilla.org/
>
> Sounds like a good intern or new contributor project to me (assuming that this is a real problem, I don't see any data presented to back up the assertion yet).

I agree that we should base any policy/tool changes based on solid evidence.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=679080 to get some
stats on this.

What group within Mozilla would a bugzilla intern belong to?

Taras

Shawn Wilsher

unread,
Aug 15, 2011, 11:02:26 PM8/15/11
to dev-pl...@lists.mozilla.org
On 8/12/2011 10:17 AM, Taras Glek wrote:
> I think 24-hours should be our ideal turnaround time and if reviews sit
> for a week without signs of progress, managers should get automatically
> involved.
Unless you can get wide adoption of this, it's just going to hurt the
people that do this. I was shooting for "one business day" turnaround
before I changed jobs. It quickly turned into me doing reviews for
probably 80% of my work day. I was being vocal about doing this
experiment, and I was getting to reviews quickly. As a result, people
would give me more reviews because my queue was either empty (or lower
than other peers), and because they had a previous fast turnaround with me.

Cheers,

Shawn

Shawn Wilsher

unread,
Aug 15, 2011, 11:03:46 PM8/15/11
to dev-pl...@lists.mozilla.org
On 8/12/2011 1:46 PM, Mike Shaver wrote:
> I don't think these are necessarily barriers, though I've worked for a
> long time in a module where they were both part of usual rigor. I
> think the delay and energy cost (and slightly-adversarial
> presentation) outweighed the usually tiny improvements to
> implementation, many of which could have been follow-up material.
I stopped letting a lot of things get fixed in follow-up bugs because I
found that people would file it, and then never touch it because the
issue wasn't important enough. The only way I could get things that the
authors felt were not important was to gate my review on it.

Cheers,

Shawn

Robert O'Callahan

unread,
Aug 15, 2011, 11:39:31 PM8/15/11
to Shawn Wilsher, dev-pl...@lists.mozilla.org

That's a good point.

If someone is spending too much time doing reviews, it's perfectly OK to
reject reviews that someone else can do. It's better to load-balance
explicitly and quickly than to subtly discourage people by being slow.

The ideal is that everyone's being fast so the fast reviewers don't stand
out.

Scott Johnson

unread,
Aug 16, 2011, 6:22:58 PM8/16/11
to dev-pl...@lists.mozilla.org
So, one idea to alleviate some of the review requests that come in is to
focus on creating more reviewers. One thing to accomplish this might be
to add a section 'additional reviewers' to the modules page on the wiki.
In order to become an 'additional reviewer', one would shadow an
existing reviewer. So, for example, an existing reviewer of a module
would receive a review request, and perform the review while another
potential reviewer shadowing them is watching (either online via a
skype/shared desktop call, or if you happen to be in the office with the
person, while you're at their desk). Then, the next review that comes in
could be done in parallel by both the existing reviewer and the
shadower, and they could meet to discuss their findings.

This could be done a few times to "train" new reviewers and familiarize
them with the code and how to review it. It takes a bit of time up
front, but ideally, it would lead to more reviewers over time, and
perhaps better review practices.

Another idea would be to create a wiki page for each of the modules
discussing the most common findings during review, and basic
expectations of a review in a given module (perhaps this might be
something that would be common across all Mozilla code).

~Scott


On 08/16/2011 06:21 AM, thus spoke Ehsan Akhgari:


> On 11-08-15 7:58 AM, Gervase Markham wrote:

>> Although I am under the impression that Bugzilla may send a mail even if
>> the query is empty; we would need to change that!
>

> It won't. I have had a whine setup for my own outstanding review
> requests for quite a while, and Bugzilla smartly skips sending the
> email if the query doesn't produce any results.
>
> Cheers,
> Ehsan

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

Gervase Markham

unread,
Aug 19, 2011, 9:31:22 AM8/19/11
to
On 10/08/11 22:33, Taras Glek wrote:
> I propose a two step solution to this

Following on from Taras' proposal, here's an attempt at an action plan:
https://wiki.mozilla.org/BMO/Review_Queue_Proposal

Comments welcome.

Gerv

Mike Hommey

unread,
Aug 19, 2011, 9:55:45 AM8/19/11
to Gervase Markham, dev-pl...@lists.mozilla.org

Just a comment on this part:

> * Set up a community expectation that all reviews should be addressed
> within a week, or reassigned.

The "or reassigned" part could lead to an endless hot potato game. The
community expectation could be that all reviews are acknowledged within a
certain fixed amount of time, and addressed (that is, end with r+ or r-)
within another fixed amount of time, starting or not from the
acknowledgement.

Mike

Neil

unread,
Aug 19, 2011, 9:58:07 AM8/19/11
to
Gervase Markham wrote:

>On 10/08/11 22:33, Taras Glek wrote:
>
>
>>I propose a two step solution to this
>>
>>
>Following on from Taras' proposal, here's an attempt at an action plan:
>https://wiki.mozilla.org/BMO/Review_Queue_Proposal
>
>

Maybe the message could be tweaked depending on whether the requester or
requestee was last to comment on the bug?

--
Warning: May contain traces of nuts.

Gervase Markham

unread,
Aug 19, 2011, 11:03:24 AM8/19/11
to
On 19/08/11 14:55, Mike Hommey wrote:
> The "or reassigned" part could lead to an endless hot potato game.

It could, but I would expect our developers to be a bit more sensible
than that :-) And if a reporter cries foul or a QA person notices the
foolishness, I'm sure we can get it sorted out.

> The
> community expectation could be that all reviews are acknowledged within a
> certain fixed amount of time, and addressed (that is, end with r+ or r-)
> within another fixed amount of time, starting or not from the
> acknowledgement.

What does the acknowledgement get you? Just a "yes, I'm the right guy
for this; it's in my queue"?

Gerv

Gervase Markham

unread,
Aug 19, 2011, 11:03:49 AM8/19/11
to
On 19/08/11 14:58, Neil wrote:
> Maybe the message could be tweaked depending on whether the requester or
> requestee was last to comment on the bug?

Could you explain that more?

Gerv


Kyle Huey

unread,
Aug 19, 2011, 11:08:48 AM8/19/11
to Gervase Markham, dev-pl...@lists.mozilla.org

Not sure about Neil, but sometimes I receive a review request and then ask
additional questions in the bug to clarify things. The ball is then in the
requestee's court. I could cancel these review requests to avoid the nag
emails, but that seems less than desirable.

- Kyle

Zack Weinberg

unread,
Aug 19, 2011, 11:24:02 AM8/19/11
to
On 2011-08-19 8:08 AM, Kyle Huey wrote:
>
> Not sure about Neil, but sometimes I receive a review request and then ask
> additional questions in the bug to clarify things. The ball is then in the
> requestee's court. I could cancel these review requests to avoid the nag
> emails, but that seems less than desirable.

I think ideally this would be a distinct rX state; I have occasionally
forgotten to respond to reviewers' questions because I missed the
bugmail and from _my_ "My Requests" page it looks like the review is
still pending.

zw

Mike Hommey

unread,
Aug 19, 2011, 11:28:50 AM8/19/11
to Gervase Markham, dev-pl...@lists.mozilla.org

I wouldn't expect the common case, where the right guy has been
requested, to yield this particular ack scenario. I was more thinking
about the "Joe Developer should review this for $reason" type of
interaction.

It just gets two review redirections, which is not that much of a
stretch, to reach 3 weeks under these rules. Which IMHO is just too
long.

Mike

Jesper Kristensen

unread,
Aug 19, 2011, 12:04:18 PM8/19/11
to

I think you should cancel the review request and ask the patch author to
set it again when he has answered the questions. That way you do not
leave confusion on if the ball is on your side or his.

Justin Wood (Callek)

unread,
Aug 19, 2011, 1:28:46 PM8/19/11
to

Whines:

This is retrofitted to every existing account, and added
automatically to new ones.

(Is there a performance problem with running 50,000+ additional queries
a week?)

IMO a whine set on canconfirm or editbugs would be a reasonable
expectation here, and cut out probably at least 1/4 of our bugzilla
users, maybe much more.

The reasoning is, that if a person does not have editbugs/canconfirm
they very very likely are not a reviewer, or even trusted enough to
adhere to these whine e-mails anyway. (we can setup a separate whine for
any "remaining" review requests, such as "to the wind" or to an user
without editbugs/canconfirm somehow, I suspect)

--
~Justin Wood (Callek)

Mook

unread,
Aug 20, 2011, 5:20:19 AM8/20/11
to

Regarding review bankruptcy, what would drive me towards updating my
long rotted patches? Will the expectation be that it will get looked at
this time? Because that either means allocating time for the incoming
review rush, or not doing so an kicking the problem down the road...

Of course, it's probable that only a small amount of these will bounce back.

--
Mook

Justin Dolske

unread,
Aug 20, 2011, 7:36:38 PM8/20/11
to
On 8/19/11 8:08 AM, Kyle Huey wrote:

> Not sure about Neil, but sometimes I receive a review request and then ask
> additional questions in the bug to clarify things. The ball is then in the
> requestee's court. I could cancel these review requests to avoid the nag
> emails, but that seems less than desirable.

Some time ago Jesse proposed adding a "Next Action" field to bugs, so
that it was easier to understand where a bug is at. EG "waiting for
security review to complete", "need updated spec", etc. "waiting for
reply from patch submitter" would be a perfect use for it as well.

[I don't remember if Jesse's proposal was just a whiteboard-like text
field, or assignee-like username field... The issue here suggests we
might want to have a username available so people can search for bugs
they need to act upon.]

Justin

Gervase Markham

unread,
Aug 22, 2011, 9:46:11 AM8/22/11
to
On 19/08/11 17:04, Jesper Kristensen wrote:
> I think you should cancel the review request and ask the patch author to
> set it again when he has answered the questions. That way you do not
> leave confusion on if the ball is on your side or his.

I think this is the right thing to do. It has more upsides than leaving
the r= request in place.

Gerv

Gervase Markham

unread,
Aug 22, 2011, 9:47:04 AM8/22/11
to
On 19/08/11 18:28, Justin Wood (Callek) wrote:
> IMO a whine set on canconfirm or editbugs would be a reasonable
> expectation here, and cut out probably at least 1/4 of our bugzilla
> users, maybe much more.

Many more, I think. However, hooking into the permissions grant to set
up the whine may be harder than hooking into account creation to set up
the whine.

Gerv

Gervase Markham

unread,
Aug 22, 2011, 9:48:37 AM8/22/11
to
On 20/08/11 10:20, Mook wrote:
> Regarding review bankruptcy, what would drive me towards updating my
> long rotted patches? Will the expectation be that it will get looked at
> this time? Because that either means allocating time for the incoming
> review rush, or not doing so an kicking the problem down the road...

Well, the idea would be that you updated the patch, and set a new review
request on the new patch, and then that would be whined about and have
expectations about it just as any other new review request.

We can't magically make more reviewer time, of course.

Gerv

Natch

unread,
Aug 24, 2011, 12:00:41 PM8/24/11
to
It so happens to be I saw this as an issue with some JS bugs and did
my own little thing about it at the time:

See: http://tinyurl.com/jsDeadEndBugs

and

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/9f5890c7c4151384

I thought that a dashboard on bugzilla would help with this (default
login page for users) where these outstanding reviews would be
indicated (and highlighted appropriately).

fantasai

unread,
Aug 25, 2011, 8:48:32 PM8/25/11
to
On 08/16/2011 03:22 PM, Scott Johnson wrote:
> So, one idea to alleviate some of the review requests that come in is to focus on creating more reviewers. One thing to
> accomplish this might be to add a section 'additional reviewers' to the modules page on the wiki. In order to become an
> 'additional reviewer', one would shadow an existing reviewer. So, for example, an existing reviewer of a module would receive
> a review request, and perform the review while another potential reviewer shadowing them is watching (either online via a
> skype/shared desktop call, or if you happen to be in the office with the person, while you're at their desk). Then, the next
> review that comes in could be done in parallel by both the existing reviewer and the shadower, and they could meet to discuss
> their findings.
>
> This could be done a few times to "train" new reviewers and familiarize them with the code and how to review it. It takes a
> bit of time up front, but ideally, it would lead to more reviewers over time, and perhaps better review practices.
>
> Another idea would be to create a wiki page for each of the modules discussing the most common findings during review, and
> basic expectations of a review in a given module (perhaps this might be something that would be common across all Mozilla code).

I occasionally, but very rarely, get called on to do reviews for patches on
code I wrote or am otherwise familiar with, even though I don't actually own
or peer any code. If I'm not very confident on my review, or I justthink the
patch could use a third set of more experienced eyes, I'll flag dbaron or
bzbarsky for sr.

So if the idea is to pull more people into reviewing, I'd suggest having
veteran reviewers reassign some reviews to a likely reviewer candidate,
then do a secondary review once the new reviewer gives an r+. That would
reduce the amount of effort required of the veteran, and the new reviewer
can learn both from their experience and from what the veteran caught.
Rinse and repeat, and you'll have another experienced reviewer. :)

~fantasai

Mike Shaver

unread,
Aug 25, 2011, 8:53:01 PM8/25/11
to fantasai, dev-pl...@lists.mozilla.org
On Thu, Aug 25, 2011 at 8:48 PM, fantasai <fantasa...@inkedblade.net> wrote:
> So if the idea is to pull more people into reviewing, I'd suggest having
> veteran reviewers reassign some reviews to a likely reviewer candidate,
> then do a secondary review once the new reviewer gives an r+. That would
> reduce the amount of effort required of the veteran, and the new reviewer
> can learn both from their experience and from what the veteran caught.
> Rinse and repeat, and you'll have another experienced reviewer. :)

We're going to do something similar in Firefox shortly, by making the
submodule peers be reviewers for anything they feel capable of
reviewing.

https://wiki.mozilla.org/Firefox/Code_Review

Mike

Henri Sivonen

unread,
Aug 29, 2011, 3:55:09 AM8/29/11
to dev-pl...@lists.mozilla.org
On Mon, Aug 15, 2011 at 2:39 PM, Gervase Markham <ge...@mozilla.org> wrote:
> On 11/08/11 19:32, Mike Shaver wrote:
>> With...
>>
>> If there were more-suitable module owners around, they'd probably
>> already be the module owner, right?
>
> Given the until-recently dysfunction of our technical systems for making
> this change, not necessarily. :-|

I believe that previously some Web page about module ownership said
something to the effect of existing module owners proposing new ones.
Therefore, I was shy to propose myself as a module owner. (Maybe I
shouldn't have been shy. After all, at the W3C Invited Experts have to
take initiative to ask the W3C to "invite" them.) For quite a while, I
was the de facto module owner for the new HTML parser even though the
new HTML parser didn't exist as a module. It appeared that existing
module owners took my de facto ownership of a de facto module for
granted and didn't bother to propose formal changes. Eventually,
Ms2ger edited the wiki to make the new HTML parser exist as a module
and marked me as the owner.

Who should have and at what point in time taken the initiative to
establish the new HTML parser as a Module? (For all practical
purposes, it was a new module from the time it initially landed.)

--
Henri Sivonen
hsiv...@iki.fi
http://hsivonen.iki.fi/

Gervase Markham

unread,
Aug 29, 2011, 5:52:23 AM8/29/11
to
On 29/08/11 08:55, Henri Sivonen wrote:
> Who should have and at what point in time taken the initiative to
> establish the new HTML parser as a Module? (For all practical
> purposes, it was a new module from the time it initially landed.)

A good question, and one to which the answer has probably been
insufficiently clear.

I think that if a new body of code exists and you are owning it, it
should probably have a module. So in hindsight, emailing the Core owner
(Brendan) when you landed the parser would have been the right thing to do.

Gerv

0 new messages