A man with a mission...

sdwilsh Status Update: 2011-05-27

Done:

  • Reviewed bug 655776 - Web Developer > Get More Tools menu item
  • Reviewed bug 566489 - Enable inline autocomplete again, but make it smarter (perceived performance)
  • Reviewed bug 658135 - Use sqlite3_stmt_readonly to check if multiple async statements need a transaction
  • Reviewed bug 564934 - Implement new Download Manager UI for browser
  • Reviewed bug 647886 - Pulling down Back/Forward menu requires an unnecessary delay
  • Reviewed bug 657961 - Use async API to get favicons for site permissions page
  • Reviewed bug 659693 - domain highlighting doesn't work with IPv6 literals
  • Reviewed bug 659332 - trailing // should disappear if input text is something like "filxxxx" in the LocationBar
  • Wrote a library to connect to pulse for node.js
  • Started work on tree-bot, a helpful bot to tell you when you've broken the tree or have test failures.

May 27, 2011 | Filed Under Mozilla | Comments Off 

sdwilsh Status Update: 2011-05-20

Done:

  • Reviewed bug 633274 - nsINavBookmarkObserver: also pass in GUID whenever we pass in an item id
  • Reviewed bug 656188 - Cache last 10 fetched bookmarks info to speed up repeated requests
  • Reviewed bug 598833 - Consider storing intrinsic state in a direct member of Element
  • Reviewed bug 566489 - Enable inline autocomplete again, but make it smarter (perceived performance)
  • Reviewed bug 656545 - Make about:memory tool-tips more discoverable
  • Reviewed bug 657327 - about:memory merge the "mapped" and "heap used" trees, and make the resulting tree flatter
  • Provided feedback on bug 655270 - SHEntries created by pushState don't have favicon
  • Reviewed bug 649867 - Fix or remove the heap-used/storage/lookaside-used memory reporter
  • Reviewed bug 573176 - Implement Site-Specific Privacy Preferences
  • Reviewed bug 566489 - Enable inline autocomplete again, but make it smarter (perceived performance)
  • Reviewed bug 658135 - Use sqlite3_stmt_readonly to check if multiple async statements need a transaction
  • Reviewed bug 650573 - Panorama hangs when restoring a session
  • Reviewed bug 658344 - Create some useful introductory text for the Scratchpad
  • Reviewed bug 658135 - Use sqlite3_stmt_readonly to check if multiple async statements need a transaction
  • Reviewed bug 657131 - Create a new Execute menu for Scratchpad
  • Blogged about what I look for during a review
  • Reviewed bug 656773 - about:memory error in private browsing mode, page is blank
  • Reviewed bug 653108 - Scratchpad is tied to the tab it was first run in
  • Continued to investigate a tp4 chrome performance regression. Finally getting some interesting data.

Coordination:

I am changing jobs on June 1st, so my review queue bandwidth is going to decrease, which will likely result in slightly longer wait times. I hope to keep it limited to three business days after June 1st, however.

May 20, 2011 | Filed Under Mozilla | Comments Off 

Changes

I’m going to write something that will probably surprise you. I say this because it sure surprised me when I realized I was even considering what I’m doing a possibility. I’m going to be moving on to something a bit different in the mobile space, and it’s going to be a different kind of challenge for me.

June 1st will be my last day at Mozilla. I’ve learned so much over the years working there, and choosing to leave was the hardest decision I’ve had to make. I do not intend to disappear from the project, however, but my activity level will decrease. Feel free to continue to send review requests my way and cc me to bugs you want my opinion on, and I’ll do my best to reply in a timely manner.

So long, and thanks for all the fish.

May 18, 2011 | Filed Under Mozilla, Personal | Comments Off 

How I Review

I handle a lot of code review and code feedback requests these days. However, it’d be great to get more people doing this for a number of reasons:

  • More people exposed to more parts of the code base
  • More review bandwidth so more work can be checked into the tree
  • Less dependence on a small set of people

In order to get more people doing this, it would be good to document to look for, and how to make sure the code is sound. I’m sure every reviewer does things a bit differently, but I’m going to share my process. There are two types of review I do these days: feedback and review.

Feedback

Feedback is pretty simple to do, and I can usually fly though any patch (even large ones) quickly. This isn’t very thorough (in fact, I tend to keep it to general comments), but I look for the following things:

  • correct API usage (XPCOM, jsm, whatever)
  • internal invariants are not violated
  • any new APIs created make sense and aren’t confusing
  • code style matches what’s there, or follows the style guide

Review

Review is more important because once a patch gets r+, it can generally land in the tree. Consequentially, I tend to spend a lot more time on any given patch. In addition to all the things I do for a feedback request (which are looked at more closely for a review), I’ll also look at the following:

  • evaluate how well tested is the code that is being added/modified. If it isn’t well tested, I’ll generally suggest a set of test cases that I feel are the bare minimum this needs to land.
  • evaluate how this might impact other work going on in this area of the codebase
  • ensure this doesn’t add any I/O on the GUI thread
  • apply the patch and run the tests
  • if the patch looks like it might regress performance, ask for the author to verify that it does not

Note that a number of these things may not be done if I know what the patch author has already done to ensure the patch is safe.

May 15, 2011 | Filed Under Code, Mozilla | Comments Off 

sdwilsh Status Update: 2011-05-13

Done:

  • Reviewed bug 654732 - Remove WinCE code from storage/* & db/sqlite3/src/Makefile.in
  • Reviewed bug 653630 - Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory
  • Reviewed bug 650509 - Other apps can read Firefox profile files
  • Reviewed bug 493783 - Fixing warnings in toolkit components (places and commandline)
  • Cleaned up bug 584316 (Fix DownloadManager usage of deprecated Storage binding APIs) so it can land.
  • Reviewed bug 633274 - nsINavBookmarkObserver: also pass in GUID whenever we pass in an item id
  • Pushed lots of stuff to the try server and triggered a bunch of extra talos runs to start establishing a regression range for bug 655930 - Windows chrome seems to be a 25% pageload hit
  • Reviewed bug 577721 - allow repositioning of the web console: above, below and out into a panel/window
  • Provided feedback on bug 573176 - Implement Site-Specific Privacy Preferences

Coordination:

My review/feedback queue has gotten a number of large patches in it lately, which has resulted in some longer waiting times for reviews. Small patches are still getting fast turn around, but the bigger ones have a bit of a waiting period.

May 13, 2011 | Filed Under Mozilla | 2 Comments 

Next Page »