reviewer_checklist.rst (6438B)
1 Reviewer Checklist 2 ================== 3 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. 8 9 10 Good web citizenship 11 -------------------- 12 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 17 wrapper-cached. 18 19 20 Correctness 21 ----------- 22 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 29 to write and review". 30 - If QA needs to verify the fix, you should provide steps to reproduce 31 (STR). 32 33 34 Quality 35 ------- 36 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 46 Logger.] 47 - Error messages that appear in web platform environments should 48 explain the reason for the error, and use web platform terminology 49 (as opposed to internal Firefox terminology). More details can be 50 found in the :ref:`helpful error messages guide 51 <Helpful error messages for web developers>`. 52 53 54 Style 55 ----- 56 57 - Follow the `style 58 guide <https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html>`__ 59 for the language and module in question. 60 - Follow local style for the surrounding code, even if that local style 61 isn't formally documented. 62 - New files have license declarations and modelines. 63 - New JS files should use strict mode. 64 - Trailing whitespace (git diff highlight this). You can use git rebase --whitespace=fix. 65 66 67 Security issues 68 --------------- 69 70 - There should be no writing to arbitrary files outside the profile 71 folder. 72 - Be careful when reading user input, network input, or files on disk. 73 Assume that inputs will be too big, too short, empty, malformed, or 74 malicious. 75 - Tag for sec review if unsure. 76 - If you're writing code that uses JSAPI, chances are you got it wrong. 77 Try hard to avoid doing that. 78 79 80 Privacy issues 81 -------------- 82 83 - There should be no logging of URLs or content from which URLs may be 84 inferred. 85 - [Fennec: Android Services has Logger.pii() for this purpose (e.g., 86 logging profile dir)]. 87 - Tag for privacy review if needed. 88 89 90 Resource leaks 91 -------------- 92 93 - In Java, memory leaks are largely due to singletons holding on to 94 caches and collections, or observers sticking around, or runnables 95 sitting in a queue. 96 - In C++, cycle-collect as needed. If JavaScript can see your object, 97 it probably needs to be cycle-collected. 98 - [Fennec: If your custom view does animations, it's better to clean up 99 runnables in onDetachFromWindow().] 100 - Ensure all file handles and other closeable resources are closed 101 appropriately. 102 - [Fennec: When writing tests that use PaintedSurface, ensure the 103 PaintedSurface is closed when you're done with it.] 104 105 106 Performance impact 107 ------------------ 108 109 - Check for main-thread IO [Fennec: Android may warn about this with 110 strictmode]. 111 - Remove debug logging that is not needed in production. 112 113 114 Threading issues 115 ---------------- 116 117 - Enormous: correct use of locking and volatility; livelock and 118 deadlock; ownership. 119 - [Fennec: All view methods should be touched only on UI thread.] 120 - [Fennec: Activity lifecycle awareness (works with "never keep 121 activities"). Also test with oom-fennec 122 (`https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/) <https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/%29>`__]. 123 124 125 Compatibility 126 ------------- 127 128 - Version files, databases, messages 129 - Tag messages with ids to disambiguate callers. 130 - IDL UUIDs are updated when the interface is updated. 131 - Android permissions should be 'grouped' into a common release to 132 avoid breaking auto-updates. 133 - Android APIs added since Froyo should be guarded by a version check. 134 135 136 Preffability 137 ------------ 138 139 - If the feature being worked on is covered by prefs, make sure they 140 are hooked up. 141 - If working on a new feature, consider adding prefs to control the 142 behavior. 143 - Consider adding prefs to disable the feature entirely in case bugs 144 are found later in the release cycle. 145 - [Fennec: "Prefs" can be Gecko prefs, SharedPreferences values, or 146 build-time flags. Which one you choose depends on how the feature is 147 implemented: a pure Java service can't easily check Gecko prefs, for 148 example.] 149 150 151 Strings 152 ------- 153 154 - There should be no string changes in patches that will be uplifted 155 (including string removals). 156 - Rev entity names for string changes. 157 - When making UI changes, be aware of the fact that strings will be 158 different lengths in different locales. 159 160 161 Documentation 162 ------------- 163 164 - The commit message should describe what the patch is changing (not be 165 a copy of the bug summary). The first line should be a short 166 description (since only the first line is shown in the log), and 167 additional description, if needed, should be present, properly 168 wrapped, in later lines. 169 - Adequately document any potentially confusing pieces of code. 170 - Flag a bug with dev-doc-needed if any addon or web APIs are affected. 171 - Use Javadocs extensively, especially on any new non-private methods. 172 - When moving files, ensure blame/annotate is preserved. 173 174 175 Accessibility 176 ------------- 177 178 - For HTML pages, images should have the alt attribute set when 179 appropriate. Similarly, a button that is not a native HTML button 180 should have role="button" and the aria-label attribute set. 181 - [Fennec: Make sure contentDescription is set for parts of the UI that 182 should be accessible]