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/>

[Benoit Jacob]

2012/4/10 Ehsan Akhgari <ehsan....@gmail.com>:

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

[Benoit Jacob]

2012/4/10 Benoit Jacob <jacob.b...@gmail.com>:

> 2012/4/10 Ehsan Akhgari <ehsan....@gmail.com>:
>> (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

[Ehsan Akhgari]

> 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.

This is beyond my proposal here. As I'm not entirely convinced about that
being a good policy in general, I'd like to leave it out of this proposal.
But you should feel free to bring it up after this change. :-)

--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Tue, Apr 10, 2012 at 4:13 PM, Boris Zbarsky <bzba...@mit.edu> 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<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. :(
>

Of course. But if we do have such peers, we have bigger problems already
even under the current policy. I believe we have escalation methods for
addressing that kind of problem in place.

>
> 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.
>

Agreed.

--
Ehsan
<http://ehsanakhgari.org/>

[Kyle Huey]

>
> 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 mostly oppose this.

For review purposes, there are in my mind two classes of patches. The
first (type 1) is generic things that don't need any domain expertise or
understanding of the module's design. Things like bug 681688 and bug
715185. These I'm ok with being reviewed by any peer (or people that
aren't even peers, e.g. Ms2ger, who have demonstrated general competence
with our codebase).

The second (type 2) are patches that require understanding the tradeoffs
made in the design of the existing code, or things particular to that
module. Things that may seem innocuous to someone not familiar with the
code might be a big problem for a peer of the relevant code. For example,
someone not familiar with content might think that adding more member
variables to nsINode is not a big deal, but that is heavily optimized for
space, and the peers would likely raise their eyebrows at that. Its easy
to think that a change is easier or simpler than it actually is.

To give concrete examples of things that I am a peer of, I can count the
number of people that I would trust to review a substantive build system
change (meaning a custom build targets, or tweaking an existing ones, or
complex configure logic) and end up with something that doesn't have race
conditions or other problems who aren't peers of the build system on one
hand. I expect lots of people could review a patch that adds a configure
option for correctness, but we also have a strong desire to limit the
number of configure options that I don't expect that people other than the
current peers would hold the line on. The IndexedDB code is a bit
different because not very many people hack on it, but the only people I
would trust to do reviews of patches of type 2 who aren't peers are Jan
Varga and maybe sdwilsh (though I doubt he's clamoring to do IndexedDB
reviews these days).

I do not support this change for patches of type 2, and because determining
whether a patch is type 1 or type 2 is tricky, I think we should be wary of
codifying even that as an official policy.

- Kyle

PS. Peers should always be redirecting review requests if they don't feel
comfortable with the review (modulo situations where there is no other
reviewer who would be more appropriate). In certain situations that may
even mean delegating to someone who isn't a peer. If those situations are
common that's probably a good sign that the person in question should be a
peer (or that the module should be split, and appropriate owners/peers
determined for the divisions).

[Ehsan Akhgari]

The background for why I'm asking this is to update the rules to match the
reality. I'm not really proposing that we change who reviews what code
because of this.

I think a large part of what determines whether somebody should be a peer
of a module is determined by how responsible they can act with regard to
incoming review requests. If somebody has not reviewed code in nsINode
before, they should at least ask for somebody else to take a look at the
code too. If somebody knows a piece of code better than everyone else on
the owner/peer list of the respective module, they should probably be the
person who would review changes to that code, even if they're not a peer of
that module. I think both of these cases happen every day at Mozilla. In
other words, this is already how we're operating, and it's been so for a
while.

There is also the question of how we're going to react if somebody reviews
something without understanding the code involved (which is the same case
that Boris brought up.) The existing policy doesn't always stop people
from doing this, and I don't think any changes to the policy will make this
situation better/worse. I think that the policy should be worded in a way
that explicitly discourages this. Let's not forget that if we don't trust
our existing peers to act responsibly, we already have much bigger problems
(the value of the review system comes from the implicit assumption that the
reviewer knows what they're doing, otherwise it could do even more harm
than good!).

Do you have other concerns besides what Boris brought up?

Thanks!
--
Ehsan
<http://ehsanakhgari.org/>

