On Mon, Jan 21, 2013 at 2:13 PM, Filip Maj <f...@adobe.com> wrote: > >>Braden just used a private branch and didn't ask for review. Maybe the >>take-away from this is that we it would be good to have the chance for a >>review for anything that was big enough to warrant a branch? I'll add this >>to the committer's wiki page. > > Yeah let's summarize our discussion points / rules of thumb down in the > wiki page in case we were missing anything there. > > TBH we haven't done many code reviews in the past. I believe > CordovaWebView was one of the biggest features we landed since about > Cordova 1.0 that we had actually organized a code review, set up a > conference call and screenshare to join in, etc. It lived in a branch I > believe through two or three point releases just because it was a giant > feature, needed a couple rebases/merges, and failed the first couple times > we tried to merge it in. >
I feel that this was to excessive, and that it actually hurt us in the long-run having CordovaWebView in review for so long. That being said, it did teach us what's important to watch out for when introducing new features, and what features will cause our users to want our heads on sticks. I think an informal review is good enough. I usually wait 24 to 48 hours before merging things in. That being said, I'm able to amuse myself with weird side projects like Purity (https://github.com/infil00p/cordova-android/tree/puritytool) and other tasks while I wait. :P > In general, committers are trusted to be working on features that are > important to the project and will land high quality code eventually into > master. We just need that extra step of discipline of running mobile-spec > and running the native tests for affected platforms on a few devices for > sanity's sake. And as always, if you can't do this for whatever reason, > you can always reach out to the list and ask for other committers to test > out feature x on device y, or even just a smoke/sanity test on another > committer's machine, etc. before merging into master. > Agreed! We have a lack of old Android 2.3 devices in the Vancouver office, and a lot of Android 4.x devices. I'll usually need to get things tested on the original Samsung Galaxy S or some other Samsung Galaxy phone that's not the top of the line, or is ruggedized or something (i.e. Samsung Galaxy Rugby).
