Super-review, what shall we do with you?

[Dave Townsend]

We've had a policy requiring super-review for certain kinds of patches
for a long time. It's changed a couple of times but the current policy
(http://www.mozilla.org/hacking/reviewers.html) primarily requires
super-review for any patch that introduces or changes an API. Basically
any function in a JS file or JSM is covered here, or at least that is my
reading of it. The reasoning is pretty straightforward, designing APIs
well up front reduces the maintenance burden in the future and hopefully
means we don't have to make changes that break add-ons.

The problem is that I frequently come across patches that were landed
without super-review despite meeting the requirements. This suggests
that many reviewers aren't aware of the policy or don't have the same
interpretation of it as I do. We probably need to do a better job of
making sure that all reviewers in particular understand the policy and
are following it.

But, I haven't yet seen an issue arise from this lack of SR. Does that
mean that the policy is too restrictive and we need to change it to more
closely match how reviewers are working?

Either we're setting ourselves up for big problems down the road or we
have a policy that is in some cases hindering development. Those are the
extremes of course and the reality is probably somewhere in between, but
I'd like to hear other people's thoughts about this.

[Dave Townsend]

Note, please post replies in mozilla.dev.platform.

[Dave Townsend]

On 11/06/12 10:09, Dave Townsend wrote:

I've made a blog post suggesting a change to the definition of what an
API is that requires super-review. I'd appreciate it if people read it
and gave me some feedback:
http://www.oxymoronical.com/blog/2012/11/What-is-an-API

[Ehsan Akhgari]

On 2012-11-23 2:29 PM, Dave Townsend wrote:
> I've made a blog post suggesting a change to the definition of what an
> API is that requires super-review. I'd appreciate it if people read it
> and gave me some feedback:
> http://www.oxymoronical.com/blog/2012/11/What-is-an-API

I don't think it makes a lot of sense to require super-review for
patches which implement web facing features which are spec'ed, since in
those cases we're really implementing APIs defined by the spec.


Speaking in a bit broader sense, I'm not really convinced that there is
actually a problem to be solved in the way that we currently do things
(defer whether or not a super-review is required to the author's and
reviewer's best judgement). And requiring super-review on every patch
which modifies an API in the sense that is defined in your blog post
will mean that a ton of the patches which are currently being landed
will require a round of super-reviews, which means more review requests
to the list of super-reviewers (which has been pretty much static for
the past few years) and slower turn-around times...

Cheers,
Ehsan

[Alexander Surkov]

It doesn't make an exception for specific areas like accessibility. It
seems accessibility module never had super reviewers in peers and it
seems we never followed the rule "super review for API change". It
sounds like this rule adds unnecessary complexity for this module.

Thank you.
Alexander.

[Mook]

On 11/23/2012 3:42 PM, Ehsan Akhgari wrote:
> On 2012-11-23 2:29 PM, Dave Townsend wrote:
>> I've made a blog post suggesting a change to the definition of what an
>> API is that requires super-review. I'd appreciate it if people read it
>> and gave me some feedback:
>> http://www.oxymoronical.com/blog/2012/11/What-is-an-API
>

<snip>

>
> Speaking in a bit broader sense, I'm not really convinced that there is
> actually a problem to be solved in the way that we currently do things
> (defer whether or not a super-review is required to the author's and
> reviewer's best judgement). And requiring super-review on every patch
> which modifies an API in the sense that is defined in your blog post
> will mean that a ton of the patches which are currently being landed
> will require a round of super-reviews, which means more review requests
> to the list of super-reviewers (which has been pretty much static for
> the past few years) and slower turn-around times...
>

Sounds like it would be useful to get more super-reviewers on board and
get faster turnaround times, then. Or switch API review to a superset
of SRs, if it's really just there to get a second set of eyes on it.
Given the, umm, interesting quality of some of the new-to-me code, it
seems like having better oversight of at least the inter-module bits
would be helpful. See also the thread from September about comments - I
honestly would have expected SRs to catch that. (The interface given as
an example in that message has no comments, and no sr= marking of any sort.)

--
Mook

[Kyle Huey]

On Nov 23, 2012 3:43 PM, "Ehsan Akhgari" <ehsan....@gmail.com> wrote:
>
> On 2012-11-23 2:29 PM, Dave Townsend wrote:
>>

>> I've made a blog post suggesting a change to the definition of what an
>> API is that requires super-review. I'd appreciate it if people read it
>> and gave me some feedback:
>> http://www.oxymoronical.com/blog/2012/11/What-is-an-API
>
>

> I don't think it makes a lot of sense to require super-review for patches
which implement web facing features which are spec'ed, since in those cases
we're really implementing APIs defined by the spec.

I won't go so far as to claim that superreview is the best way to do this,
but web specs do need careful API review before we implement them. The sr
requirement does help there.

> Speaking in a bit broader sense, I'm not really convinced that there is
actually a problem to be solved in the way that we currently do things
(defer whether or not a super-review is required to the author's and
reviewer's best judgement). And requiring super-review on every patch
which modifies an API in the sense that is defined in your blog post will
mean that a ton of the patches which are currently being landed will
require a round of super-reviews, which means more review requests to the
list of super-reviewers (which has been pretty much static for the past few
years) and slower turn-around times...
>

> Cheers,
> Ehsan
>
> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning

- Kyle

[Mounir Lamouri]

On 23/11/12 23:42, Ehsan Akhgari wrote:
> I don't think it makes a lot of sense to require super-review for
> patches which implement web facing features which are spec'ed, since in
> those cases we're really implementing APIs defined by the spec.

Blindly implementing Web API isn't a good practice. We very often
propose changes to specifications because we found issues while
implementing.

In practice, I think most DOM peers are super-reviewers which means that
most of the time I don't really bother asking a super-review unless I
really feel like we need double-checking the change.

However, I think super-reviews a generally a good tool. With the Firefox
OS effort, it allowed us to clearly differentiate the API review with
the implementation reviews.

Cheers,
--
Mounir

[Dave Townsend]

On 11/23/12 16:52, Alexander Surkov wrote:
> It doesn't make an exception for specific areas like accessibility. It
> seems accessibility module never had super reviewers in peers and it
> seems we never followed the rule "super review for API change". It
> sounds like this rule adds unnecessary complexity for this module.

I'm really only talking about the API review part of the super-review
rules here. The listed exceptions would still apply. Interestingly
though accessibility isn't currently listed as an exception to the
super-review rules.

[Dave Townsend]

It's a bit difficult to draw a consensus from everyone here but mostly
it seems that people think the status quo with module owners and
reviewers making their judgement is working out here. I'm going to email
all the module owners directly and ask that they look over the policy
and communicate with their peers about what their expectations are.