sdwilsh Status Update: 2011-04-29
Done:
Managed to keep review turn-around time to at most two days this week, with most patches getting reviewed or feedback within one day.
- Reviewed bug 570189 - Throw an error when there are unbound values
- Reviewed bug 652355 - Remove virtual accessor GetURIExternal from Link
- Reviewed bug 633282 - Aero glass glaze effect jumps down when status info overlay appears
- Reviewed bug 472530 - when statement creation fails, dump lastErrorText to stdout in debug builds
- Reviewed bug 619623 - Intermittent toolkit/components/places/tests/cpp/test_IHistory.cpp | Expected true (or '1'), got false (or '0') at line 364
- Reviewed bug 653261 - Add a function to format the date and time displayed in the Download Manager
- Provided feedback on bug 649154 - Implement DOMCrypt 'window.crypt' as a mozilla DOM property
- Provided feedback on bug 564934 - Implement new Download Manager UI for browser
Getting (and Staying) at Review Queue: 0
I’ll assert that one of the biggest pain points of getting a patch in the tree is getting review. This is largely a problem because it can often take a very long time to get that review from the limited set of people who are allowed to review your patch. Even a veteran contributor who knows his way around the system can have his patches sitting in somebody’s review queue for weeks.
I’ve decided I’m going to do something about this by setting a good example, and hope that others follow. For the past two weeks, I’ve been driving my review queue down to zero almost every day. This sounds like a lot of work (especially for anyone who gets a lot of review requests), but I’ve found that in practice, it isn’t so bad. I usually start out every morning by going through all my bugmail before I dig into my days work. I’ve simply added a step after this where I do code reviews until about noon. This doesn’t often take that long as most patches I get aren’t terribly large, aren’t terribly complicated, or there aren’t many to review. If I happen to not get through all of the review requests, I then set aside an hour at the end of my day to go through the rest. In theory, this means my turn-around time for review requests is one business day. In practice, it has come out to be closer to a two or three days due to a number of large patches that I’ve had to review.
You might say “well, that’s easy to do if you don’t get many review requests!” This is very true, but it’s not like I get a small number of requests. Over the past two weeks I’ve done review and feedback requests for 24 bugs, many of which required more than one review iteration. I don’t think that’s a small number of reviews (at least two per day).
I have one caveat with this though; I don’t want to have the quality of my reviews to degrade. For most patches, I’ve found that if I do any more than five or six a day I start to miss things because I’m too mentally drained at that point. If I have some really large patches I’ll review less, and if it is a bunch of small patches, I’ll review more. Sacrificing code quality for the sake of fast turn-around times isn’t ideal, in my opinion.
I would love it if more people tried this out to see if it works for them. The people requesting review from you would really appreciate it!
sdwilsh Status Update: 2011-04-22
Done:
- Reviewed bug 570189 - Throw an error when there are unbound values
- Reviewed bug 650410 - Test for bug 383369 uses a timeout because it doesn't know how to use the download manager API!
- Reviewed bug 642176 - Integrate Workspace extension into the browser
- Reviewed bug 636725 - Unit tests for Workspaces
- Reviewed bug 646524 - Workspace: cache the sandboxes
- Reviewed bug 633282 - Aero glass glaze effect jumps down when status info overlay appears
- Reviewed bug 468313M - Having to click 'Ignore this warning' for every page on the suspected 'Attack Site' is seriously annoying
- Reviewed bug 641531 - Close Places containers after use
- Reviewed bug 641074 - Broken folder shortcuts should not interrupt a Places result (nsINavHistoryContainerResultNode.childCount throws)
- A whole bunch of prep work for the Stanford Open Source Bootcamp
sdwilsh Status Update: 2011-04-15
Done:
- Reviewed bug 647557 - Uninitialized variable transitionType in History::UpdatePlaces
- Reviewed bug 595888 - Show file size when attempting to download a file
- Reviewed bug 620832 - Web console attempts to remove its observers multiple times
- Provided feedback on bug 646262 - Dont dig into the download manager database directly
- Reviewed bug 629489 - Update mozStorage.h on how to add more errors
- Landed (in time for Firefox 5!) bug 599641 - Run ANALYZE after expiration
- Reviewed bug 632275 - Web Console: Error message when click on an object to create an inspect window
- Reviewed bug 649211 - PlacesDBUtils console output is broken
- Reviewed bug 641176 - Integrate Workspace extension into the browser
- Put up a patch for bug 649435 - Remove the frozen date from the User Agent String (warning: bikeshed)
- Reviewed bug 636725 - Unit tests for Workspaces
- Provided feedback and review for bug 633653 - revamp about:memory
- Reviewed bug 644680 - FileUtils.openSafeFileOutputStream() should init stream with DEFER_OPEN
- Reviewed bug 524091 - Remove microsummaries support. Seldom used, undiscoverable and unmaintained.
- Reviewed bug 620627 - PlacesSQLQueryBuilder::SelectAsDay() is not l12y friendly
- Reviewed bug 646070 - Respect chrome developer tools preference in workspace
- Provided feedback on bug 564934 - Implement new Download Manager UI for browser
sdwilsh Status Update: 2011-04-01
Done:
- Reviewed bug 642660 - Add API for storing oauth tokens and account profiles
- Pushed a bunch of things I patched last week
- Cleaned up performance best practices and file I/O code snippets on MDC.
- Reviewed bug 595888 - Show file size when attempting to download a file
- Did a bunch of research and have an initial patch up on bug 599641 - Run ANALYZE after expiration