[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/ 𝄂

[Jason Duell]

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

[Kyle Huey]

> The background for why I'm asking this is to update the rules to match the
> reality. I'm not really proposing that we change who reviews what code
> because of this.
>

.
My point is that I believe that in certain cases we are too liberal now,
and I believe that codifying that would make the situation worse.

> I think a large part of what determines whether somebody should be a peer
> of a module is determined by how responsible they can act with regard to
> incoming review requests. If somebody has not reviewed code in nsINode
> before, they should at least ask for somebody else to take a look at the
> code too. If somebody knows a piece of code better than everyone else on
> the owner/peer list of the respective module, they should probably be the
> person who would review changes to that code, even if they're not a peer of
> that module. I think both of these cases happen every day at Mozilla. In
> other words, this is already how we're operating, and it's been so for a
> while.
>

I agree with just about everything that you've written here.

>
> There is also the question of how we're going to react if somebody reviews
> something without understanding the code involved (which is the same case
> that Boris brought up.) The existing policy doesn't always stop people
> from doing this, and I don't think any changes to the policy will make this
> situation better/worse. I think that the policy should be worded in a way
> that explicitly discourages this. Let's not forget that if we don't trust
> our existing peers to act responsibly, we already have much bigger problems
> (the value of the review system comes from the implicit assumption that the
> reviewer knows what they're doing, otherwise it could do even more harm
> than good!).
>

Except that our peers don't always act responsibly. People routinely push
code to the tree that fails tests or even fails to compile (I did it
earlier this week!)

I'm hesitant to do this because I don't want to call people out, and my
intent here is not to blame anyone, but here are a couple concrete examples.

https://bugzilla.mozilla.org/show_bug.cgi?id=587698#c37<https://bugzilla.mozilla.org/show_bug.cgi?id=587698#c37>,
where a team working on one module disabled tests for another module
without consulting the owners or peers of that module. This masked
https://bugzilla.mozilla.org/show_bug.cgi?id=689128 (and who knows what
else) for a month.

https://bugzilla.mozilla.org/show_bug.cgi?id=714553, where a team wrote a
bunch of custom build logic, and did not want to take the fix that the
peers of the build system thought was correct because it would change their
workflow. After sitting at an impasse for some time philor landed the
hacky fix in the bug to stop the tree from burning and Ted and I decided
not to fight that battle any further.

A bug that I can't find a link to right now, where a patch landed for a bug
that was filed in the wrong component without reviews by any of the
relevant people. The change was discovered when regressions were noticed
and backed out.

Thankfully these are rare occurrences. The vast majority of the time, the
system works as designed. You are absolutely correct that the existing
system does not always prevent this from happening. You are wrong that
changes to the policy will not change the situation. The key point here is
that people were comfortable doing the reviews (since they did them), which
as far as I can tell means they were "in policy" under the proposal.

> Do you have other concerns besides what Boris brought up?
>

My concern is that making this change will increase the frequency of these
events. I don't see any reason to believe it won't.

- Kyle

[Justin Lebar]

>> 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?

[Mike Connor]

On 2012-04-10, at 3:49 PM, Ehsan Akhgari wrote:

> (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!

I suppose the core questions I have here are:

* What specific problem are you trying to solve? You mention unnecessary trouble, but don't give specifics, so I don't know how to evaluate this proposed solution.

* If the problem is on the seeking side, I'm fairly sure this would make things worse, as in many cases I would imagine there are far more people who _shouldn't_ review a patch than those who should, so why do you think it'd make things easier?

* Are there modules where review is not timely from the owner or peers, where there are other qualified reviewers who can help? If so, why not simply add more peers to those modules?

* Owners and peers are free to delegate specific reviews to others, so why not encourage more of that, if the problem is that module reviewers are overloaded but aren't yet ready to give unfettered review access to more people?

* Why try to impose a generic solution? Individual module owners can make this choice for their modules, have they expressed interest in adopting this type of model?

-- Mike

[Alexander Surkov]

> + 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 <dba...@dbaron.org> 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
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning

[Justin Dolske]

On 4/10/12 2:09 PM, Ehsan Akhgari wrote:

> The background for why I'm asking this is to update the rules to match the
> reality. I'm not really proposing that we change who reviews what code
> because of this.

I'm generally in favor of this. I think it's working well for
Toolkit/Firefox, and having greater consistency would sure be swell. :)

But perhaps "Core" is too broad of a module and is should first be split
into a few distinct pieces? (Or, equivalently, have specific reviewer
pools for specific groups of submodules?)

The submodules of Firefox and Toolkit are generally pretty similar, so
this system has worked well there. The submodules of Core, OTOH, cover a
wide array of different disciplines... Build system, JS Engine, NSS,
DOM/Layout, Widget, Gfx. Even just those 5 are pretty radically different.

Looking at it from a different angle: A noob with a front-end patch
could ask for review from anyone on the Firefox/Toolkit list, and odds
are that person will be comfortable reviewing it. That wouldn't be true
of a Core reviewers list, there are very distinct groups of expertise,
and I would suspect that odds are a randomly chosen reviewer would not
feel comfortable reviewing an random core patch.

Justin

