Source code
Revision control
Copy as Markdown
Other Tools
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.]
- Error messages that appear in web platform environments should
explain the reason for the error, and use web platform terminology
(as opposed to internal Firefox terminology). More details can be
found in the :ref:`helpful error messages guide
<Helpful error messages for web developers>`.
Style
-----
- Follow the `style
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/) <https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/%29>`__].
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]