wiki:CodeReview

Once you put a patch up for review (i.e. mark a patch r?), it goes into the Review Queue. That may not be enough to get it reviewed in a timely manner. WebKit reviewers are all overloaded. The responsibility is on the committer to make it as easy as possible to get the code reviewed. To that end, you should:

  • Look at the people who most recently modified the code you're changing and explicitly CC them on the bug.
  • Add a layout test when you fix a bug/change a behavior or explain why that isn't possible and add a manual test to WebCore/manual-tests.
  • Make your patch as small as possible. Really, seriously, go out of your way to make sure you've broken down your patch into the smallest self-contained chunks possible.
  • Only do/fix one thing per patch. Don't change a behavior and then add a fix for an incidental thing that you noticed while doing the original change. It should be two patches.
  • Only do one patch per bug. Multiple iterations of the same patch is fine but use a new bug for a new patch. Otherwise, it gets confusing to read through all of the comments and understand which ones apply to each patch.
  • Do style cleanup or other significant refactoring in a separate patch, preferably as a precursor to the patch you want to land. Especially end of line whitespace cleanup as this just adds a lot of visual noise to the review.
  • Add short per function comments in the change log (see any of Darin Adler's changes for a good example). These help reviewers quickly understand why you are doing a change in that function.
  • Fix errors from any bots that run after you upload your patch.

If you've done all the above, then you can also ping on #webkit to see if there are any reviewers who can review it for you. If it's a trivial change, than anyone can review it. If it's a complicated or gnarly part of the codebase, you'll likely need to of the approval of one of the people who recently modified the code.

Last modified 14 years ago Last modified on Jul 22, 2010 12:24:19 PM