docs/contributing/reviewer_checklist.rst

   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
  48
  49 Style
  50 -----
  51
  52 -  Follow the `style
  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.
  64
  65
  66 Security issues
  67 ---------------
  68
  69 -  There should be no writing to arbitrary files outside the profile
  70    folder.
  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
  73    malicious.
  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.
  77
  78
  79 Privacy issues
  80 --------------
  81
  82 -  There should be no logging of URLs or content from which URLs may be
  83    inferred.
  84 -  [Fennec: Android Services has Logger.pii() for this purpose (e.g.,
  85    logging profile dir)].
  86 -  Tag for privacy review if needed.
  87
  88
  89 Resource leaks
  90 --------------
  91
  92 -  In Java, memory leaks are largely due to singletons holding on to
  93    caches and collections, or observers sticking around, or runnables
  94    sitting in a queue.
  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
 100    appropriately.
 101 -  [Fennec: When writing tests that use PaintedSurface, ensure the
 102    PaintedSurface is closed when you're done with it.]
 103
 104
 105 Performance impact
 106 ------------------
 107
 108 -  Check for main-thread IO [Fennec: Android may warn about this with
 109    strictmode].
 110 -  Remove debug logging that is not needed in production.
 111
 112
 113 Threading issues
 114 ----------------
 115
 116 -  Enormous: correct use of locking and volatility; livelock and
 117    deadlock; ownership.
 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>`__].
 122
 123
 124 Compatibility
 125 -------------
 126
 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.
 133
 134
 135 Preffability
 136 ------------
 137
 138 -  If the feature being worked on is covered by prefs, make sure they
 139    are hooked up.
 140 -  If working on a new feature, consider adding prefs to control the
 141    behavior.
 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
 147    example.]
 148
 149
 150 Strings
 151 -------
 152
 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.
 158
 159
 160 Documentation
 161 -------------
 162
 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.
 172
 173
 174 Accessibility
 175 -------------
 176
 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]