[Ehsan Akhgari]

On Tue, Apr 10, 2012 at 10:24 PM, Mike Connor <mco...@mozilla.com> wrote:

> I suppose the core questions I have here are:
>
> * What specific problem are you trying to solve? You mention unnecessary
> trouble, but don't give specifics, so I don't know how to evaluate this
> proposed solution.
>

Here are the problems I'm trying to solve:

* Enable Core peers who are not on the list of peers for a module review
patches to the code they know best without "breaking the rules".
* Enable Core peers review patches to unowned modules without breaking the
rules, and make it easier for new contributors to know who to ask for
review in one of those modules (i.e., "look at the hg log and see who most
recently reviewed code in that module.)
* Provide a way for growing more people to be reviewers. In my experience,
the bar on somebody becoming a peer today is much higher than it needs to
be.

> * If the problem is on the seeking side, I'm fairly sure this would make
> things worse, as in many cases I would imagine there are far more people
> who _shouldn't_ review a patch than those who should, so why do you think
> it'd make things easier?
>

I was not proposing to provide the superset of everyone who's currently an
owner/peer as a list of people who _can_ review a patch. The current rule
of thumb in at least many areas of the code base is to see who has been
reviewing patches to the same code more recently and ask them for review,
and I think that has been working well in general. That is what I'm hoping
that we would end up with, provided that the person in question is a peer
of some module. Sorry if I was not clear on that.

> * Are there modules where review is not timely from the owner or peers,
> where there are other qualified reviewers who can help? If so, why not
> simply add more peers to those modules?
>

Yes, there are. To give you an example, I'm probably a good person to
review code in nsTextControlFrame, but that is only a tiny component of the
Layout Engine module, and I don't think I deserve to be a peer of that
module just because I know one piece well. Our modules are simply too
coarse grained to make adding more peers work well in every case.

> * Owners and peers are free to delegate specific reviews to others, so why
> not encourage more of that, if the problem is that module reviewers are
> overloaded but aren't yet ready to give unfettered review access to more
> people?
>

I think encouraging delegation is something that we should be doing more
of. The current review policy, as far as I can see, only allows delegation
to the peers of a module, so my proposal would probably make finding
somebody to delegate to a bit easier if you want to stick to the rules, but
I generally think that is an orthogonal issue, and no matter what our
review policy is, reviewers should strive to delegate more where it makes
sense.

> * Why try to impose a generic solution? Individual module owners can make
> this choice for their modules, have they expressed interest in adopting
> this type of model?

I'm not trying to impose a solution, this is a discussion after all. :-)
I think this change has worked fairly well for Firefox/Toolkit. But I do
understand the hesitation to apply the same idea to Core (which includes
way more modules and lines of code and Firefox/Toolkit). I don't know how
to approach this at a smaller scale though.

[Ehsan Akhgari]

On Tue, Apr 10, 2012 at 7:52 PM, Kyle Huey <m...@kylehuey.com> wrote:

> Thankfully these are rare occurrences. The vast majority of the time,
>> the system works as designed. You are absolutely correct that the existing
>> system does not always prevent this from happening. You are wrong that
>> changes to the policy will not change the situation. The key point here is
>> that people were comfortable doing the reviews (since they did them), which
>> as far as I can tell means they were "in policy" under the proposal.
>>
>

Yeah, I understand this concern. I'm not sure if this is something which
can be addressed at the policy level though. If people behave
irresponsibly when reviewing code, they're not adhering to either the
existing policy or this proposal, as far as I can tell.

[Benjamin Smedberg]

On 4/10/2012 3:49 PM, Ehsan Akhgari wrote:
> 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 don't think that 'Core' is a useful toplevel grouping for this sort of
thing. In particular, one of the reasons this does work for toolkit and
Firefox is that there is still a single overall module owner who knows
all of the peers, can set expectations directly, and can mediate if
there are any concerns about overstepping bounds or unresponsive
reviewers or things like this. "Core" does not have a similar structure.
I do think we should try to be flexible (in general) and freely
delegate. But I don't think that a general 'core peers can use their
judgement everywhere' approach is a good governance/policy structure.

--BDS

[Gavin Sharp]

On Wed, Apr 11, 2012 at 8:51 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> Here are the problems I'm trying to solve:
>
> * Enable Core peers who are not on the list of peers for a module review
> patches to the code they know best without "breaking the rules".

> * Provide a way for growing more people to be reviewers.  In my experience,
> the bar on somebody becoming a peer today is much higher than it needs to
> be.

It seems like these problems would be addressed well by adding an
explicit "owners/peers can delegate review in their modules" clause to
the "rules", as suggested by dbaron. That probably won't be quite as
controversial as your original proposal (it matches common practice
and stands a lower risk of diluting responsibility).

