Revision 549939 of How to Submit a Patch

  • Revision slug: Developer_Guide/How_to_Submit_a_Patch
  • Revision title: How to Submit a Patch
  • Revision id: 549939
  • Created:
  • Creator: Karlt
  • Is current revision? No
  • Comment removed a comma that should not have been added

Revision Content

Submitting a patch and getting it reviewed and committed to the Mozilla source tree involves several steps:

Workflow of submitting a patch: Preparation | Module Ownership | Creating a patch
| Testing | Getting Reviews | Addressing Review Comments | Committing the Patch

Preparation

Every change to the code is tracked by a bug in 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 about the change, and if none exists, file a new one. Most of the communication about code changes takes place in the bug, so be sure that the bug describes the exact problem being solved.

Please verify that the bug is in the correct product and component. For more infomation, ask questions on the newsgroups or on the #developers IRC channel.

The person who is working on a bug should be the "assignee" of that bug in bugzilla. If somebody else is currently the assignee of a bug, email that person to coordinate changes; if not, leave a message in the bug stating that you are working on it and suggest that someone with bug-editing privileges assign it to you.

Module ownership

All code is supervised by a module owner. This person will be responsible for reviewing and accepting the change. Before writing code, determine the module owner and verify with them that the proposed change is 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 IRC. The revision log for the relevant file can 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 MXR or by running hg log browser/base/content/browser.js. The corresponding checkin message will contain something like "r=nickname", identifying potential reviewers for the code in question.

Creating a patch

Changes to the Mozilla source code are presented in the form of a patch. See "How can I generate a patch" for more information about creating patches which are formatted properly for easy review.

Each patch should represent a single complete change. Separate distinct changes into multiple patches. If your change results in a large, complex patch, then see whether it can be broken down into smaller, easy to understand patches representing complete steps, applied on top of each other (the mq extension for mercurial can be of help here). This will make it easier to review your changes, leading to quicker reviews and better confidence in the review.

Testing

All changes must be tested. In most cases, an automated test is required for every change to the code. In some cases where it is not possible to write an automated test, manual tests called Litmus tests are used.

Ensure that the change has not caused regressions by running the automated test suite, locally, or using the Mozilla try server. A module owner or developer on IRC may be willing to submit jobs for those without try server privileges.

Attaching the patch

Attach the finished patch to the bug using the "Add an attachment" link in bugzilla. Check the "patch" box when attaching the patch so that reviewers and bugzilla users can read it.

Don't be afraid to post partial patches demonstrating potential approaches and ask for preliminary feedback on them. It is easier for others to comment and offer suggestions when a question is accompanied by 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 his designated peers. Patches which cross modules, change APIs, or have security-related changes must have an additional super-review.

See "Who should review my change?" for a way to find a reviewer. For more information about the review process, see the code review FAQ. If your change affects user interface, see "Requesting feedback and ui-review for desktop Firefox front-end changes".

request-review.pngTo request a review or super-review, click the Details link for the attachment in bugzilla. On the left side are dropdowns with various labels.  Find the "review" entry and change the flag to "?", and enter the email address of the module owner or peer who will be reviewing the patch. Remember to submit changes!

Getting attention: If a reviewer doesn't respond within a week or so of the review request:

  • Join #developers on Mozilla's IRC server and ask if anyone knows why a review may be delayed (please link to the bug in question, as well).
  • If the review is still not addressed, mail the reviewer directly, asking if/when he'll have time to review the patch or who else might 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. The reviewer may mark "review-" 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 by marking "review+" but indicating minor changes that must be made, such as spelling or indentation fixes. All recommended corrections should be made, but a re-review is unnecessary. Make the changes, attach the revised version, and use the relevant checkbox on the attachment page to render the original patch obsolete. 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, but for all non-trivial changes, another review is necessary.

If at any point the review process stalls for more than two weeks, see the "Getting attention" section above.

