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 and splinter view both highlight this, as does hg with the color extension enabled). Whitespace can be fixed easily in Mercurial using the `CheckFiles extension `__. In git, 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]