Gavin

[Ehsan Akhgari]

I understand the desire to go for a smaller change to the policy, but I
think we're forgetting the fact that people _already_ ask review from
people who are not an official peer of a module, and if we believe that is
a problem overall (not just in rare examples of bad incidents), there's way
more to fix than just the policy. I'm still not sure why my proposal is
considered to be controversial when it's mostly trying to document our de
facto practices.

--
Ehsan
<http://ehsanakhgari.org/>

[Gavin Sharp]

On Wed, Apr 11, 2012 at 10:38 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> I understand the desire to go for a smaller change to the policy, but I
> think we're forgetting the fact that people _already_ ask review from
> people who are not an official peer of a module

I don't see anyone forgetting that fact. The contention seems to be
about how we should codify that reality in the policy. dbaron's
suggestion leaves discretion up to the module owners and peers of the
relevant module, while yours dilutes responsibility even further into
a single mega-module that is "core" (a pretty large, loosely defined
concept, as bsmedberg points out).

In a lot of ways, the exact text of policy says is only useful to the
people who need to rely on it (e.g. because they are not familiar with
our existing practices, or don't feel comfortable enough making
decisions without it). I think it makes most sense to write the policy
with that audience in mind, and explicitly leave a lot up to the
judgement of module owners/peers, rather than trying to proscribe the
reality perceived by the people discussing things in this newsgroup
(all people quite familiar with how things work in practice!).

> I'm still not sure why my proposal is considered to be controversial
> when it's mostly trying to document our de facto practices.

As far as I can tell, people reacted negatively to the part of your
original proposal that involved making it "in policy" for any peer of
any module considered "Core" to review any code anywhere in that
mega-module. That's much broader than the clarification "peers can
delegate review at their discretion". Baby steps :)

Gavin

[Ehsan Akhgari]

OK. So let's wait to see if anyone objects to dbaron's suggestion.

In the mean time I will continue to think about what we can do for the
unowned modules. But I'm afraid we don't have too many options about them
(unless people step up and start owning them, that is.)

[Mike Connor]

On 2012-04-11, at 4:24 PM, Ehsan Akhgari wrote:

> In the mean time I will continue to think about what we can do for the
> unowned modules. But I'm afraid we don't have too many options about them
> (unless people step up and start owning them, that is.)

Do we have a list? This sounds like something we should be clearly identifying and working to resolve (both the module-ownership group and probably engineering management at MoCo) on some sort of prioritized basis.

-- Mike

[Ehsan Akhgari]

Sure, look at the modules on <https://wiki.mozilla.org/Modules/Core>
without an owner. Also, see the list of bugzilla components without
modules at the end of that page. In addition, some modules might have
areas that are not really owned by anybody who's currently around any more.

--
Ehsan
<http://ehsanakhgari.org/>

[Ben Bucksch]

Could we also say: If somebody created a file or modified significant
parts of it, he's automatically qualified for review of that file? In
fact, he's often more qualified for that part than the module owner.

[Benjamin Smedberg]

No. That person is definitely the most knowledgeable person about a
file, but that doesn't automatically make them a good peer/review. Being
a peer also requires understanding surrouding and related code, changes
which are happening elsewhere, and a trust relationship with the module
owner.

--BDS

[Ben Bucksch]

Yes, that can be argued. It can also even more easily argued that the
content/ module owner is not familiar with all the particular details of
nsPlaintextSerializer.cpp. He might not even understand the different
operation modes it has. He might approve an innocent-looking change, but
that breaks either copy&paste or rich plaintext output badly, because
neither contributor nor content/ module owner even knew that it existed
or had this whitespace particularity.

[Gavin Sharp]

On Wed, Apr 18, 2012 at 1:46 PM, Ben Bucksch
<ben.buck...@beonex.com> wrote:
> Yes, that can be argued. It can also even more easily argued that the
> content/ module owner is not familiar with all the particular details of
> nsPlaintextSerializer.cpp. He might not even understand the different
> operation modes it has. He might approve an innocent-looking change, but
> that breaks either copy&paste or rich plaintext output badly, because
> neither contributor nor content/ module owner even knew that it existed or
> had this whitespace particularity.

It's not clear what your point is. Are you referring to some specific
incident? Everyone can make mistakes during review, module owners
included. But module owners and peers have a responsibility to oversee
all of the code in a given module (that includes knowing when you
aren't the best person to review something, and delegating
appropriately). Benjamin's point is that "the last person who touched
this file" doesn't have that same level of responsibility, and so we
shouldn't explicitly grant them "review power" without involving
module owner/peer judgement (which doesn't need to have much
overhead).

