Proposal to change review requirements in Core

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposal to change review requirements in Core

Ehsan Akhgari
(CCing dev-planning and dev-platform to keep them in the loop; follow-ups
to governance, please!)

A while ago, the Firefox and Toolkit modules switched to a new review model
where the owner and any of the peers for its sub-modules are trusted to
review any patch in Firefox/Toolkit, based on the assumption that reviewers
are trusted to act responsibly and redirect review requests for code they
don't understand, or patches they're not comfortable to review (
https://wiki.mozilla.org/Firefox/Code_Review).

This is already the way that many of the Core sub-modules reviewers (myself
included) work today.  But according to the existing review rules for Core,
people are only allowed to review code in sub-modules that they own or a
peer of.  This sometimes creates unnecessary trouble for people seeking and
doing the reviews, and is especially troublesome for our unowned Core
sub-modules.  Changing the review rules in Core to match Firefox/Toolkit
will also simplify the review rules for a large portion of our code base,
which will hopefully make things a bit easier for new contributors.

Therefore, I'm proposing that we change the review rules in Core to match
the reality, by allowing owners and peers of any sub-module to review any
patch in code if they're comfortable with doing so, and forward the review
requests to other people if they feel they're not the right reviewer for
the code in question.

I'll volunteer to write up a wiki page about this and then would ask
Brendan to make this official!

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Benoit Jacob-2
2012/4/10 Ehsan Akhgari <[hidden email]>:

> (CCing dev-planning and dev-platform to keep them in the loop; follow-ups
> to governance, please!)
>
> A while ago, the Firefox and Toolkit modules switched to a new review model
> where the owner and any of the peers for its sub-modules are trusted to
> review any patch in Firefox/Toolkit, based on the assumption that reviewers
> are trusted to act responsibly and redirect review requests for code they
> don't understand, or patches they're not comfortable to review (
> https://wiki.mozilla.org/Firefox/Code_Review).
>
> This is already the way that many of the Core sub-modules reviewers (myself
> included) work today.  But according to the existing review rules for Core,
> people are only allowed to review code in sub-modules that they own or a
> peer of.  This sometimes creates unnecessary trouble for people seeking and
> doing the reviews, and is especially troublesome for our unowned Core
> sub-modules.  Changing the review rules in Core to match Firefox/Toolkit
> will also simplify the review rules for a large portion of our code base,
> which will hopefully make things a bit easier for new contributors.
>
> Therefore, I'm proposing that we change the review rules in Core to match
> the reality, by allowing owners and peers of any sub-module to review any
> patch in code if they're comfortable with doing so, and forward the review
> requests to other people if they feel they're not the right reviewer for
> the code in question.

As far as Gfx is concerned, one would have to go one step further to
match reality: non-owners non-peers would have to be allowed to review
patches, on parts of the codebase that they know of course.

If this sounds surprising, think of this example: some new contributor
X starts coding a new feature, is the main author of that new code;
someone else Y makes a patch for that new code. Y should then ask X
for review, primarily because nobody else knows that code as
thoroughly, not even someone who reviewed it. Secondarily, it doesn't
hurt to make new contributor X discover the joys of patch reviewing.

Benoit
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Benoit Jacob-2
2012/4/10 Benoit Jacob <[hidden email]>:

> 2012/4/10 Ehsan Akhgari <[hidden email]>:
>> (CCing dev-planning and dev-platform to keep them in the loop; follow-ups
>> to governance, please!)
>>
>> A while ago, the Firefox and Toolkit modules switched to a new review model
>> where the owner and any of the peers for its sub-modules are trusted to
>> review any patch in Firefox/Toolkit, based on the assumption that reviewers
>> are trusted to act responsibly and redirect review requests for code they
>> don't understand, or patches they're not comfortable to review (
>> https://wiki.mozilla.org/Firefox/Code_Review).
>>
>> This is already the way that many of the Core sub-modules reviewers (myself
>> included) work today.  But according to the existing review rules for Core,
>> people are only allowed to review code in sub-modules that they own or a
>> peer of.  This sometimes creates unnecessary trouble for people seeking and
>> doing the reviews, and is especially troublesome for our unowned Core
>> sub-modules.  Changing the review rules in Core to match Firefox/Toolkit
>> will also simplify the review rules for a large portion of our code base,
>> which will hopefully make things a bit easier for new contributors.
>>
>> Therefore, I'm proposing that we change the review rules in Core to match
>> the reality, by allowing owners and peers of any sub-module to review any
>> patch in code if they're comfortable with doing so, and forward the review
>> requests to other people if they feel they're not the right reviewer for
>> the code in question.

Oh, I misread the "and forward the review requests to other people"
part. Your plan sounds fine then.

Benoit

>
> As far as Gfx is concerned, one would have to go one step further to
> match reality: non-owners non-peers would have to be allowed to review
> patches, on parts of the codebase that they know of course.
>
> If this sounds surprising, think of this example: some new contributor
> X starts coding a new feature, is the main author of that new code;
> someone else Y makes a patch for that new code. Y should then ask X
> for review, primarily because nobody else knows that code as
> thoroughly, not even someone who reviewed it. Secondarily, it doesn't
> hurt to make new contributor X discover the joys of patch reviewing.
>
> Benoit
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Boris Zbarsky
In reply to this post by Ehsan Akhgari
On 4/10/12 3:49 PM, Ehsan Akhgari wrote:
> A while ago, the Firefox and Toolkit modules switched to a new review model
> where the owner and any of the peers for its sub-modules are trusted to
> review any patch in Firefox/Toolkit, based on the assumption that reviewers
> are trusted to act responsibly and redirect review requests for code they
> don't understand, or patches they're not comfortable to review (
> https://wiki.mozilla.org/Firefox/Code_Review).

This seems fine if people actually act responsibly.  In the past, we've
had some history of people reviewing patches in code they don't
understand to such an extent that they don't even know they don't
understand it...  It's not a good place to be.  :(

> This is already the way that many of the Core sub-modules reviewers (myself
> included) work today.

Indeed.  Explicitly saying so (and pointing out that if someone is not
100% sure they understand the code they're reviewing they should really
ask someone else, if there is such a someone else), seems like a ood idea.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Justin Lebar-3
FWIW, there are 96 checkins with r=jlebar/justin.lebar, even though
I'm not a peer of any modules, so, under a strict interpretation of
the rules, I shouldn't be able to review anything.

Changing the rules to better match reality -- that ownership of parts
of our code is often informal -- seems like a good thing to me.

-Justin

On Tue, Apr 10, 2012 at 4:13 PM, Boris Zbarsky <[hidden email]> wrote:

> On 4/10/12 3:49 PM, Ehsan Akhgari wrote:
>>
>> A while ago, the Firefox and Toolkit modules switched to a new review
>> model
>> where the owner and any of the peers for its sub-modules are trusted to
>> review any patch in Firefox/Toolkit, based on the assumption that
>> reviewers
>> are trusted to act responsibly and redirect review requests for code they
>> don't understand, or patches they're not comfortable to review (
>> https://wiki.mozilla.org/Firefox/Code_Review).
>
>
> This seems fine if people actually act responsibly.  In the past, we've had
> some history of people reviewing patches in code they don't understand to
> such an extent that they don't even know they don't understand it...  It's
> not a good place to be.  :(
>
>
>> This is already the way that many of the Core sub-modules reviewers
>> (myself
>> included) work today.
>
>
> Indeed.  Explicitly saying so (and pointing out that if someone is not 100%
> sure they understand the code they're reviewing they should really ask
> someone else, if there is such a someone else), seems like a ood idea.
>
> -Boris
>
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

L. David Baron
[ looping the other lists back in ]

On Tuesday 2012-04-10 18:07 -0400, Justin Lebar wrote:
> FWIW, there are 96 checkins with r=jlebar/justin.lebar, even though
> I'm not a peer of any modules, so, under a strict interpretation of
> the rules, I shouldn't be able to review anything.
>
> Changing the rules to better match reality -- that ownership of parts
> of our code is often informal -- seems like a good thing to me.

Perhaps another way of expressing what we actually do is that:

 * Owners or peers can do reviews within that module, or delegate
   that task to others.  This implies that:

   + If you're not an owner or peer of the module, you should ask an
     owner or peer for review, though they may in turn delegate to
     somebody else.

   + If you are an owner or peer of a module, you can ask others for
     review when appropriate (with the goal of broadening expertise
     in the module and training eventual new peers or owners).

 * Certain simple changes to a widely-used API can be reviewed by an
   owner/peer of the module that provides the API without consulting
   each of the users.

-David

--
𝄞   L. David Baron                         http://dbaron.org/   𝄂
𝄢   Mozilla                           http://www.mozilla.org/   𝄂
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Daniel Veditz-2
In reply to this post by Justin Lebar-3
On 4/10/12 3:07 PM, Justin Lebar wrote:
> FWIW, there are 96 checkins with r=jlebar/justin.lebar, even though
> I'm not a peer of any modules, so, under a strict interpretation of
> the rules, I shouldn't be able to review anything.

If your review was requested by a peer or owner that's not a
violation even in a strict sense (delegation is allowed).

> Changing the rules to better match reality -- that ownership of parts
> of our code is often informal -- seems like a good thing to me.

Or change your peer status to match reality.

-Dan Veditz
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Jason Duell
In reply to this post by L. David Baron
On 04/10/2012 03:19 PM, L. David Baron wrote:
> Perhaps another way of expressing what we actually do is that:
>
>   * Owners or peers can do reviews within that module, or delegate
>     that task to others.

We made this the official necko review policy recently, and it seems to
me the best plan.   It allows other people to review stuff, but they
have to check in first with a relevant module peer.    It's both more
flexible than the proposal to make all Core peers reviewers, and more
likely to avoid reviews by someone who don't know important intricacies
of some piece of code.

Jason



_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Bobby Holley-2
In reply to this post by Daniel Veditz-2
On Tue, Apr 10, 2012 at 3:55 PM, Daniel Veditz <[hidden email]> wrote:

> Or change your peer status to match reality.
>

Well, that doesn't always work. I think being a peer generally means that a
person can review any/most changes in that module. But module boundaries
don't always offer the necessary granularity. Sometimes, people have
specific expertise in a topic or area of code that makes them appropriate
reviewers for certain things. For example, I delegate XPConnect cycle
collector reviews to mccr8, even though it wouldn't really make sense for
him to be an XPConnect peer (for traditional definitions of 'peer').

But in general, I think it makes sense for review requests to go through
official owners/peers and have them delegate as necessary. If it becomes
obvious that certain reviews are always being bounced to someone in
particular, experienced developers can use their discretion to sort-circuit
things.

bholley
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Justin Lebar-3
In reply to this post by Daniel Veditz-2
>> Changing the rules to better match reality -- that ownership of parts
>> of our code is often informal -- seems like a good thing to me.
>
> Or change your peer status to match reality.

Well...given an unowned module -- say, jemalloc a year ago -- should
the rule be that no progress can be made until someone steps up and
asks to own it?

The story with jemalloc is: It was basically unowned after jasone
left.  I made some changes, which I asked Kyle to review, since I
guessed he had a passing familiarity with the code, for the purposes
of the build system.

If the rule had been that we needed to have an owner, I'm not sure I
would have been comfortable stepping up, especially since, at the
beginning, none of us knew what we were doing.

It's also worth noting that not every bit of code which is owned is
necessarily deserving of its own module.  njn and I own about:memory,
but should that be a module?  It's a few javascript files and some
interfaces which are used in various places.  In this case, hg log is
a much more accurate record of ownership than any wiki will ever be.

To give another example, I asked Mike Hommey to review my recent
changes to WindowsDllInterceptor.  Should I have asked Vlad for
review, even though he's gone, since he wrote that file and there was
no formal transfer of ownership?  I think Mike was fully capable of
giving a careful review.

Maybe my experience is unusual, but I deal with lots of un- and
informally-owned code.  Inasmuch as I can continue to ignore any new
policy like I ignore the current policy, I don't care what the new
rules are.  But when it comes to code with no clear owner, I think a
policy of asking for reviews from people capable of giving a careful
review is about the best we can hope for.  What's the alternative --
designating that our already over-worked superreviewers are now the
owners of all un-owned code?
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Dave Townsend
In reply to this post by Daniel Veditz-2
On 04/10/12 17:09, Justin Lebar wrote:

>>> Changing the rules to better match reality -- that ownership of parts
>>> of our code is often informal -- seems like a good thing to me.
>>
>> Or change your peer status to match reality.
>
> Well...given an unowned module -- say, jemalloc a year ago -- should
> the rule be that no progress can be made until someone steps up and
> asks to own it?
>
> The story with jemalloc is: It was basically unowned after jasone
> left.  I made some changes, which I asked Kyle to review, since I
> guessed he had a passing familiarity with the code, for the purposes
> of the build system.
>
> If the rule had been that we needed to have an owner, I'm not sure I
> would have been comfortable stepping up, especially since, at the
> beginning, none of us knew what we were doing.

This is (hopefully but far more often than we would like) not the norm,
but I think in that case we just have to use our best judgement at the
time. I think we should find people willing to be responsible for code
that is unowned, even if that responsibility is just an agreement to
help patch authors find appropriate reviewers for code. In that
circumstance the "owner" here doesn't have to actually know the code in
question much, though obviously that would be better.

> It's also worth noting that not every bit of code which is owned is
> necessarily deserving of its own module.  njn and I own about:memory,
> but should that be a module?  It's a few javascript files and some
> interfaces which are used in various places.  In this case, hg log is
> a much more accurate record of ownership than any wiki will ever be.

I disagree, we should aim to have modules covering all code in the
browser so we hopefully have people ultimately responsible for
everything (brendan being the ultimate ultimate authority), even if that
responsibility is about helping you confirm who would be the best
reviewer for some code. I believe Gerv was doing work to ensure that in
fact.

For about:memory it is in the toolkit module so technically I am the
owner and any of the toolkit peers comfortable can review it or delegate
review for it. And yes I agree the peers should be using the log to see
who is familiar with the code and delegating to them if they feel that
is best.

> To give another example, I asked Mike Hommey to review my recent
> changes to WindowsDllInterceptor.  Should I have asked Vlad for
> review, even though he's gone, since he wrote that file and there was
> no formal transfer of ownership?  I think Mike was fully capable of
> giving a careful review.

Again part of toolkit, so technically you should have talked to one of
the toolkit peers or me about it to confirm that we were ok with Mike
doing the review there. I am :)

> Maybe my experience is unusual, but I deal with lots of un- and
> informally-owned code.  Inasmuch as I can continue to ignore any new
> policy like I ignore the current policy, I don't care what the new
> rules are.  But when it comes to code with no clear owner, I think a
> policy of asking for reviews from people capable of giving a careful
> review is about the best we can hope for.  What's the alternative --
> designating that our already over-worked superreviewers are now the
> owners of all un-owned code?

The problem of course is in deciding who is capable of giving a careful
review. You've been around for long enough to have a good idea of this.
Not everyone has so they have to rely on the peer lists.

As I said above I don't think ownership has to imply being on the hook
for all reviews. I think it should be about having people we trust to
help choose reviewers. The super-reviewer list seems like a reasonable
fall-back, in cases where we really have nothing else, to help the
author figure out the right reviewer.

I think we've always been extremely lenient in letting reviewers find
the right reviewer when it comes to it, we generally trust committers to
be confident that any patch they land has had appropriate reviews and
there is almost no checks in place to ensure that. I think that's fine,
rules are made to be broken when necessary.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Alexander Surkov
In reply to this post by L. David Baron
>   + If you're not an owner or peer of the module, you should ask an
>     owner or peer for review, though they may in turn delegate to
>     somebody else.
>
>   + If you are an owner or peer of a module, you can ask others for
>     review when appropriate (with the goal of broadening expertise
>     in the module and training eventual new peers or owners).

+1. That's a rule we should keep to otherwise we end up with Boris
example: "In the past, we've had some history of people reviewing
patches in code they don't understand to such an extent that they
don't even know they don't understand it."

Alex.

On Wed, Apr 11, 2012 at 7:19 AM, L. David Baron <[hidden email]> wrote:

> [ looping the other lists back in ]
>
> On Tuesday 2012-04-10 18:07 -0400, Justin Lebar wrote:
>> FWIW, there are 96 checkins with r=jlebar/justin.lebar, even though
>> I'm not a peer of any modules, so, under a strict interpretation of
>> the rules, I shouldn't be able to review anything.
>>
>> Changing the rules to better match reality -- that ownership of parts
>> of our code is often informal -- seems like a good thing to me.
>
> Perhaps another way of expressing what we actually do is that:
>
>  * Owners or peers can do reviews within that module, or delegate
>   that task to others.  This implies that:
>
>   + If you're not an owner or peer of the module, you should ask an
>     owner or peer for review, though they may in turn delegate to
>     somebody else.
>
>   + If you are an owner or peer of a module, you can ask others for
>     review when appropriate (with the goal of broadening expertise
>     in the module and training eventual new peers or owners).
>
>  * Certain simple changes to a widely-used API can be reviewed by an
>   owner/peer of the module that provides the API without consulting
>   each of the users.
>
> -David
>
> --
> 𝄞   L. David Baron                         http://dbaron.org/   𝄂
> 𝄢   Mozilla                           http://www.mozilla.org/   𝄂
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Robert O'Callahan-3
In reply to this post by Bobby Holley-2
On Wed, Apr 11, 2012 at 11:10 AM, Bobby Holley <[hidden email]>wrote:

> Well, that doesn't always work. I think being a peer generally means that a
> person can review any/most changes in that module. But module boundaries
> don't always offer the necessary granularity.
>

Feel free to make up your own submodule boundaries and grant persistent
authority to Andrew to review within those boundaries. I think this is a
valid application of the authority peers have to delegate reviews.

But in general, I think it makes sense for review requests to go through
> official owners/peers and have them delegate as necessary. If it becomes
> obvious that certain reviews are always being bounced to someone in
> particular, experienced developers can use their discretion to sort-circuit
> things.
>

Yes.

I don't believe a single written rule, consistently applied, can hit the
optimal point between being too strict and too lax.

I think our current rule is a tiny bit too strict, but I think it's working
well enough to not be worth tweaking. Being a bit too strict means less
experienced developers are likely to err on the side of being conservative,
which is sounds right. More experienced developers tend to operate on the
lax side of the rule, which is also fine.

Rob
--
“You have heard that it was said, ‘Love your neighbor and hate your enemy.’
But I tell you, love your enemies and pray for those who persecute you,
that you may be children of your Father in heaven. ... If you love those
who love you, what reward will you get? Are not even the tax collectors
doing that? And if you greet only your own people, what are you doing more
than others?" [Matthew 5:43-47]
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Zack Weinberg-2
In reply to this post by Daniel Veditz-2
On 2012-04-10 5:09 PM, Justin Lebar wrote:
>
> Maybe my experience is unusual, but I deal with lots of un- and
> informally-owned code.  Inasmuch as I can continue to ignore any new
> policy like I ignore the current policy, I don't care what the new
> rules are.  But when it comes to code with no clear owner, I think a
> policy of asking for reviews from people capable of giving a careful
> review is about the best we can hope for.  What's the alternative --
> designating that our already over-worked superreviewers are now the
> owners of all un-owned code?

I've had similar problems (some of my patches are _still_ looking for a
good reviewer, years later :-/) and I worry about people who are new to
the process not being able to figure out who is an appropriate reviewer.

I don't know if this would actually work in practice, but it's the only
idea I've got: a documented set of people who pledge to find reviewers
for anything. If you need a review and you don't know who is
appropriate, tag one of them and they'll help.

The big problem I see is that the set of people qualified to do that job
is roughly the set of, as you said, already-over-worked super-reviewers. :-/

zw
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Gavin Sharp-3
On Wed, Apr 11, 2012 at 2:12 PM, Zack Weinberg <[hidden email]> wrote:
> I've had similar problems (some of my patches are _still_ looking for a good
> reviewer, years later :-/) and I worry about people who are new to the
> process not being able to figure out who is an appropriate reviewer.

I imagine that by "good reviewer" you mean "someone willing to spend
the time reviewing unowned code", not "someone with enough authority
to be the reviewer". The distinction is important, I think - it's a
different kind of problem that needs a different solution.

> I don't know if this would actually work in practice, but it's the only idea
> I've got: a documented set of people who pledge to find reviewers for
> anything. If you need a review and you don't know who is appropriate, tag
> one of them and they'll help.
>
> The big problem I see is that the set of people qualified to do that job is
> roughly the set of, as you said, already-over-worked super-reviewers. :-/

I don't think that's true - the ability to identify a suitable
reviewer given a patch is held by many people who aren't super
reviewers. Pretty much anyone with a high level familiarity with our
code base and tools can learn to do it pretty effectively (even if it
just means doing something like
http://weblog.latte.ca/blake/employment/mozilla/thunderbird/reviewerChooser#
). "Ask #developers" is usually a pretty effective way to find a
reviewer, if you're having trouble. It isn't always effective if
you're dealing with unowned code, but but as I said above, I think
that's a different issue that needs its own separate solution.

Gavin
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Zack Weinberg-2
In reply to this post by Zack Weinberg-2
On 2012-04-11 2:59 PM, Gavin Sharp wrote:

> On Wed, Apr 11, 2012 at 2:12 PM, Zack Weinberg<[hidden email]>
> wrote:
>> I've had similar problems (some of my patches are _still_ looking
>> for a good reviewer, years later :-/) and I worry about people who
>> are new to the process not being able to figure out who is an
>> appropriate reviewer.
>
> I imagine that by "good reviewer" you mean "someone willing to spend
> the time reviewing unowned code", not "someone with enough authority
> to be the reviewer". The distinction is important, I think - it's a
> different kind of problem that needs a different solution.

I may actually mean someone who meets _both_ qualifications: for
instance, it doesn't matter how much positive feedback an NSPR patch
has, if wtc doesn't like it or doesn't have time to look at it, it's
going nowhere.  My oldest outstanding review request (for bug #305206;
two and a half years with no reaction) may be misdirected, but at the
time I originally posted the patch, it seemed to be aimed at the only
person with both authority and time in that area.

> the ability to identify a suitable reviewer given a patch is held by
> many people who aren't super reviewers. Pretty much anyone with a
> high level familiarity with our code base and tools can learn to do
> it pretty effectively (even if it just means doing something like
> http://weblog.latte.ca/blake/employment/mozilla/thunderbird/reviewerChooser#

That tool is clever, but to my mind, it is a little too likely to
pick people who haven't been active in many years.

> "Ask #developers" is usually a pretty effective way to find a
> reviewer, if you're having trouble. It isn't always effective if
> you're dealing with unowned code, but but as I said above, I think
> that's a different issue that needs its own separate solution.

Well, provided you know #developers exists in the first place...

zw
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to change review requirements in Core

Gavin Sharp-3
On Wed, Apr 11, 2012 at 3:30 PM, Zack Weinberg <[hidden email]> wrote:
> Well, provided you know #developers exists in the first place...

Yes, discoverability/documentation is yet another problem.
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews
is fairly decent, but could be improved with a mention of
#introduction/#developers (and may benefit from being split into it's
own easily-linkable/searchable top level page).

Gavin
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning