Reviewer Checklist

Submitting patches to Mozilla source code needn’t be complex. This article provides a list of best practices for your patch content that reviewers will check for or require. Following these best practices will lead to a smoother, more rapid process of review and acceptance.

Good web citizenship

  • Make sure new web-exposed APIs actually make sense and are either standards track or preffed off by default.

  • In C++, wrapper-cache as needed. If your object can be gotten from somewhere without creating it in the process, it needs to be wrapper-cached.

Correctness

  • The bug being fixed is a valid bug and should be fixed.

  • The patch fixes the issue.

  • The patch is not unnecessarily complicated.

  • The patch does not add duplicates of existing code (‘almost duplicates’ could mean a refactor is needed). Commonly this results in “part 0” of a bug, which is “tidy things up to make the fix easier to write and review”.

  • If QA needs to verify the fix, you should provide steps to reproduce (STR).

Quality

  • If you can unit-test it, you should unit-test it.

  • If it’s JS, try to design and build so that xpcshell can exercise most functionality. It’s quicker.

  • Make sure the patch doesn’t create any unused code (e.g., remove strings when removing a feature)

  • All caught exceptions should be logged at the appropriate level, bearing in mind personally identifiable information, but also considering the expense of computing and recording log output. [Fennec: Checking for log levels is expensive unless you’re using Logger.]

Style

  • Follow the style guide for the language and module in question.

  • Follow local style for the surrounding code, even if that local style isn’t formally documented.

  • New files have license declarations and modelines.

  • New JS files should use strict mode.

  • Trailing whitespace (git diff highlight this). You can use git rebase –whitespace=fix.

Security issues

  • There should be no writing to arbitrary files outside the profile folder.

  • Be careful when reading user input, network input, or files on disk. Assume that inputs will be too big, too short, empty, malformed, or malicious.

  • Tag for sec review if unsure.

  • If you’re writing code that uses JSAPI, chances are you got it wrong. Try hard to avoid doing that.

Privacy issues

  • There should be no logging of URLs or content from which URLs may be inferred.

  • [Fennec: Android Services has Logger.pii() for this purpose (e.g., logging profile dir)].

  • Tag for privacy review if needed.

Resource leaks

  • In Java, memory leaks are largely due to singletons holding on to caches and collections, or observers sticking around, or runnables sitting in a queue.

  • In C++, cycle-collect as needed. If JavaScript can see your object, it probably needs to be cycle-collected.

  • [Fennec: If your custom view does animations, it’s better to clean up runnables in onDetachFromWindow().]

  • Ensure all file handles and other closeable resources are closed appropriately.

  • [Fennec: When writing tests that use PaintedSurface, ensure the PaintedSurface is closed when you’re done with it.]

Performance impact

  • Check for main-thread IO [Fennec: Android may warn about this with strictmode].

  • Remove debug logging that is not needed in production.

Threading issues

  • Enormous: correct use of locking and volatility; livelock and deadlock; ownership.

  • [Fennec: All view methods should be touched only on UI thread.]

  • [Fennec: Activity lifecycle awareness (works with “never keep activities”). Also test with oom-fennec (https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/)].

Compatibility

  • Version files, databases, messages

  • Tag messages with ids to disambiguate callers.

  • IDL UUIDs are updated when the interface is updated.

  • Android permissions should be ‘grouped’ into a common release to avoid breaking auto-updates.

  • Android APIs added since Froyo should be guarded by a version check.

Preffability

  • If the feature being worked on is covered by prefs, make sure they are hooked up.

  • If working on a new feature, consider adding prefs to control the behavior.

  • Consider adding prefs to disable the feature entirely in case bugs are found later in the release cycle.

  • [Fennec: “Prefs” can be Gecko prefs, SharedPreferences values, or build-time flags. Which one you choose depends on how the feature is implemented: a pure Java service can’t easily check Gecko prefs, for example.]

Strings

  • There should be no string changes in patches that will be uplifted (including string removals).

  • Rev entity names for string changes.

  • When making UI changes, be aware of the fact that strings will be different lengths in different locales.

Documentation

  • The commit message should describe what the patch is changing (not be a copy of the bug summary). The first line should be a short description (since only the first line is shown in the log), and additional description, if needed, should be present, properly wrapped, in later lines.

  • Adequately document any potentially confusing pieces of code.

  • Flag a bug with dev-doc-needed if any addon or web APIs are affected.

  • Use Javadocs extensively, especially on any new non-private methods.

  • When moving files, ensure blame/annotate is preserved.

Accessibility

  • For HTML pages, images should have the alt attribute set when appropriate. Similarly, a button that is not a native HTML button should have role=”button” and the aria-label attribute set.

  • [Fennec: Make sure contentDescription is set for parts of the UI that should be accessible]