I don't think we need rules to codify "module owners or peers can
delegate review (formally or informally) of specific code areas within
their modules". People are generally reasonable enough to figure this
out on their own - it happens all the time in practice. It certainly
wouldn't hurt to document this current practice, if that's what you're
getting at.

Gavin

[Ben Bucksch]

On 18.04.2012 23:02, Gavin Sharp wrote:
> It's not clear what your point is.

It was stated in the original post: The author of a file is the best
reviewer and should be officially allowed to review. In my opinion, he
should even be required to review it.

> Are you referring to some specific incident?

I tried to keep the discussion rational, not a matter of opinion, by
giving a concrete example, but the same is probably true for many files.

It shows that the module owner is often simply unaware of details, which
means he doesn't even know that it would be better to delegate.

> "the last person who touched this file"

I didn't say that. "Last to touch" is very different from "author of
most of the file".

I'd think the author of a file feels a higher level of responsibility
and dedication than the owner of a whole module.

> I don't think we need rules to codify "module owners or peers can
> delegate review (formally or informally) of specific code areas within
> their modules". People are generally reasonable enough to figure this
> out on their own - it happens all the time in practice.

Sure. I'm already assuming that this is the rule, because delegation is
reasonable and happens all the time. That's not what I meant, though.

Ben

[Gavin Sharp]

On Wed, Apr 18, 2012 at 2:16 PM, Ben Bucksch
<ben.buck...@beonex.com> wrote:
> It was stated in the original post: The author of a file is the best
> reviewer and should be officially allowed to review. In my opinion, he
> should even be required to review it.

> I didn't say that. "Last to touch" is very different from "author of most of
> the file".
>
> I'd think the author of a file feels a higher level of responsibility and
> dedication than the owner of a whole module.

We can all agree that having someone who wrote the code review patches
is often the most sensible solution; what I don't agree with is that
we need change the review rules to somehow enforce that. It's just not
necessary to make a rule to enforce something that already happens all
the time in practice (where it makes sense), particularly given that
there are some cases where it really doesn't. Leaving it up to module
owner/peer judgement is sufficient.

Gavin

[Gijs Kruitbosch]

On 18/04/2012 23:16 PM, Ben Bucksch wrote:
> On 18.04.2012 23:02, Gavin Sharp wrote:
>> It's not clear what your point is.
>
> It was stated in the original post: The author of a file is the best reviewer
> and should be officially allowed to review. In my opinion, he should even be
> required to review it.

Like Benjamin, I think this is the wrong way to go about it. Benjamin already
said why the owner/peers are still better picks in general (we were apparently
not speaking about the possibility of (partial) delegation, which should
obviously exist). However, there are some other reasons why I'd be very weary of
such a change. Particularly:

- this undermines the module owner / peers' authority over the module. It
implies that once you create a file, you decide what happens to it. That
wouldn't scale well and runs counter to my (and, I suspect, others') intuition
of how the review system works.
- patches pretty much always touch more than one file. It's not reasonable to
get review from all the authors of all the files. In fact, even getting review
from a peer / module owner for all modules touched can be a rather heavy burden
in some cases (often lightened because some people are peers/owners of several
modules).
- the original author of the file may not be around/available. The current
module owner/peer system allows spreading review requests, even if peers may not
necessarily have written a majority or even plurality of the code in a module.
Requiring (or even strongly preferring) the author to review would break this
feature of the review system.

~ Gijs

[Ben Bucksch]

On 18.04.2012 23:31, Gavin Sharp wrote:
> On Wed, Apr 18, 2012 at 2:16 PM, Ben Bucksch
> <ben.buck...@beonex.com> wrote:

>> >It was stated in the original post: The author of a file is the best
>> >reviewer and should be officially allowed to review. In my opinion, he
>> >should even be required to review it.

>> >I didn't say that. "Last to touch" is very different from "author of most of
>> >the file".
>> >
>> >I'd think the author of a file feels a higher level of responsibility and
>> >dedication than the owner of a whole module.
> We can all agree that having someone who wrote the code review patches
> is often the most sensible solution; what I don't agree with is that
> we need change the review rules to somehow enforce that.

Can we agree that they should be allowed to review, as a general rule?

Ben

[Gavin Sharp]

On Wed, Apr 18, 2012 at 2:39 PM, Ben Bucksch
<ben.buck...@beonex.com> wrote:
> Can we agree that they should be allowed to review, as a general rule?

It depends on what you mean by "allowed to". "Allowed to by the module
owners and peers" is fine (this is what happens in practice). "Allowed
by some explicit policy with no other restrictions" is not, because of
the various issues Benjamin and I have brought up. It's not even clear
what problem making such policy changes would solve - you haven't
really identified any cases where they would help.

