Adopting the mozilla-central super-review policy in comm-central
Mark Banner
We have been discussing adopting the mozilla-central super-review policy
within comm-central [1]. The current policy is here:
http://www.mozilla.org/hacking/reviewers.html
We are proposing to adopt it in at least editor/, directory/, mailnews/
and mail/.
The outcome of the discussion so far is that:
- We'd have a reduction in small patches that require sr.
- sr would be focussed more towards patches touching APIs.
- Overall we'd expect workload on srs to be reduced.
- A person doing a review where the patch doesn't require sr can always
divert the review or request additional review if they feel it is
necessary (e.g. non-trivial patch or inexperience).
- There may be some added risk of regressions due to not having an sr,
some of which will be offset by the change in test policies.
Open questions:
- Does calendar/ want inclusion or exclusion in this policy?
- Does suite/ want inclusion or exclusion in this policy? Karsten said:
> Note that /suite/mailnews even has a stricter policy than the rest of
> /suite, and I don't think this will change
> (<http://www.seamonkey-project.org/dev/review-and-flags>).
Comments, thoughts and answers to the questions welcome.
Mark.
[1] http://groups.google.com/group/tb-planning/t/20ae27bb9f85e347
Jens Hatlak
> We have been discussing adopting the mozilla-central super-review policy
> within comm-central [1]. The current policy is here:
>
> http://www.mozilla.org/hacking/reviewers.html
> We are proposing to adopt it in at least editor/, directory/, mailnews/ and mail/.
I won't comment on the change for those directories, since, being a
front-end guy, it'll hardly affect me. I'll just respect whatever is
decided if applicable to changes I make. Still I'd like to reply to this
question:
> - Does suite/ want inclusion or exclusion in this policy? Karsten said:
>
>> Note that /suite/mailnews even has a stricter policy than the rest of
>> /suite, and I don't think this will change
>> (<http://www.seamonkey-project.org/dev/review-and-flags>).
suite/mailnews aside, that page contains the following sentence:
"sr= is required for all UI changes (note that for small UI changes it
may happen to be faster to get r+sr from a super reviewer)."
This directly contradicts the following sentence from the new policy:
"This means that one reviewer cannot provide both review and
super-review on a single patch."
If you ask me, SeaMonkey should do like Thunderbird and stop misusing
the sr flag for the purpose of the ui-review flag. With that, r+uir (or
r+u-ir?) for a single patch should be perfectly in line with the new policy.
Coming back to suite/mailnews, I'd appreciate if the sr requirement
would then be adapted to the above:
- simple SM MailNews patches that affect UI need r+ui-r
- all other SM MailNews patches need r+sr
That might sound like only adding complexity if you keep in mind that
currently Neil is our only super-reviewer and UI tzar. But I really
think that thinking/ambiguity needs to go away, and this is the first
step toward decoupling.
Greetings,
Jens
--
Jens Hatlak <http://jens.hatlak.de/>
SeaMonkey Trunk Tracker <http://smtt.blogspot.com/>
Justin Wood (Callek)
> [follow-ups set to mozilla.governance]
>
> We have been discussing adopting the mozilla-central super-review policy
> within comm-central [1]. The current policy is here:
>
> http://www.mozilla.org/hacking/reviewers.html
>
> We are proposing to adopt it in at least editor/, directory/, mailnews/
> and mail/.
>
> The outcome of the discussion so far is that:
>
> - We'd have a reduction in small patches that require sr.
> - sr would be focussed more towards patches touching APIs.
> - Overall we'd expect workload on srs to be reduced.
> - A person doing a review where the patch doesn't require sr can always
> divert the review or request additional review if they feel it is
> necessary (e.g. non-trivial patch or inexperience).
> - There may be some added risk of regressions due to not having an sr,
> some of which will be offset by the change in test policies.
>
> Open questions:
>
> - Does calendar/ want inclusion or exclusion in this policy?
>
> - Does suite/ want inclusion or exclusion in this policy?
My feeling here is, "yes". But should probably discussed tangential to
this thread, and initially with the entire SeaMonkey-Council, before
opening broader.
--
~Justin Wood (Callek)
Siddharth Agarwal
> I won't comment on the change for those directories, since, being a
> front-end guy, it'll hardly affect me.
Hm, do XUL elements acting as extension points count as "API"? I'd
presume so.
Dan Mosedale
is an API in a certain sense. It's not at all clear to me that we want
sr for every front-end touching change, however. Part of our longer
term solution to this problem is via Jetpack, where we define a smaller,
simpler set of APIs that we have a much stronger commitment to, and
those would definitely count. In the short, it's not obvious to me how
we can draw a bright line here that makes sense, so I'm inclined to
leave it as a judgement call for the reviewer. Other thoughts?
Dan
Mark Banner
That's a good point. Certainly difficult. I would expect that sr would
apply more in situations where we know there's likely a significant
extension impact, obviously that's hard to categorise. One example could
be if I changed the openTab function for content tabs - that's used by a
few extensions now I believe, so having a sr on that would probably be a
good idea.
Having something like jetpack would make it simpler though ;-)
Mark.
Mark Banner
> [follow-ups set to mozilla.governance]
>
> We have been discussing adopting the mozilla-central super-review policy
> within comm-central [1]. The current policy is here:
>
> http://www.mozilla.org/hacking/reviewers.html
>
> We are proposing to adopt it in at least editor/, directory/, mailnews/
> and mail/.
Based on the current responses, I will be filing a bug on Monday to
update the policy and officially put this into effect.
calendar/ and suite/ will be excluded from the policy but can always be
incorporated later if their owners wish it so.
Mark.
Philipp Kewisch
wrote:
> [follow-ups set to mozilla.governance]
I think for calendar/ this policy doesn't make sense, since we don't
have many reviewers. We've been setting a soft policy in the past,
that certain changes need more than one reviewer (i.e calendar/
providers/storage), but I wouldn't be so strict as to require a sr for
it.
I guess you figured this already ;-)
Philipp
Robert Kaiser
> calendar/ and suite/ will be excluded from the policy but can always be
> incorporated later if their owners wish it so.
We will likely have a SeaMonkey dev meeting later this year and probably
will talk this over along with the general topic of code management and
reviewing rules.
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 needs answers to. And most of the time,
I even appreciate irony and fun! :)