In many open source projects, developers will accept patches in an unfinished state, finish them and apply them.  In Mozilla's culture, the reviewer will only review and comment on a patch.  If the submitter declines to make the revisions, the patch will sit unless and until someone else chooses to take up the effort.

Committing the patch

A patch can be committed after it has been properly reviewed.

Please build the application with the patch applied, ensuring it runs and passes automatic tests (and mention you have done so in the bug) and/or push to the try server (also stating this in the bug). Submitting an untested patch wastes the committer's time and may burn the tree. Please save everyone's time and effort by completing all necessary verifications.

Please ensure that the patch is properly formatted to make it as easy as possible for a committer to check in your patch.

Add the checkin-needed keyword to the bug (or if Bugzilla doesn't allow you to add the keyword, ask someone else to add it). Volunteers with commit access regularly search for bugs with the checkin-needed keyword and commit within a week or so. If the patch does not get checked in within a reasonable timeframe, join #developers on irc.mozilla.org and ask someone to check it on your behalf.

If you're committing yourself, please remember to follow Committing Rules and Responsibilities.

Regressions

It could be that your code causes functional or performance regressions. There is a tight policy on performance regressions in particular, which means your code may well end up being backed out and you will have to fix it and resubmit it. Regressions mean the tests (which you ran before checking in, didn't you) are not comprehensive enough, so 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.

{{ languages( { "ja": "ja/Getting_your_patch_in_the_tree" } ) }}

Revision Source

<p>Submitting a patch and getting it reviewed and committed to the Mozilla source tree involves several steps:</p>
<p style="text-align: center;"><img alt="Workflow of submitting a patch: Preparation | Module Ownership | Creating a patch
| Testing | Getting Reviews | Addressing Review Comments | Committing the Patch" class="internal default" src="/@api/deki/files/3599/=submitting-patch-workflow.png" style="width: 267px; height: 348px;" /></p>
<h2 id="Preparation">Preparation</h2>
<p>Every change to the code is tracked by a bug in&nbsp;<a class="link-https" href="https://bugzilla.mozilla.org/" title="https://bugzilla.mozilla.org/">bugzilla.mozilla.org</a>; without a bug, code will not be reviewed, and without review, code will not be accepted. To avoid duplication, <a class="link-https" href="https://bugzilla.mozilla.org/query.cgi?format=specific">search for an existing bug</a> about the change, and if none exists, file a new one. Most of the communication about code changes takes place in the bug, so be sure that the bug describes the exact problem being solved.</p>
<p>Please verify that the bug is in the correct product and component. For more infomation, ask questions on the newsgroups or on the #developers IRC channel.</p>
<p>The person who is working on a bug should be the "assignee" of that bug in bugzilla. If somebody else is currently the assignee of a bug, email that person to coordinate changes; if not, leave a message in the bug stating that you are working on it and suggest that someone with bug-editing privileges assign it to you.</p>
<h2 id="Module_ownership">Module ownership</h2>
<p>All code is supervised by a <a class="internal" href="/en/Mozilla_Modules_and_Module_Ownership" title="en/Mozilla Modules and Module Ownership">module owner</a>. This person will be responsible for reviewing and accepting the change. Before writing code, determine the module owner and verify with them that the proposed change is acceptable. They may want to look over any new user interface (UI review), functions (API review), or testcases for the proposed change.</p>
<p>If module ownership is not clear, ask on the newsgroups or on IRC. The revision log for the relevant file can 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 <a class="external" href="http://mxr.mozilla.org/mozilla/source/">MXR</a> or by running <code>hg log browser/base/content/browser.js</code>. The corresponding checkin message will contain something like "r=nickname", identifying potential reviewers for the code in question.</p>
<h2 id="Creating_a_patch">Creating a patch</h2>
<p>Changes to the Mozilla source code are presented in the form of a patch. See "<a href="/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F" title="/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F">How can I generate a patch</a>" for more information about creating patches which are formatted properly for easy review.</p>
<p>Each patch should represent a single complete change. Separate distinct changes into multiple patches. If your change results in a large, complex patch, then see whether it can be broken down into <a href="https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/#many-small-commits">smaller, easy to understand patches representing complete steps</a>, applied on top of each other (the <a href="/en/Mercurial_Queues" title="en/Mercurial Queues">mq extension</a> for mercurial can be of help here). This will make it easier to review your changes, <a class="external" href="http://groups.google.com/group/mozilla.dev.planning/msg/2f99460f57f776ef?hl=en" title="http://groups.google.com/group/mozilla.dev.planning/msg/2f99460f57f776ef?hl=en">leading to quicker reviews </a>and better confidence in the review.</p>
<h2 id="Testing">Testing</h2>
<p>All changes must be tested. In most cases, an <a class="internal" href="/en/Mozilla_automated_testing" title="En/Mozilla automated testing">automated test</a> is required for every change to the code. In some cases where it is not possible to write an automated test, manual tests called <a class="internal" href="/en/Litmus_tests" title="En/Litmus tests">Litmus tests</a> are used.</p>
<p>Ensure that the change has not caused regressions by running the automated test suite, locally, or using the <a class="link-https" href="https://wiki.mozilla.org/Build:TryServer" title="https://wiki.mozilla.org/Build:TryServer">Mozilla try server</a>. A module owner or developer on IRC&nbsp;may be willing to submit jobs for those without try server privileges.</p>
<h2 id="Attaching_the_patch">Attaching the patch</h2>
<p>Attach the finished patch to the bug using the "Add an attachment" link in bugzilla. Check the "patch" box when attaching the patch so that reviewers and bugzilla users can read it.</p>
<p>Don't be afraid to post partial patches demonstrating potential approaches and ask for preliminary feedback on them. It is easier for others to comment and offer suggestions when a question is accompanied by code.</p>
<h2 id="Getting_the_patch_reviewed" name="Getting_the_patch_reviewed">Getting reviews</h2>
<p>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 his designated peers. Patches which cross modules, change APIs, or have security-related changes must have an additional <a class="external" href="http://www.mozilla.org/hacking/reviewers.html">super-review</a>.</p>
<p>See <a class="external" href="http://weblog.latte.ca/blake/employment/mozilla/thunderbird/reviewerChooser" title="http://weblog.latte.ca/blake/employment/mozilla/thunderbird/reviewerChooser">"Who should review my change?"</a> for a way to find a reviewer. For more information about the review process, see the <a class="internal" href="/en/Code_Review_FAQ" title="En/Code Review FAQ">code review FAQ</a>. If your change affects user interface, see "<a href="/En/Developer_Guide/Requesting_feedback_and_ui-review_for_desktop_Firefox_front-end_changes" title="Requesting feedback and ui-review for desktop Firefox front-end changes">Requesting feedback and ui-review for desktop Firefox front-end changes</a>".</p>
<p><img align="right" alt="request-review.png" class="internal rwrap" src="/@api/deki/files/3598/=request-review.png" style="width: 519px; height: 149px; border: 1px solid black;" />To request a review or super-review, click the Details link for the attachment in bugzilla. On the left side are dropdowns with various labels.&nbsp; Find the&nbsp;"review" entry and change the flag to "?", and enter the email address of the module owner or peer who will be reviewing the patch. Remember to submit changes!</p>
<p><em>Getting attention:</em>&nbsp;If a reviewer doesn't respond within a week or so of the review request:</p>
<ul>
 <li>Join <a class="link-irc" href="irc://irc.mozilla.org/developers">#developers</a> on Mozilla's IRC server and ask if anyone knows why a review may be delayed (please link to the bug in question, as well).</li>
 <li>If the review is still not addressed, mail the reviewer directly, asking if/when he'll have time to review the patch or who else might be able to review it.</li>
 <li>As a last resort, ask in the appropriate newsgroup on <code>news.mozilla.org</code>.</li>
</ul>
<h2 id="Addressing_review_comments">Addressing review comments</h2>
<p>It is unusual for patches to be perfect the first time. The reviewer may mark "review-" 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.&nbsp; Carefully work through the changes that the reviewer recommends, attach a new patch and request review again.</p>
<p>Sometimes a reviewer will grant conditional review by marking "review+" but indicating minor changes that must be made, such as spelling or indentation fixes. All recommended corrections should be made, but a re-review is unnecessary. Make the changes, attach the revised version, and use the relevant checkbox on the attachment page to render the original patch obsolete. If there is any confusion about the revisions, another review should be requested.&nbsp;</p>
<p>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, but for all non-trivial changes, another review is necessary.</p>
<p>If at any point the review process stalls for more than two weeks, see the "Getting attention" section above.</p>
<p>In many open source projects, developers will accept patches in an unfinished state, finish them and apply them.&nbsp; In Mozilla's culture, <strong>the reviewer will only review and comment on a patch</strong>.&nbsp; If the submitter declines to make the revisions, the patch will sit unless and until someone else chooses to take up the effort.</p>
<h2 id="Getting_the_patch_checked_into_the_tree" name="Getting_the_patch_checked_into_the_tree">Committing the patch</h2>
<p>A patch can be committed after it has been properly reviewed.</p>
<div class="note">
 <p>Please build the application with the patch applied, ensuring it runs and passes automatic tests (and mention you have done so in the bug) and/or push to the <a class="link-https" href="https://wiki.mozilla.org/Build:TryServerAsBranch" title="https://wiki.mozilla.org/Build:TryServerAsBranch">try server</a> (also stating this in the bug). Submitting an untested patch wastes the committer's time and may burn the tree. Please save everyone's time and effort by completing all necessary verifications.</p>
 <p>Please ensure that the patch is <a href="/en/Creating_a_patch_that_can_be_checked_in" title="https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in">properly formatted</a> to make it as easy as possible for a committer to check in your patch.</p>
</div>
<p>Add the <a class="link-https" href="https://bugzilla.mozilla.org/describekeywords.cgi"><code>checkin-needed</code></a> keyword to the bug (or if Bugzilla doesn't allow you to add the keyword, ask someone else to add it). Volunteers with commit access regularly search for bugs with the checkin-needed keyword and commit within a week or so. If the patch does not get checked in within a reasonable timeframe, join <a class="external" href="http://irc.mozilla.org/developers">#developers</a> on <a class="external" href="http://irc.mozilla.org/">irc.mozilla.org</a> and ask someone to check it on your behalf.</p>
<p>If you're committing yourself, please remember to follow <a href="/En/Developer_Guide/Committing_Rules_and_Responsibilities" title="en/Developer Guide/Committing Rules and Responsibilities">Committing Rules and Responsibilities</a>.</p>
<h2 id="Getting_the_patch_checked_into_the_tree" name="Getting_the_patch_checked_into_the_tree">Regressions</h2>
<p>It could be that your code causes functional or performance regressions. There is a tight <a class="external" href="http://www.mozilla.org/hacking/regression-policy.html" title="http://www.mozilla.org/hacking/regression-policy.html">policy</a> on performance regressions in particular, which means your code may well end up being backed out and you will have to fix it and resubmit it. Regressions mean the tests (which you ran before checking in, didn't you)&nbsp;are not comprehensive enough, so a resubmitted patch or a patch to fix the regression should be accompanied by appropriate tests.</p>
<p>After authoring a few patches, consider <a href="http://www.mozilla.org/hacking/committer/" title="http://www.mozilla.org/hacking/committer/">getting commit access to Mozilla source code</a>.</p>
<p>{{ languages( { "ja": "ja/Getting_your_patch_in_the_tree" } ) }}</p>
Revert to this revision