Proposed changes to super-review process for mozilla-central
Mike Connor
there have been questions about the idea of super-review, and where it
is important and adds value. The super-reviewers group have discussed
this and reached a consensus, and it's time to get wider feedback.
The core principles behind these changes are that:
* The super-reviewers group is capable of making the biggest impact in
the areas of API and code design, security bugs, and testability/
maintainability
* All of the code in mozilla-central can benefit from this type of
review and guidance
* Module owner review, assuming strong ownership and ever-growing test
coverage, should be sufficient for many patches.
The proposal we have is that super-review will now be required for any
patches to mozilla-central meeting any of the following criteria:
* any significant architectural refactoring
* any change in how Mozilla code modules interact
* any change to an API for extensions
* any change to an API for web content
* security changes
Module owners/peers may ask for a super-review in other cases not
explicitly covered here, like any other conditional grant of review.
Any patch not meeting the above criteria does _not_ require super-
review under this proposal, even in areas where super-review has
previously been required for all patches.
IMPORTANT: This proposal covers browser and toolkit, which previously
had no requirement for super-review.
Feedback is strongly encouraged, as we'd like to move this forward as
soon as possible.
-- Mike
Dave Townsend
my life more difficult as previously most of my patches would not
require sr, I believe the additional overview should help us as a whole.
I'd like to see some clarification though on exactly what "API" covers,
particularly for the extensions case. Are we just talking about xpcom
interfaces, or XBL bindings, or down to the level of objects and
functions in common window scopes?
Justin Wood (Callek)
> I'd like to see some clarification though on exactly what "API" covers,
> particularly for the extensions case. Are we just talking about xpcom
> interfaces, or XBL bindings, or down to the level of objects and
> functions in common window scopes?
Do ID changes cover extension case? etc.
I just anticipate the API cases being vague for some causing people to
think they need sr+ when they don't and others not realizing xyz
actually changes API and needs it, but never requests it.
I feel the ambiguous nature may even strike some Module Owners.
Speaking of, based on the above I get the impression that I could (in
most cases) ask the Module Owner for review and never need an sr.
--
~Justin Wood (Callek)
Boris Zbarsky
> I'd like to see some clarification though on exactly what "API" covers,
> particularly for the extensions case. Are we just talking about xpcom
> interfaces, or XBL bindings, or down to the level of objects and
> functions in common window scopes?
My take on this, for what it's worth, is that this is the module owner's
call.... This does mean that module owner review needs to precede sr,
but that's a good thing in that it'll keep sr focused on the issues Mike
described instead of trying to sort out the code details.
-Boris
Justin Dolske
> I'd like to see some clarification though on exactly what "API" covers,
> particularly for the extensions case. Are we just talking about xpcom
> interfaces, or XBL bindings, or down to the level of objects and
> functions in common window scopes?
That was my first question too. I think it would help to know what the
intent here is.
Do we really need sr+ for, literally, "any change to an API"? Even
changes that are otherwise small and uninteresting?
Justin
Mark Banner
Having to wait for the module owner to decide seems to me like it will
delay patches getting into the tree - if the requester asks for review
from a module peer, then they will also have to find out what the owner
thinks about sr. If they request review from the module owner, then the
module owner has to remember to say if sr is required or not, and I'd
expect this would push more load towards the module owners anyway.
If all module owners are superreviewers then this isn't too bad, the
requesters can just ask for sr from the module owners. Although I still
see this as putting unnecessary load for an "uncertain" case.
Standard8
timeless
> Having to wait for the module owner to decide seems to me like it will delay
> patches getting into the tree
my patches are already delayed by years. i don't see this as a problem.
you could write tests and contact all the people you're going to break
while you wait for the module owner.
and write a nice MDC article and a post to m.d.<something> announcing
your API change
> if the requester asks for review from a
> module peer, then they will also have to find out what the owner thinks
> about sr. If they request review from the module owner, then the module
> owner has to remember to say if sr is required or not, and I'd expect this
> would push more load towards the module owners anyway.
>
> If all module owners are superreviewers then this isn't too bad, the
> requesters can just ask for sr from the module owners. Although I still see
> this as putting unnecessary load for an "uncertain" case.
one would hope that a peer could also tell you to get an sr ....
Boris Zbarsky
> Having to wait for the module owner to decide seems to me like it will
> delay patches getting into the tree - if the requester asks for review
> from a module peer
s/module owner/module owner or peer/ in my mail, sure. Whoever's doing
the r= needs to be able to make the sr call, imo.
-Boris
Dave Townsend
I think certainly whoever does the main review should always be able to
suggest that an sr is necessary, but a good definition of what an API
change is would help tell the patcher they need sr up front, probably
speeding the process, and also help guide the peers on when they should
be requesting the sr.
Dave Townsend
> On Fri, Jan 2, 2009 at 12:02 PM, Mark Banner
>> Having to wait for the module owner to decide seems to me like it will delay
>> patches getting into the tree
>
> my patches are already delayed by years. i don't see this as a problem.
Well clearly if that is the case we shouldn't care about speeding up any
of our processes.
> you could write tests and contact all the people you're going to break
> while you wait for the module owner.
Patches should already include tests before requesting review ideally.
And identifying all the people you are going to break is of course not
reasonable.
> and write a nice MDC article and a post to m.d.<something> announcing
> your API change
Great, obviously can't post, at least not finally until the review is
complete in case changes are necessary.
Benjamin Smedberg
> I'd like to see some clarification though on exactly what "API" covers,
> particularly for the extensions case. Are we just talking about xpcom
> interfaces, or XBL bindings, or down to the level of objects and
> functions in common window scopes?
All of the above. Increasingly I think we should be thinking of APIs in
terms of what's reflected into JavaScript, *not* XPCOM interfaces. Things
reflected into global window scopes, such as the various functions in
utilityOverlay.js should definitely be getting an API review in addition to
code review.
--BDS
mco...@mozilla.com
I think I'm leaning towards Benjamin's definition of APIs, which will
definitely impact many patches in browser/toolkit. That's an expected
outcome, FWIW, but a lot of that will fall on my shoulders, at least
in the short term, and it shouldn't be a major obstacle.
-- Mike
> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning
timeless
> I think I'm leaning towards Benjamin's definition of APIs, which will
> definitely impact many patches in browser/toolkit. That's an expected
> outcome, FWIW, but a lot of that will fall on my shoulders, at least in the
> short term, and it shouldn't be a major obstacle.
while we're talking about APIs.
would changing an unfrozen API like nsICookieService which a major OS
vendor decided to use have been covered by the requirement for sr?
:)
Shawn Wilsher
> while we're talking about APIs.
>
> would changing an unfrozen API like nsICookieService which a major OS
> vendor decided to use have been covered by the requirement for sr?
>
/sdwilsh
Justin Wood (Callek)
Yea I think its safe to say that *.idl would qualify without question here.
That is not to say that just because it is an *.idl we should freeze it,
I would disagree there too.
--
~Justin Wood (Callek)
Dan Mosedale
> Yep, of course module peers can make that assessment.
>
> I think I'm leaning towards Benjamin's definition of APIs, which will
> definitely impact many patches in browser/toolkit. That's an expected
> outcome, FWIW, but a lot of that will fall on my shoulders, at least in
> the short term, and it shouldn't be a major obstacle.
>
I'm mildly concerned that this reverts some of the gain / ability to
iterate quickly that the existing browser/toolkit review policies were
created to provide. That said, Gecko is in a very different place now
than it was then, so perhaps that's the right thing to do.
Dan
Dan Mosedale
>
> I think I'm leaning towards Benjamin's definition of APIs, which will
> definitely impact many patches in browser/toolkit. That's an expected
> outcome, FWIW, but a lot of that will fall on my shoulders, at least in
> the short term, and it shouldn't be a major obstacle.
>
I'm mildly concerned that this reverts some of the gain / ability to
Benjamin Smedberg
When FF/toolkit were getting off the ground (pre 0.9 or so) it was all new
code, and a skunkworks project in many ways. If we were doing FF/toolkit
now, it would probably still be done as an incubator repository and wouldn't
be subject to SR. This is for example how the new embedding harness works,
and to some extent Fennec.
We don't particularly want to impede new UI features (many of which have no
API impact), nor bug fixing. What we do want is to make sure that changes
that cross modules are designed, and don't impose high costs later.
--BDS
Dan Mosedale
> We don't particularly want to impede new UI features (many of which have no
> API impact), nor bug fixing.
This is what confuses me. My interpretation of the rest of this thread
(and my intuition as well) is that huge chunks of the UI are implicitly
APIs that extensions depend on, and they would be covered by this
policy. Am I misinterpreting?
Dan
Benjamin Smedberg
Hrm. Just because extensions could depend on any particular detail of our UI
doesn't mean that it should be considered an "API". The particular IDs or
locations of menu items are not, IMO, an API, even though extension authors
depend on them to get overlay positions correct.
The documented behavior of <browser> or <tabbrowser> is an API: changes to
it should have design review by an sr.
The internal implementation of the <browser> or <tabbrowser> is not an API:
that is, there are internal _methods() and an internal XBL content structure
used to implement the documented API.
The line is going to be blurry in some cases. Are you worried about the sr
requirement slowing down development in these cases?
--BDS
Justin Wood (Callek)
> On 1/6/09 1:30 PM, Dan Mosedale wrote:
>> On 1/5/09 11:00 AM, Benjamin Smedberg wrote:
>>> We don't particularly want to impede new UI features (many of which
>>> have no
>>> API impact), nor bug fixing.
>> This is what confuses me. My interpretation of the rest of this thread
>> (and my intuition as well) is that huge chunks of the UI are implicitly
>> APIs that extensions depend on, and they would be covered by this
>> policy. Am I misinterpreting?
>
> Hrm. Just because extensions could depend on any particular detail of our UI
> doesn't mean that it should be considered an "API". The particular IDs or
> locations of menu items are not, IMO, an API, even though extension authors
> depend on them to get overlay positions correct.
>
> The documented behavior of<browser> or<tabbrowser> is an API: changes to
> it should have design review by an sr.
>
> The internal implementation of the<browser> or<tabbrowser> is not an API:
> that is, there are internal _methods() and an internal XBL content structure
> used to implement the documented API.
Ahh I did not get that impression from the rest of this thread.
But surely toolkit/ widgets (XBL-wise) are easily acknowledged as API.
Do we consider contentUtils.js API or browser internal?
At what level of abstraction do we want to quantify API?
> The line is going to be blurry in some cases. Are you worried about the sr
> requirement slowing down development in these cases?
Can't we error on the side of "solid line" and let rarer blurry cases
reside on reviewers discretion instead of "blurry line by default"
causing more false requests and added burden on patch author, reviewers
and sr itself?
--
~Justin Wood (Callek)
Shawn Wilsher
> Can't we error on the side of "solid line" and let rarer blurry cases
> reside on reviewers discretion instead of "blurry line by default"
> causing more false requests and added burden on patch author,
> reviewers and sr itself?
should be sr'd. Yeah, the patch author might not know, so there could
be a bit more time added to the review process if they didn't think it
needed sr.
I don't think it's a good idea to make black and white rules in a world
full of shades of gray.
/sdwilsh
timeless
> I would really really hope that module owners/peers know when something
> should be sr'd. Yeah, the patch author might not know, so there could be a
> bit more time added to the review process if they didn't think it needed sr.
it's an interesting wish, but history has shown that even for idl
things, we haven't really thought much about it.
Benjamin Smedberg
> But surely toolkit/ widgets (XBL-wise) are easily acknowledged as API.
> Do we consider contentUtils.js API or browser internal?
They are functions at global scope in the browser chrome which extensions
use. Whether we want them to be API or not, I think they are de-facto APIs
that extensions use.
>> The line is going to be blurry in some cases. Are you worried about
>> the sr
>> requirement slowing down development in these cases?
>
> Can't we error on the side of "solid line" and let rarer blurry cases
> reside on reviewers discretion instead of "blurry line by default"
> causing more false requests and added burden on patch author, reviewers
> and sr itself?
If we had to make a solid line, I think we'd err more on the side of
"everything is an API". But I think it's better to let common sense reign
than try to codify everything.
--BDS
Mike Connor
On Jan 7, 2009, at 11:44 AM, Benjamin Smedberg wrote:
> If we had to make a solid line, I think we'd err more on the side of
> "everything is an API". But I think it's better to let common sense
> reign
> than try to codify everything.
I think we'll actually evolve an understanding over time of what does
and doesn't need review. Best to err on the side of caution in the
short term, of course.
People should remember that, in general, the SR step is to review the
changes that require SR. It isn't necessarily "review the entire
patch" as much as "review the parts that need SR." This means that,
in practice, SR will often be a fraction of the total review step, so
we should be able to keep up in a timely fashion.
-- Mike
Michael Connor
On 7-Jan-09, at 12:37 PM, Mike Connor wrote:
>
> On Jan 7, 2009, at 11:44 AM, Benjamin Smedberg wrote:
>
>> If we had to make a solid line, I think we'd err more on the side of
>> "everything is an API". But I think it's better to let common sense
>> reign
>> than try to codify everything.
>
> I think we'll actually evolve an understanding over time of what
> does and doesn't need review. Best to err on the side of caution in
> the short term, of course.
>
> People should remember that, in general, the SR step is to review
> the changes that require SR. It isn't necessarily "review the
> entire patch" as much as "review the parts that need SR." This
> means that, in practice, SR will often be a fraction of the total
> review step, so we should be able to keep up in a timely fashion.
Okay, I think there was pretty broad consensus on this change. I
think we should go ahead and make this change starting as of March
15th (to give me time to get the edits to the policy docs in place).
I'll try to get those done in the next two days, so we can have an
edit pass etc.
Thanks to everyone who commented.
-- Mike
Dave Townsend
I see no announcement or new policy docs so I assume the changes aren't
in place yet?