4 Submitting patches to Mozilla source code needn't be complex. This
5 article provides a list of best practices for your patch content that
6 reviewers will check for or require. Following these best practices
7 will lead to a smoother, more rapid process of review and acceptance.
13 - Make sure new web-exposed APIs actually make sense and are either
14 standards track or preffed off by default.
15 - In C++, wrapper-cache as needed. If your object can be gotten from
16 somewhere without creating it in the process, it needs to be
23 - The bug being fixed is a valid bug and should be fixed.
24 - The patch fixes the issue.
25 - The patch is not unnecessarily complicated.
26 - The patch does not add duplicates of existing code ('almost
27 duplicates' could mean a refactor is needed). Commonly this results
28 in "part 0" of a bug, which is "tidy things up to make the fix easier
30 - If QA needs to verify the fix, you should provide steps to reproduce
37 - If you can unit-test it, you should unit-test it.
38 - If it's JS, try to design and build so that xpcshell can exercise
39 most functionality. It's quicker.
40 - Make sure the patch doesn't create any unused code (e.g., remove
41 strings when removing a feature)
42 - All caught exceptions should be logged at the appropriate level,
43 bearing in mind personally identifiable information, but also
44 considering the expense of computing and recording log output.
45 [Fennec: Checking for log levels is expensive unless you're using
53 guide <https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html>`__
54 for the language and module in question.
55 - Follow local style for the surrounding code, even if that local style
56 isn't formally documented.
57 - New files have license declarations and modelines.
58 - New JS files should use strict mode.
59 - Trailing whitespace (git diff and splinter view both highlight this,
60 as does hg with the color extension enabled). Whitespace can be fixed
61 easily in Mercurial using the `CheckFiles
62 extension <https://www.mercurial-scm.org/wiki/CheckFilesExtension>`__.
63 In git, you can use git rebase --whitespace=fix.
69 - There should be no writing to arbitrary files outside the profile
71 - Be careful when reading user input, network input, or files on disk.
72 Assume that inputs will be too big, too short, empty, malformed, or
74 - Tag for sec review if unsure.
75 - If you're writing code that uses JSAPI, chances are you got it wrong.
76 Try hard to avoid doing that.
82 - There should be no logging of URLs or content from which URLs may be
84 - [Fennec: Android Services has Logger.pii() for this purpose (e.g.,
85 logging profile dir)].
86 - Tag for privacy review if needed.
92 - In Java, memory leaks are largely due to singletons holding on to
93 caches and collections, or observers sticking around, or runnables
95 - In C++, cycle-collect as needed. If JavaScript can see your object,
96 it probably needs to be cycle-collected.
97 - [Fennec: If your custom view does animations, it's better to clean up
98 runnables in onDetachFromWindow().]
99 - Ensure all file handles and other closeable resources are closed
101 - [Fennec: When writing tests that use PaintedSurface, ensure the
102 PaintedSurface is closed when you're done with it.]
108 - Check for main-thread IO [Fennec: Android may warn about this with
110 - Remove debug logging that is not needed in production.
116 - Enormous: correct use of locking and volatility; livelock and
118 - [Fennec: All view methods should be touched only on UI thread.]
119 - [Fennec: Activity lifecycle awareness (works with "never keep
120 activities"). Also test with oom-fennec
121 (`https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/) <https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/%29>`__].
127 - Version files, databases, messages
128 - Tag messages with ids to disambiguate callers.
129 - IDL UUIDs are updated when the interface is updated.
130 - Android permissions should be 'grouped' into a common release to
131 avoid breaking auto-updates.
132 - Android APIs added since Froyo should be guarded by a version check.
138 - If the feature being worked on is covered by prefs, make sure they
140 - If working on a new feature, consider adding prefs to control the
142 - Consider adding prefs to disable the feature entirely in case bugs
143 are found later in the release cycle.
144 - [Fennec: "Prefs" can be Gecko prefs, SharedPreferences values, or
145 build-time flags. Which one you choose depends on how the feature is
146 implemented: a pure Java service can't easily check Gecko prefs, for
153 - There should be no string changes in patches that will be uplifted
154 (including string removals).
155 - Rev entity names for string changes.
156 - When making UI changes, be aware of the fact that strings will be
157 different lengths in different locales.
163 - The commit message should describe what the patch is changing (not be
164 a copy of the bug summary). The first line should be a short
165 description (since only the first line is shown in the log), and
166 additional description, if needed, should be present, properly
167 wrapped, in later lines.
168 - Adequately document any potentially confusing pieces of code.
169 - Flag a bug with dev-doc-needed if any addon or web APIs are affected.
170 - Use Javadocs extensively, especially on any new non-private methods.
171 - When moving files, ensure blame/annotate is preserved.
177 - For HTML pages, images should have the alt attribute set when
178 appropriate. Similarly, a button that is not a native HTML button
179 should have role="button" and the aria-label attribute set.
180 - [Fennec: Make sure contentDescription is set for parts of the UI that
181 should be accessible]