On 11/6/2012 1:09 PM, Dave Townsend wrote:
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?
As the primary author of the previous requirements, let me dump some
knowledge about the original intent, and then let the discussion go
where it may. I don't think I have particularly strong opinions about it.
* At the time, super-review was required for *all* patches.
* Super-review was a burden for super-reviewers and slowed the project down.
* There was a sense that our process has historically been very good at
detailed code review, and historically very spotty at "design" review.
* What super-reviewers really wanted to do was apply their
cross-functional knowledge and experience to the issues where that
experience would be useful, without slowing down the more mundane
bugfixing aspects of the project.
* At the time (the Firefox 3 beta period, IIRC), we had recently
"discovered" how disruptive some JS changes could be. Specifically,
extensions rely on some functions in browser.js and in the XBL bindings
for <browser> and <tabbrowser> extensively, and even minor changes in
their function could have unexpected consequences. So we didn't want to
limit the policy to "IDL-defined XPCOM APIs" but rather recognize that
there are some very important APIs in JavaScript. This was before JSMs
even existed, IIRC.
The wording of the current policy wanted to capture these general
sentiments by requiring integration and API review. We talked at length
about whether to rename it to "design/integration review".
Superreviewers were asked to specifically look at the following aspects
of the code:
* Understanding the interactions between modules and making sure that we
weren't introducing technical debt with too-tight couping, but also
weren't going architecture-astronaut and designing too-general APIs.
* Understand the ways that changes affect addon compatibility and make
value judgements about whether breaking changes were worthwhile.
* Apply experience to APIs to try and bring consistency
* Superreviewers agreed that for the most part they were not going to do
line-by-line code review on patches.
Some of these functions may have since been taken over by other groups.
The `addon-compat` keyword, for example, may solve some of the problems
related to extension-facing APIs. I do think we still aren't always as
deliberate as we should be about the cost/reward of changing/maintaining
addon APIs, and the keyword is mainly used as a notification for "we
broke something; Jorge please help addons clean up after us".
It was never our intent that "pseudo APIs" was meant to cover all JS
code, and there is certainly lots of reviewer judgement involved whether
a particular new function is just implementation, or an API, or right
now an implementation that extension might force to become an API in the
future.
--BDS
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
