diff options
Diffstat (limited to 'docs/contributing/how_to_submit_a_patch.rst')
| -rw-r--r-- | docs/contributing/how_to_submit_a_patch.rst | 276 |
1 files changed, 276 insertions, 0 deletions
diff --git a/docs/contributing/how_to_submit_a_patch.rst b/docs/contributing/how_to_submit_a_patch.rst new file mode 100644 index 0000000000..54cf3f14d3 --- /dev/null +++ b/docs/contributing/how_to_submit_a_patch.rst @@ -0,0 +1,276 @@ +How to submit a patch +===================== + ++--------------------------------------------------------------------+ +| This page is an import from MDN and the contents might be outdated | ++--------------------------------------------------------------------+ + +Submitting a patch, getting it reviewed, and committed to the Firefox +source tree involves several steps. This article explains how. + +.. note:: + + We are also providing a :ref:`Firefox Contributors Quick Reference <Firefox Contributors' Quick Reference>` for contributors. + +The process of submission is illustrated by the following diagram, and +each step is detailed below: + +.. mermaid:: + + graph TD; + Preparation --> c[Working on a patch]; + c[Working on a patch] --> Testing; + Testing --> c[Working on a patch]; + Testing --> e[Submit the patch]; + e[Submit the patch] --> d[Getting Reviews] + d[Getting Reviews] -- Addressing Review comment --> c[Working on a patch]; + d[Getting Reviews] --> h[Push the change]; + + + + +Preparation +----------- + +Every change to the code is tracked by a bug report +in `bugzilla.mozilla.org <https://bugzilla.mozilla.org/>`__. Without a +bug, code will not be reviewed, and without review, code will not be +accepted. To avoid duplication, `search for an existing +bug <https://bugzilla.mozilla.org/query.cgi?format=specific>`__ about +your change, and only if none exists, file a new one. Most communication +about code changes take place in the associated code +review, so be sure the bug describes the exact problem being solved. + +Please verify the bug is for the correct product and component. For more +information, ask questions on the newsgroups, or on the #developers room +on `chat.mozilla.org <https://chat.mozilla.org>`__. + +The person working on a bug should be the 'assignee' of that bug in +Bugzilla. If somebody else is currently the assignee of a bug, email +this person to coordinate changes. If the bug is unassigned, leave a +message in the bug's comments, stating that you intend working on it, +and suggest that someone with bug-editing privileges assign it to you. + +Some teams wait for new contributors to attach their first patch before +assigning a bug. This makes it available for other contributors, in case +the new contributor is unable to level up to patch creation. By +expressing interest in a bug comment, someone from that team should +guide you through their process. + + +Module ownership +---------------- + +All code is supervised by a `module +owner <https://www.mozilla.org/en-US/about/governance/policies/module-ownership/>`__. +This person will be responsible for reviewing and accepting the change. +Before writing your code, determine the module owner, verifying your +proposed change is considered acceptable. They may want to look over any +new user interface (UI review), functions (API review), or testcases for +the proposed change. + +If module ownership is not clear, ask on the newsgroups or `on +Matrix <https://chat.mozilla.org>`__. The revision log for the relevant +file might also be helpful. For example, see the change log for {{ +Source("browser/base/content/browser.js") }}, by clicking the "Hg Log" +link at the top of `Searchfox <https://searchfox.org/mozilla-central/source/>`__, or +by running ``hg log browser/base/content/browser.js``. The corresponding +checkin message will contain something like "r=nickname", identifying +active code submissions, and potential code reviewers. + + +Working on a patch +------------------ + +Changes to the Firefox source code are presented in the form of a patch. +A patch is a commit to version control. Firefox and related code is +stored in our `Mercurial +server <https://hg.mozilla.org/mozilla-central>`__. We have extensive +documentation on using Mercurial in our guide, :ref:`Mercurial Overview`. + +Each patch should represent a single complete change, separating +distinct changes into multiple individual patches. If your change +results in a large, complex patch, seek if it can be broken into +`smaller, easy to understand patches representing complete +steps <https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/#many-small-commits>`__, +applied on top of each other. This makes it easier to review your +changes, `leading to quicker +reviews, <https://groups.google.com/group/mozilla.dev.planning/msg/2f99460f57f776ef?hl=en>`__ +and improved confidence in this review outcome. + +Also ensure that your commit message is formatted appropriately. A +simple commit message should look like this: + +:: + + Bug 123456 - Change this thing to work better by doing something. r=reviewers + +The ``r=reviewers`` part is optional; if you are using Phabricator, +Lando will add it automatically based on who actually granted review, +and in any case the person who does the final check-in of the patch will +make sure it's added. + +The text of the message should be what you did to fix the bug, not a +description of what the bug was. If it is not obvious why this change is +appropriate, then `explain why in the commit +message <https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages>`__. +If this does not fit on one line, then leave a blank line and add +further lines for more detail and/or reasoning. + +You can edit the message of the current commit at any time using +``hg commit --amend`` or ``hg histedit``. + +Also look at our :ref:`Reviewer Checklist` for a list +of best practices for patch content that reviewers will check for or +require. + + +Testing +------- + +All changes must be tested. In most cases, an `automated +test <https://developer.mozilla.org/docs/Mozilla/QA/Automated_testing>`__ is required for every +change to the code. + +While we desire to have automated tests for all code, we also have a +linter tool which runs static analysis on our JavaScript, for best +practices and common mistakes. See :ref:`ESLint` for more information. + +Ensure that your change has not caused regressions, by running the +automated test suite locally, or using the `Mozilla try +server <https://wiki.mozilla.org/Build:TryServer>`__. Module owners, or +developers `on Matrix <https://chat.mozilla.org>`__ may be willing to +submit jobs for those currently without try server privileges. + + +Submit the patch +---------------- + +.. note:: + + Make sure you rebase your patch on top of the latest build before you + submit to prevent any merge conflicts. + +Mozilla uses Phabricator for code review. See the `Mozilla Phabricator +User +Guide <https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html>`__ +for instructions. + +Don't be shy in posting partial patches, demonstrating potential +approaches, and asking for preliminary feedback. It is easier for others +to comment, and offer suggestions, when a question is accompanied by +some code. + + +Getting reviews +--------------- + +Thorough code reviews are one of Mozilla's ways of ensuring code +quality. Every patch must be reviewed by the module owner of the code, +or one of their designated peers. + +For more information about the review process, see the :ref:`Code Review FAQ`. + +To request a review, you will need to specify one or more usernames +either when you submit the patch, or afterward in the UI. See the +`Mozilla Phabricator User +Guide <https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html>`__ +for details. + +*Getting attention:* If a reviewer doesn't respond within a week, or so, +of the review request: + +- Join #developers on Mozilla's `Matrix + server <https://chat.mozilla.org>`__, and ask if anyone knows why a + review may be delayed. Please link to the bug too. +- If the review is still not addressed, mail the reviewer directly, + asking if/when they'll have time to review the patch, or might + otherwise be able to review it. +- As a last resort, ask in the appropriate newsgroup on + ``news.mozilla.org``. + + +Addressing review comments +-------------------------- + +It is unusual for patches to be perfect the first time around. The +reviewer may use the ‘Request Changes’ +`action <http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#reviewing-patches>`__ +and list problems that must be addressed before the patch can be +accepted. Please remember that requesting revisions is not meant to +discourage participation, but rather to encourage the best possible +resolution of a bug. Carefully work through the changes that the +reviewer recommends, attach a new patch, and request review again. + +Sometimes a reviewer will grant conditional review with the ‘Accept +Revision’ action but will also indicate minor necessary changes, such as +spelling, or indentation fixes. All recommended corrections should be +made, but a re-review is unnecessary. Make the changes and submit a new +patch. If there is any confusion about the revisions, another review +should be requested. + +Sometimes, after a patch is reviewed, but before it can be committed, +someone else makes a conflicting change. If the merge is simple, and +non-invasive, post an updated version of the patch. For all non-trivial +changes, another review is necessary. + +If at any point the review process stalls for more than two weeks, see +the previous 'Getting attention' section. + +In many open source projects, developers will accept patches in an +unfinished state, finish them, and apply the completed code. In +Mozilla's culture, **the reviewer will only review and comment on a +patch**. If a submitter declines to make the revisions, the patch will +sit idle, until someone chooses to take it on. + + +Pushing the change +------------------ + +A patch can be pushed (aka. 'landed') after it has been properly +reviewed. + +.. note:: + + Note: Be sure to build the application with the patch applied. This + ensures it runs as expected, passing automated tests, and/or runs + through the `try + server <https://wiki.mozilla.org/Build:TryServerAsBranch>`__. In the + bug, please also mention you have completed this step. + + Submitting untested patches wastes the committer's time, and may burn + the release tree. Please save everyone's time and effort by + completing all necessary verifications. + + +Add the tag "check-in needed" on the revision(s) in phabricator. To do +so, click on the "Edit" button on a phabricator revision, and start +typing "check-in needed" in the Tags field. It should auto-complete. If +Phabricator doesn't allow you to add the keyword, ask someone else to +add it. Community members with commit access, regularly search for +revisions with the checkin-needed keyword, often committing in a day or +so. If the patch does not get checked, within a reasonable timeframe, +join #developers on `chat.mozilla.org <https://chat.mozilla.org>`__, asking +someone to check on your behalf. In most circumstances, a link to a +passing Try run will be required, in order to verify the patch will not +cause any new failures after landing. + +If pushing the patch yourself, please follow :ref:`Committing rules and responsibilities`. +`Lando <https://moz-conduit.readthedocs.io/en/latest/lando-user.html>`__ is used +to automatically land your code. + + +Regressions +----------- + +It is possible your code causes functional or performance regressions. +There is a tight +`policy <https://www.mozilla.org/about/governance/policies/regressions/>`__ on +performance regressions, in particular. This means your code may be +dropped, leaving you to fix and resubmit it. Regressions, ultimately +mean the tests you ran before checking in are not comprehensive enough. +A resubmitted patch, or a patch to fix the regression, should be +accompanied by appropriate tests. + +After authoring a few patches, consider `getting commit access to +Mozilla source code <https://www.mozilla.org/about/governance/policies/commit/>`__. |