I recently posted in the newsgroups about a concern over super-review. In some cases patches that seem to meet the policy aren’t getting super-reviewed. Part of the problem here is that the policy is a little ambiguous. It says that any API or pseudo-API requires super-review but depending on how you read that section it could mean any patch that changes the signature of a JS function is classed as an API. We need to be smarter than that. Here is a straw-man proposal for defining what is an API:
Any code that is intended to be used by other parts of the application, add-ons or web content is an API and requires super-review when added or its interface is changed
Some concrete examples to demonstrate what I think this statement covers:
- JS modules and XPCOM components in toolkit are almost all intended to be used by applications or add-ons and so are APIs
- UI code in toolkit (such as extensions.js) aren’t meant to be used elsewhere and so aren’t APIs (though they may contain a few cases such as observer notifications, these should be clearly marked out in the file)
- Any functions or objects exposed to web content are APIs
- The majority of code in browser/ is only meant to be used within browser/ and so isn’t an API. There are some exceptions to this where extensions rely on certain functionality such as tabbrowser.xml
Do you think my simple statement above matches those cases and others that you can think of? Is this a good way to define what needs super-review?
I think this wouldn’t cover some things that should be. Under these rules, tabbrowser.xml wouldn’t be an API, since that’s (originally), err, somewhere in XPFE. And IIRC it had all sorts of cruft in it, like the only accessor to the tabstrip being mStrip (which certainly didn’t sound like an API, but people used it anyway). Also, the fun that is bootstrapped extension’s reason codes…
It might be more useful to have a SR-light option for some things, instead; don’t worry about the code, make sure the interfaces are solid. Make it go faster so it’s not as much of an effort (especially since you’d need r? anyway, which should take longer as that has to look at the actual code changes).
Sorry if this is a dupe, the comment entry form ate my comment and now I have to make up a similar one :p
I think the more interesting question is not what an API is (let’s just assume something broad), but rather what kinds of APIs need extra review, and to what degree.
For example… A content-facing API should probably have the strictest review requirements. There are questions about interop, security, standardization, efforts by other vendors, etc. Getting the API wrong means we and the web are stuck with it, or we break things when we fix/remove it. OTOH, when adding a simple property to some existing obscure internal API, the risk is very low and requiring a superreview just extra process (overkill). In between is a spectrum of grays (at least 50 shades, ahem).
I don’t think it’s fair to end-developers to make them understand the nuances of these issues and deal with the uncertainties of “is this an API” or “does this need superreview”. But we do have a pool of reviewers we can work with on these issues, and get to some general understanding in terms of goals instead of detailed rules.