Re: Super-review, what shall we do with you?
On 11/23/12 11:29, Dave Townsend wrote: On 11/06/12 10:09, 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? 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. 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 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. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On Tuesday 2012-12-04 10:00 +, Neil wrote: Blake Kaplan wrote: Neil n...@parkwaycc.co.uk wrote: static const PRUnichar* kResetBackupDirectory = NS_LITERAL_STRING(resetBackupDirectory).get(); Isn't this an anti-pattern anyway because the string (and the memory owned by it) will get destructed after the semicolon runs leaving kResetBackupDirectory pointing at deleted memory? Yes, this is why I said it is technically incorrect, but we use a string literal (L or u) where we can; only on systems that don't support UTF16 string literals do we allocate memory. If no such systems remain, we should really switch to NS_L(resetBackupDirectory) throughout. But if we want to use NS_L widely, we should first rename it to have a name that looks more like u (which is what it does, since it always produces 2-byte characters) than L. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla http://www.mozilla.org/ 턂 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On a side note, what can we do about checking for unusually verbose or inefficient constructs? Examples: static const PRUnichar* kResetBackupDirectory = NS_LITERAL_STRING(resetBackupDirectory).get(); This is technically incorrect on systems that don't support a 16-bit char type (short wchar_t or char16_t). (When can we require a real UTF16 type?) nsString type = NS_LITERAL_STRING(npapi-carbon-event-model-failure); This is very inefficient because the string gets copied into an allocated buffer. Both of the above could have either used NS_NAMED_LITERAL_STRING or been inlined into the single use. nsCOMPtrnsIVariant d(already_AddRefednsIVariant(XPCVariant::newVariant(ccx, v))); We have a templated function that saves you from needlessly retyping the type: nsCOMPtrnsIVariant d(dont_AddRef(XPCVariant::newVariant(ccx, v))); ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On 12/1/2012 4:28 PM, Neil wrote: On a side note, what can we do about checking for unusually verbose or inefficient constructs? Examples: We could create a compiler plugin that examines the AST for known badness. See bug 733873. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On 12/1/12 4:28 PM, Neil wrote: On a side note, what can we do about checking for unusually verbose or inefficient constructs? Examples: I don't think this has anything to to with sr policy, nor should it. Justin ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On 2012-11-26 7:17 AM, smaug wrote: As a reviewer and someone who cares about quality, this annoys me because I know it is something that could largely be solved through decent automation and tools. Yes. We certainly should have at least coding style checker, and uuid update checker. https://bugzilla.mozilla.org/show_bug.cgi?id=813809 Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
Justin Dolske schrieb: I think we should consider jettisoning/rewriting that part of the policy. It doesn't match what we've been doing in reality(*) Yes, that's why we almost f***ed up 17.0 and needed to do a last-minute reversion of patches that changed IIDs while on beta, for example. If we wouldn't have caught that problem with applying proper sr, our problem lies deeper, of course. Robert Kaiser ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On Monday 2012-11-26 04:21 +0100, Robert Kaiser wrote: Justin Dolske schrieb: I think we should consider jettisoning/rewriting that part of the policy. It doesn't match what we've been doing in reality(*) Yes, that's why we almost f***ed up 17.0 and needed to do a last-minute reversion of patches that changed IIDs while on beta, for example. If we wouldn't have caught that problem with applying proper sr, our problem lies deeper, of course. I don't think that's relevant, since I don't think anybody's proposing that we have re-review and re-superreview of patches for aurora and beta. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla http://www.mozilla.org/ 턂 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On 11/25/12 7:29 PM, L. David Baron wrote: On Monday 2012-11-26 04:21 +0100, Robert Kaiser wrote: Justin Dolske schrieb: I think we should consider jettisoning/rewriting that part of the policy. It doesn't match what we've been doing in reality(*) Yes, that's why we almost f***ed up 17.0 and needed to do a last-minute reversion of patches that changed IIDs while on beta, for example. If we wouldn't have caught that problem with applying proper sr, our problem lies deeper, of course. I don't think that's relevant, since I don't think anybody's proposing that we have re-review and re-superreview of patches for aurora and beta. I concur. IMO we should be applying static analysis, auditing, etc to catch these types of issues. [Super] reviewers shouldn't need to waste brain cycles on style checking, IID change requirements, etc. I concede we currently generally don't do a very good job at this. As a reviewer and someone who cares about quality, this annoys me because I know it is something that could largely be solved through decent automation and tools. Not having this wastes my time as a reviewer and increases the probability of bad things slipping through review and into the code base. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Super-review, what shall we do with you?
On 11/6/12 10:09 AM, 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. I think we should consider jettisoning/rewriting that part of the policy. It doesn't match what we've been doing in reality(*), and we don't seem to be in a terrible state as a result. A bit of data: by my crude Bugzilla search, I count 104 sr+'d bugs that have been fixed in the last 3 months. (http://is.gd/45WAxl) By product: Core - 75 Toolkit - 9 Mailnews - 8 NSS+NSPR - 6 TB+SM+CM - 6 (*) Core's pretty clearly still using sr+, so if it's working and they want to keep it that's just fine. I direct my comments at Toolkit and Firefox. The policy page goes into the motivations a bit, which are generally sound, but I think that a big part of the problem is that it's presented in absolutest terms... Any API change. Any cross-module change. Only refactoring has some wiggle room via significant. These things all vary in degree as well as the cost for making a mistake. I think we should consider rewriting these rules as guidelines and placing the expectation on reviewers to determine if/when superreview is needed. This isn't that different from how reviewers operate in Toolkit and Firefox now... Instead of hyperspecialized reviewers, it's a general pool with the expectation that members are able to self-determine what they're able to review or when they should flag someone else for additional review. This would also help from a new-contributor angle -- the whole what is sr? When do I need it? Who does it? question is hard to answer until one learns the ropes. But it's reasonable for reviewers to know. Justin ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