(Explicit written rules are only useful when relying on shared
understanding and common sense fails. They can also be useful for
communicating existing practices, but there are ways to do that don't
have the disadvantages of being phrased as rules.)

Gavin

[Boris Zbarsky]

On 4/18/12 5:16 PM, Ben Bucksch wrote:
> In my opinion, he should even be required to review it.

Please feel free to get your XBL and style system patches reviewed by
hyatt, your docshell changes reviewed by rpotts, your layout patches
reviewed by kipp or troy (or if you're lucky waterson), your DOM event
changes reviewed by joki, your HTTP networking changes reviewed by
darin, and your Firefox UI changes reviewed by blakeross and ben. Let
me know when you manage to get something checked in.

Seriously, that's not a workable requirement, for the vast majority of
our codebase.

> I didn't say that. "Last to touch" is very different from "author of
> most of the file".

And both are different from "most competent to review changes to the
file" and that is once again different from "most likely to provide the
highest quality review in finite time".

> I'd think the author of a file feels a higher level of responsibility
> and dedication than the owner of a whole module.

Bunk. See above for a list of file authors who don't feel all that much
responsibility and have not much remaining dedication to the code they
wrote. Except for maybe darin, I expect they feel precisely 0
responsibility.

Now can we stop trying to make blanket rules based on rare edge cases (a
file is well-owned by someone, the module owner does not know the file
that well, _and_ doesn't know who the person who knows it well is)?

Thanks,
Boris

[Justin Dolske]

On 4/18/12 2:39 PM, Ben Bucksch wrote:

>> We can all agree that having someone who wrote the code review patches
>> is often the most sensible solution; what I don't agree with is that
>> we need change the review rules to somehow enforce that.
>
> Can we agree that they should be allowed to review, as a general rule?

Anyone can review anything. People who have an interest in a particular
patch (be it due to technical knowledge or just a passing curiosity)
should feel free to do so [*]. It's a great way to learn new areas of
code and grow new reviewers.

But the term is overloaded -- the _important_ meaning is whose review is
sufficient in order to to land a patch, given Module Owner
rules/peers/delegation. In that sense, I'd firmly agree with others in
this thread. Just because a person has a file in the tree, that does not
and should not qualify them to capital-R review further changes.

By way of example: Suppose we receive a patch for a new feature from
Homer. Homer's a nice guy, but a bit of a bumbling idiot (doh). His
patch eventually lands, but it takes many review passes. He's not the
blurst programmer ever, but some suspect he just has a monkey banging on
a keyboard. Should Homer be able to capital-R review further changes to
the code all by himself?

Module owners already can (and have) allowed delegated reviews, so I'm
not sure what problem expanding this would solve.

Justin

[Mike Connor]

I'm not up to date on Gecko hacking, but at a quick skim that didn't seem so bad. Would you be able to identify the list of modules you feel are either not owned, or under-owned, and stick that on a wiki/etherpad? I'd be happy to carry the ball on an effort to ensure we have ownership of all pieces, but I don't think I'm best placed to assess where we need help.

-- Mike

[Ehsan Akhgari]

Absolutely! https://etherpad.mozilla.org/UnownedGeckoModules

--
Ehsan
<http://ehsanakhgari.org/>

[Kyle Huey]

> Absolutely! https://etherpad.mozilla.org/UnownedGeckoModules
>

Very little of this is unowned.

Core::Event Handling - This is the DOM module, and is owned by smaug
Core::File Handling - This is a strange component, but generally seems to
be docshell type stuff
Core::Find Backend (probably the same as typeaheadfind above) - This is
unowned
Core::Gecko Profiler - This doesn't have a module (we should create one!)
but is owned by BenWa et al. as I understand it.
Core::Hardware Abstraction Layer (HAL) - This also doesn't have a module
but should be owned by the device APIs people most likely
Core::HTML: Form Submission - This is DOM, and is owned by mounir
Core::History: Global - This is docshell
Core::Identity - This doesn't exist yet
Core::Image Blocking - This should die
Core::JavaScript Debugging APIs - This is the JS engine module
Core::Localization - Not sure what this is.
Core::Nanojit - Nanojit isn't in mozilla anymore, so this should go to the
Tamarin product or just die.
Core::Networking: Domain Lists - Gerv owns this, I believe.
Core::Print Preview - This is probably layout.
Core::Printing: Output - Ditto.
Core::Printing: Setup - Ditto.
Core::Profile: BackEnd - Not sure what module this is, but bsmedberg owns
it.
Core::Profile: Migration - This should probably die.
Core::Profile: Roaming - No idea what this is. The last bug in this
component was filed in 2008.
Core::QuickLaunch (AKA turbo mode) - This component should probably die.
Core::Rewriting and Analysis - Static analysis is kind of dead
Core::Selection - Not sure, but this is probably some combination of
DOM/Layout.
Core::Serializers - Maybe hsivonen owns this?
Core::Spelling checker - Don't you own this?
Core::Video/Audio - This is absolutely not unowned. We have an entire team
that works on this!
Core::Web Services - Pretty sure we removed this code a while back. This
component probably just needs to die.
Core::WebDAV - I don't know what this is, but the component has 4 open bugs.
Core::WebRTC - Stuff that doesn't exist yet
Core::WebRTC (Audio/Video) - Ditto
Core::WebRTC (Networking) - Ditto
Core::WebRTC (Signaling) - Ditto
Core::Widget: OS/2 - This is still alive and was maintained fairly recently
Core::Widget: Photon - This is dead and should be removed.
Core::X-remote - I don't know what this is.
Core::XForms - Is XForms still alive?
Core::XUL - There's a XUL module, and it has an owner ...
Core::jemalloc - This is owned by some combination of Justin Lebar, Mike
Hommey, and me.

Most of this stuff has owners, either on the module owners page or defacto
owners. It seems like your complaint is more that we don't have modules
for code that is owned?

- Kyle

[Ehsan Akhgari]

On Fri, Apr 20, 2012 at 3:53 PM, Kyle Huey <m...@kylehuey.com> wrote:

> Absolutely! https://etherpad.mozilla.org/UnownedGeckoModules
>>
>
> Very little of this is unowned.
>
> Core::Event Handling - This is the DOM module, and is owned by smaug
>

OK, then the modules page needs to be updated (same for the result of the
components similar to this).

> Core::File Handling - This is a strange component, but generally seems to
> be docshell type stuff
>

OK (but I find that very surprising!).

> Core::Find Backend (probably the same as typeaheadfind above) - This is
> unowned
> Core::Gecko Profiler - This doesn't have a module (we should create one!)
> but is owned by BenWa et al. as I understand it.
>

Agreed.

> Core::Hardware Abstraction Layer (HAL) - This also doesn't have a module
> but should be owned by the device APIs people most likely
> Core::HTML: Form Submission - This is DOM, and is owned by mounir
> Core::History: Global - This is docshell
> Core::Identity - This doesn't exist yet
>

It will, very soon. :-)

> Core::Image Blocking - This should die
>

Why?

> Core::JavaScript Debugging APIs - This is the JS engine module
> Core::Localization - Not sure what this is.
>

l10n issues, perhaps (and I guess owned by Axel)?

> Core::Nanojit - Nanojit isn't in mozilla anymore, so this should go to the
> Tamarin product or just die.
> Core::Networking: Domain Lists - Gerv owns this, I believe.
> Core::Print Preview - This is probably layout.
> Core::Printing: Output - Ditto.
> Core::Printing: Setup - Ditto.
> Core::Profile: BackEnd - Not sure what module this is, but bsmedberg owns
> it.
> Core::Profile: Migration - This should probably die.
> Core::Profile: Roaming - No idea what this is. The last bug in this
> component was filed in 2008.
> Core::QuickLaunch (AKA turbo mode) - This component should probably die.
>

Agreed (about QuickLaunch).

> Core::Rewriting and Analysis - Static analysis is kind of dead
> Core::Selection - Not sure, but this is probably some combination of
> DOM/Layout.
>

This is the code responsible for the window.getSelection() API and friends,
and things like caret placement and navigation.

> Core::Serializers - Maybe hsivonen owns this?
>

Maybe, I'd have thought bz, but in general this code has a lot of bugs and
is very unloved.

> Core::Spelling checker - Don't you own this?
>

No! ;-) While I have been reviewing patches in this components, I don't
know a lot about parts of it (although I've grown to know a lot of it I
guess.)

