docs/contributing/how_to_submit_a_patch.rst

   1 How to submit a patch
   2 =====================
   3
   4 +--------------------------------------------------------------------+
   5 | This page is an import from MDN and the contents might be outdated |
   6 +--------------------------------------------------------------------+
   7
   8 Submitting a patch, getting it reviewed, and committed to the Firefox
   9 source tree involves several steps. This article explains how.
  10
  11 .. note::
  12
  13    We are also providing a :ref:`Firefox Contributors Quick Reference <Firefox Contributors' Quick Reference>` for contributors.
  14
  15 The process of submission is illustrated by the following diagram, and
  16 each step is detailed below:
  17
  18 .. mermaid::
  19
  20      graph TD;
  21          Preparation --> c[Working on a patch];
  22          c[Working on a patch] --> Testing;
  23          Testing --> c[Working on a patch];
  24          Testing --> e[Submit the patch];
  25          e[Submit the patch] --> d[Getting Reviews]
  26          d[Getting Reviews] -- Addressing Review comment --> c[Working on a patch];
  27          d[Getting Reviews] --> h[Push the change];
  28
  29
  30
  31
  32 Preparation
  33 -----------
  34
  35 Every change to the code is tracked by a bug report
  36 in `bugzilla.mozilla.org <https://bugzilla.mozilla.org/>`__. Without a
  37 bug, code will not be reviewed, and without review, code will not be
  38 accepted. To avoid duplication, `search for an existing
  39 bug <https://bugzilla.mozilla.org/query.cgi?format=specific>`__ about
  40 your change, and only if none exists, file a new one. Most communication
  41 about code changes take place in the associated code
  42 review, so be sure the bug describes the exact problem being solved.
  43
  44 Please verify the bug is for the correct product and component. For more
  45 information, ask questions on the newsgroups, or on the #developers room
  46 on `chat.mozilla.org <https://chat.mozilla.org>`__.
  47
  48 The person working on a bug should be the 'assignee' of that bug in
  49 Bugzilla. If somebody else is currently the assignee of a bug, email
  50 this person to coordinate changes. If the bug is unassigned, leave a
  51 message in the bug's comments, stating that you intend working on it,
  52 and suggest that someone with bug-editing privileges assign it to you.
  53
  54 Some teams wait for new contributors to attach their first patch before
  55 assigning a bug. This makes it available for other contributors, in case
  56 the new contributor is unable to level up to patch creation. By
  57 expressing interest in a bug comment, someone from that team should
  58 guide you through their process.
  59
  60
  61 Module ownership
  62 ----------------
  63
  64 All code is supervised by a `module
  65 owner <https://www.mozilla.org/en-US/about/governance/policies/module-ownership/>`__.
  66 This person will be responsible for reviewing and accepting the change.
  67 Before writing your code, determine the module owner, verifying your
  68 proposed change is considered acceptable. They may want to look over any
  69 new user interface (UI review), functions (API review), or testcases for
  70 the proposed change.
  71
  72 If module ownership is not clear, ask on the newsgroups or `on
  73 Matrix <https://chat.mozilla.org>`__. The revision log for the relevant
  74 file might also be helpful. For example, see the change log for
  75 ``browser/base/content/browser.js``, by clicking the "Hg Log"
  76 link at the top of `Searchfox <https://searchfox.org/mozilla-central/source/>`__, or
  77 by running ``hg log browser/base/content/browser.js``. The corresponding
  78 checkin message will contain something like "r=nickname", identifying
  79 active code submissions, and potential code reviewers.
  80
  81
  82 Working on a patch
  83 ------------------
  84
  85 Changes to the Firefox source code are presented in the form of a patch.
  86 A patch is a commit to version control. Firefox and related code is
  87 stored in our `Mercurial
  88 server <https://hg.mozilla.org/mozilla-central>`__. We have extensive
  89 documentation on using Mercurial in our guide, :ref:`Mercurial Overview`.
  90
  91 Each patch should represent a single complete change, separating
  92 distinct changes into multiple individual patches. If your change
  93 results in a large, complex patch, seek if it can be broken into
  94 `smaller, easy to understand patches representing complete
  95 steps <https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/#many-small-commits>`__,
  96 applied on top of each other. This makes it easier to review your
  97 changes, `leading to quicker
  98 reviews, <https://groups.google.com/group/mozilla.dev.planning/msg/2f99460f57f776ef?hl=en>`__
  99 and improved confidence in this review outcome.
 100
 101 Also ensure that your commit message is formatted appropriately. A
 102 simple commit message should look like this:
 103
 104 ::
 105
 106    Bug 123456 - Change this thing to work better by doing something. r=reviewers
 107
 108 The ``r=reviewers`` part is optional; if you are using Phabricator,
 109 Lando will add it automatically based on who actually granted review,
 110 and in any case the person who does the final check-in of the patch will
 111 make sure it's added.
 112
 113 The text of the message should be what you did to fix the bug, not a
 114 description of what the bug was. If it is not obvious why this change is
 115 appropriate, then `explain why in the commit
 116 message <https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages>`__.
 117 If this does not fit on one line, then leave a blank line and add
 118 further lines for more detail and/or reasoning.
 119
 120 You can edit the message of the current commit at any time using
 121 ``hg commit --amend`` or ``hg histedit``.
 122
 123 Also look at our :ref:`Reviewer Checklist` for a list
 124 of best practices for patch content that reviewers will check for or
 125 require.
 126
 127
 128 Testing
 129 -------
 130
 131 All changes must be tested. In most cases, an `automated
 132 test <https://developer.mozilla.org/docs/Mozilla/QA/Automated_testing>`__ is required for every
 133 change to the code.
 134
 135 While we desire to have automated tests for all code, we also have a
 136 linter tool which runs static analysis on our JavaScript, for best
 137 practices and common mistakes. See :ref:`ESLint` for more information.
 138
 139 Ensure that your change has not caused regressions, by running the
 140 automated test suite locally, or using the `Mozilla try
 141 server <https://wiki.mozilla.org/Build:TryServer>`__. Module owners, or
 142 developers `on Matrix <https://chat.mozilla.org>`__ may be willing to
 143 submit jobs for those currently without try server privileges.
 144
 145
 146 Submit the patch
 147 ----------------
 148
 149 .. note::
 150
 151    Make sure you rebase your patch on top of the latest build before you
 152    submit to prevent any merge conflicts.
 153
 154 Mozilla uses Phabricator for code review. See the `Mozilla Phabricator
 155 User
 156 Guide <https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html>`__
 157 for instructions.
 158
 159 Don't be shy in posting partial patches, demonstrating potential
 160 approaches, and asking for preliminary feedback. It is easier for others
 161 to comment, and offer suggestions, when a question is accompanied by
 162 some code.
 163
 164
 165 Getting reviews for my patch
 166 ----------------------------
 167
 168 See the dedicated page :ref:`Getting reviews`
 169
 170
 171 Addressing review comments
 172 --------------------------
 173
 174 It is unusual for patches to be perfect the first time around. The
 175 reviewer may use the ‘Request Changes’
 176 `action <http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#reviewing-patches>`__
 177 and list problems that must be addressed before the patch can be
 178 accepted. Please remember that requesting revisions is not meant to
 179 discourage participation, but rather to encourage the best possible
 180 resolution of a bug. Carefully work through the changes that the
 181 reviewer recommends, attach a new patch, and request review again.
 182
 183 Sometimes a reviewer will grant conditional review with the ‘Accept
 184 Revision’ action but will also indicate minor necessary changes, such as
 185 spelling, or indentation fixes. All recommended corrections should be
 186 made, but a re-review is unnecessary. Make the changes and submit a new
 187 patch. If there is any confusion about the revisions, another review
 188 should be requested.
 189
 190 Sometimes, after a patch is reviewed, but before it can be committed,
 191 someone else makes a conflicting change. If the merge is simple, and
 192 non-invasive, post an updated version of the patch. For all non-trivial
 193 changes, another review is necessary.
 194
 195 If at any point the review process stalls for more than two weeks, see
 196 the previous 'Getting attention' section.
 197
 198 In many open source projects, developers will accept patches in an
 199 unfinished state, finish them, and apply the completed code. In
 200 Mozilla's culture, **the reviewer will only review and comment on a
 201 patch**. If a submitter declines to make the revisions, the patch will
 202 sit idle, until someone chooses to take it on.
 203
 204
 205 Pushing the change
 206 ------------------
 207
 208 A patch can be pushed (aka. 'landed') after it has been properly
 209 reviewed.
 210
 211 .. note::
 212
 213    Be sure to build the application with the patch applied. This
 214    ensures it runs as expected, passing automated tests, and/or runs
 215    through the `try
 216    server <https://wiki.mozilla.org/Build:TryServerAsBranch>`__. In the
 217    bug, please also mention you have completed this step.
 218
 219    Submitting untested patches wastes the committer's time, and may burn
 220    the release tree. Please save everyone's time and effort by
 221    completing all necessary verifications.
 222
 223
 224 Ask the reviewer to land the patch for you.
 225 For more details, see :ref:`push_a_change`
 226
 227 `Lando <https://moz-conduit.readthedocs.io/en/latest/lando-user.html>`__ is used
 228 to automatically land your code.
 229
 230
 231 Regressions
 232 -----------
 233
 234 It is possible your code causes functional or performance regressions.
 235 There is a tight
 236 `policy <https://www.mozilla.org/about/governance/policies/regressions/>`__ on
 237 performance regressions, in particular. This means your code may be
 238 dropped, leaving you to fix and resubmit it. Regressions, ultimately
 239 mean the tests you ran before checking in are not comprehensive enough.
 240 A resubmitted patch, or a patch to fix the regression, should be
 241 accompanied by appropriate tests.
 242
 243 After authoring a few patches, consider `getting commit access to
 244 Mozilla source code <https://www.mozilla.org/about/governance/policies/commit/>`__.