> Core::Video/Audio - This is absolutely not unowned. We have an entire
> team that works on this!
> Core::Web Services - Pretty sure we removed this code a while back. This
> component probably just needs to die.
> Core::WebDAV - I don't know what this is, but the component has 4 open
> bugs.
> Core::WebRTC - Stuff that doesn't exist yet
> Core::WebRTC (Audio/Video) - Ditto
> Core::WebRTC (Networking) - Ditto
> Core::WebRTC (Signaling) - Ditto
>

Again, they will, soon (and we have a team working on this I believe.)

> Core::Widget: OS/2 - This is still alive and was maintained fairly recently
> Core::Widget: Photon - This is dead and should be removed.
> Core::X-remote - I don't know what this is.
>

Neither do I!

> Core::XForms - Is XForms still alive?
>

I think we should ask surkov. AFAIK he's the closest person to an owner if
this has ever had one.

> Core::XUL - There's a XUL module, and it has an owner ...
> Core::jemalloc - This is owned by some combination of Justin Lebar, Mike
> Hommey, and me.
>
> Most of this stuff has owners, either on the module owners page or defacto
> owners. It seems like your complaint is more that we don't have modules
> for code that is owned?
>

I'm not sure if this was much of a complaint. :-) We should kill the
modules which have died, create new ones for those where we don't have
modules, denote the de facto owners for those who do have them as de jure,
and find owners for those which don't really have an owner. This just
needs some work, and I guess mconnor sort of volunteered for that.

[Majken Connor]

Put these comments on the etherpad? Easier to link to it than to this
discussion, then you can share it with more people to get more input on who
does/should/doesn't own what.

> _______________________________________________
> governance mailing list
> gover...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/governance
>

[Ehsan Akhgari]

Done.

--
Ehsan
<http://ehsanakhgari.org/>

[L. David Baron]

On Friday 2012-04-20 16:07 -0400, Ehsan Akhgari wrote:
> > Core::X-remote - I don't know what this is.
>
> Neither do I!

It's the thing that lets other apps open a browser window on
X11-based systems. (There's a protocol and a command-line client
for that protocol, and Firefox also supports command-line
invocations of the protocol.)

I suppose it could be merged into one of the widget modules; it's
used under both GTK and QT configurations.

-David

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

[Henri Sivonen]

On Wed, Apr 18, 2012 at 11:46 PM, Ben Bucksch
<ben.buck...@beonex.com> wrote:
> It can also even more easily argued that the
> content/ module owner is not familiar with all the particular details of
> nsPlaintextSerializer.cpp. He might not even understand the different
> operation modes it has. He might approve an innocent-looking change, but
> that breaks either copy&paste or rich plaintext output badly, because
> neither contributor nor content/ module owner even knew that it existed or
> had this whitespace particularity.

So https://bugzilla.mozilla.org/show_bug.cgi?id=650784 caused
https://bugzilla.mozilla.org/show_bug.cgi?id=731896 . I think it would
have been unreasonable to stall work on the first bug pending review
from Akkana when it's been years since she left the project and
there's no indication that she even reads bugmail. Furthermore, the
cause of the second bug wasn't any change in nsPlainTextSerializer.cpp
but a change in the the input given to nsPlainTextSerializer.cpp.
Expertise in the workings of nsPlainTextSerializer.cpp would not have
prepared a reviewer to catch the regression.

Besides, testing on Nightly worked like it should and the regression
got fixed before beta. It's a better outcome to regress and fix the
regression ahead of release than to be so afraid of regressions that
no work gets gone in a timely fashion.

On Thu, Apr 19, 2012 at 12:02 AM, Gavin Sharp <ga...@gavinsharp.com> wrote:
> Are you referring to some specific incident?

https://bugzilla.mozilla.org/show_bug.cgi?id=650784 and
https://bugzilla.mozilla.org/show_bug.cgi?id=650776 are probably
relevant for context.

On Thu, Apr 19, 2012 at 12:36 AM, Gijs Kruitbosch
<gijskru...@gmail.com> wrote:
> - this undermines the module owner / peers' authority over the module. It
> implies that once you create a file, you decide what happens to it. That
> wouldn't scale well and runs counter to my (and, I suspect, others')
> intuition of how the review system works.

Indeed.

On Thu, Apr 19, 2012 at 12:57 AM, Gavin Sharp <ga...@gavinsharp.com> wrote:

> On Wed, Apr 18, 2012 at 2:39 PM, Ben Bucksch
> <ben.buck...@beonex.com> wrote:
>> Can we agree that they should be allowed to review, as a general rule?
>

> It depends on what you mean by "allowed to".

It's relevant to also consider being "allowed to" say "r-" in a way
that blocks a patch from landing. See
https://bugzilla.mozilla.org/show_activity.cgi?id=650776

On Thu, Apr 19, 2012 at 2:44 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
> Please feel free to get your XBL and style system patches reviewed by hyatt,
> your docshell changes reviewed by rpotts, your layout patches reviewed by
> kipp or troy (or if you're lucky waterson), your DOM event changes reviewed
> by joki, your HTTP networking changes reviewed by darin, and your Firefox UI
> changes reviewed by blakeross and ben. Let me know when you manage to get
> something checked in.

More to the point: When someone else on whose patches such review
requests are made manages to get something landed. See
https://bugzilla.mozilla.org/show_activity.cgi?id=650784

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