mozilla

Compare Revisions

How to Submit a Patch

Change Revisions

Revision 39618:

Revision 39618 by BenjaminSmedberg on

Revision 39619:

Revision 39619 by Ssmedberg on

Title:
How to Submit a Patch
How to Submit a Patch
Slug:
Developer_Guide/How_to_Submit_a_Patch
Developer_Guide/How_to_Submit_a_Patch
Tags:
"Developing Mozilla", Bugzilla
"Developing Mozilla", Bugzilla
Content:

Revision 39618
Revision 39619
n17      Every change to the code is tracked by a bug in&nbsp;<a clan17      Every change to the code is tracked by a bug in&nbsp;<a cla
>ss="link-https" href="https://bugzilla.mozilla.org/" title="https>ss="link-https" href="https://bugzilla.mozilla.org/" title="https
>://bugzilla.mozilla.org/">bugzilla.mozilla.org</a>. Search for an>://bugzilla.mozilla.org/">bugzilla.mozilla.org</a>; without a bug
> existing bug about the change, and file a new one if appropriate>, code will not be reviewed, and without review, code will not be
>. Without a bug it is impossible to get reviews. Most of the comm> accepted.&nbsp; To avoid repetition, search for an existing bug 
>unication about code changes takes place in the bug. Make sure th>about the change, and if none exists, file a new one.&nbsp; Most 
>at the bug describes the exact problem being solved.>of the communication about code changes takes place in the bug, s
 >o be sure that the bug describes the exact problem being solved.
n20      Make sure that bug is in the correct product and component.n20      Please verify that the bug is in the correct product and co
> In case of confusion ask on the newsgroups or on IRC.>mponent. For more infomation, ask questions on the newsgroups or 
 >on IRC channel #developers.
n23      The person who is working on a bug should be the "owner" ofn23      The person who is working on a bug should be the "owner" of
> that bug in bugzilla. If somebody else is currently the owner of> that bug in bugzilla. If somebody else is currently the owner of
> a bug, email that person to coordinate changes and avoid duplica> a bug, email that person to coordinate changes; if not, claim ow
>te work.>nership of the bug being addressed tod avoid duplicate work.
n29      Determine the <a class="internal" href="/en/Mozilla_Modulesn29      All code is supervised by a <a class="internal" href="/en/M
>_and_Module_Ownership" title="en/Mozilla Modules and Module Owner>ozilla_Modules_and_Module_Ownership" title="en/Mozilla Modules an
>ship">module owner</a> of the code being changed. This person wil>d Module Ownership">module owner</a>. This person will be respons
>l be responsible for reviewing and accepting the change. Before w>ible for reviewing and accepting the change. Before writing code,
>riting code, check with the module owner to make sure that the pr> determine the module owner and verify with him that the proposed
>oposed change is acceptable. The module owner may want to look ov> change is acceptable. He may want to look over any new user inte
>er any new user interface (UI review), functions (API review), or>rface (UI review), functions (API review), or testcases for the p
> testcases for the proposed change.>roposed change.
n32      If module ownership is not clear, ask on the newsgroups or n32      If module ownership is not clear, ask on the newsgroups or 
>on IRC. Another good place to determine ownership is the revision>on IRC. The revision log for the relevant file can also be helpfu
> log for the files being changed. For example, see the change log>l. For example, see the change log for {{ Source("browser/base/co
> for {{ Source("browser/base/content/browser.js") }}, which is av>ntent/browser.js") }}, by clicking the "Hg Log" link at the top o
>ailable via the "Hg Log" link at the top of <a class="external" h>f <a class="external" href="http://mxr.mozilla.org/mozilla/source
>ref="http://mxr.mozilla.org/mozilla/source/">MXR</a> or by runnin>/">MXR</a> or by running <code>hg log browser/base/content/browse
>g <code>hg log browser/base/content/browser.js</code>. If someone>r.js</code>. The corresponding checkin message will contain somet
>'s reviewed a patch, the corresponding checkin message will conta>hing like "r=nickname", identifying potential reviewers for the c
>in something like "r=nickname".>ode in question.
n44      Changes must be tested. In most cases, an <a class="internan44      All changes must be tested. In most cases, an <a class="int
>l" href="/en/Mozilla_automated_testing" title="En/Mozilla automat>ernal" href="/en/Mozilla_automated_testing" title="En/Mozilla aut
>ed testing">automated test</a> is required for every change to th>omated testing">automated test</a> is required for every change t
>e code. In some cases it may not be possible to write an automate>o the code. In some cases where it is not possible to write an au
>d test. In these cases manual tests called <a class="internal" hr>tomated test, manual tests called <a class="internal" href="/en/L
>ef="/en/Litmus_tests" title="En/Litmus tests">Litmus tests</a> ar>itmus_tests" title="En/Litmus tests">Litmus tests</a> are used.
>e used. 
n47      Ensure that the change has not caused regressions by runninn47      Ensure that the change has not caused regressions by runnin
>g the automated test suite, locally, or using the <a class="link->g the automated test suite, locally, or using the <a class="link-
>https" href="https://wiki.mozilla.org/Build:TryServer" title="htt>https" href="https://wiki.mozilla.org/Build:TryServer" title="htt
>ps://wiki.mozilla.org/Build:TryServer">Mozilla try server</a>. If>ps://wiki.mozilla.org/Build:TryServer">Mozilla try server</a>. Be
> you do not have access to the try server, ask the module owner o>cause access to the try server is limited, a module owner or deve
>r somebody on IRC&nbsp;to submit the job on your behalf.>loper on IRC&nbsp;may be willing to submit jobs for those without
 > try server privledges.&nbsp;
n53      After creating the patch, attach it to the bug using the "An53      Attach the finished patch to the bug using the "Add an atta
>dd an attachment" link in bugzilla. Check the "patch" box when at>chment" link in bugzilla. Check the "patch" box when attaching th
>taching the patch so that reviewers and bugzilla users can read t>e patch so that reviewers and bugzilla users can read it.
>he patch. 
n56      Don't be afraid to post partial patches demonstrating potenn56      Don't be afraid to post partial patches demonstrating poten
>tial approaches and ask for preliminary feedback on them. A quest>tial approaches and ask for preliminary feedback on them. It is e
>ion accompanied by code shows thought and is more likely to provo>asier for others to comment and offer suggestions when a question
>ke discussion and response.> is accompanied by code.
n62      Thorough code reviews are one of Mozilla's way of ensuring n62      Thorough code reviews are one of Mozilla's way of ensuring 
>code quality. Every patch must be reviewed by the module owner of>code quality. Every patch must be reviewed by the module owner of
> the code or one of his designated peers. In additionpatches wh> the code or one of his designated peers. Patches which cross mod
>ich cross modules, change APIs, or have security-related changes >uleschange APIs, or have security-related changes must have an 
>must have an additional <a class="external" href="http://www.mozi>additional <a class="external" href="http://www.mozilla.org/hacki
>lla.org/hacking/reviewers.html">super-review</a>. For more inform>ng/reviewers.html">super-review</a>. For more information, see th
>ation, see the <a class="internal" href="/en/Code_Review_FAQ" tit>e <a class="internal" href="/en/Code_Review_FAQ" title="En/Code R
>le="En/Code Review FAQ">code review FAQ</a>.>eview FAQ">code review FAQ</a>.
n65      <img align="right" alt="request-review.png" class="internaln65      <img align="right" alt="request-review.png" class="internal
> rwrap" src="/@api/deki/files/3598/=request-review.png" style="wi> rwrap" src="/@api/deki/files/3598/=request-review.png" style="wi
>dth: 519px; height: 149px; border: 1px solid black;">To request r>dth: 519px; height: 149px; border: 1px solid black;">To request a
>eview or super-review, click the Details link for the attachment > review or super-review, click the Details link for the attachmen
>in bugzilla. On the left side are dropdowns with various labels; >t in bugzilla. On the left side are dropdowns with various labels
>find the&nbsp;"review" entry and change the flag to "?"; enter th>.&nbsp; Find the&nbsp;"review" entry and change the flag to "?", 
>e bugmail address of the module owner or peer who will be reviewi>and enter the bugmail address of the module owner or peer who wil
>ng the patch. Remember to submit changes!>l be reviewing the patch. Remember to submit changes!
n71      <li>Join <a class="link-irc" href="irc://irc.mozilla.org/den71      <li>Join <a class="link-irc" href="irc://irc.mozilla.org/de
>velopers">#developers</a> on Mozilla's IRC server and ask about i>velopers">#developers</a> on Mozilla's IRC server and ask if anyo
>t there.>ne knows why a review may be delayed.
n73      <li>Mail the reviewer directly, asking if/when he'll have tn73      <li>If the review is still not addressed, mail the reviewer
>ime to review the patch or who else might be able to review it.> directly, asking if/when he'll have time to review the patch or 
 >who else might be able to review it.
n75      <li>Ask in the appropriate newsgroup on <code>news.mozilla.n75      <li>As a last resort, ask in the appropriate newsgroup on <
>org</code>.>code>news.mozilla.org</code>.
n82      It is unusual for patches to be perfect the first time. Then82      It is unusual for patches to be perfect the first time. The
> reviewer may mark "review-" and list problems that must be addre> reviewer may mark "review-" and list problems that must be addre
>ssed before the patch can be accepted. Do not be downhearted! Car>ssed before the patch can be accepted. Please remember that reque
>efully work through the changes that the reviewer recommends, att>sting revisions is not meant to discourage participation, but rat
>ach a new patch and request review again.>her to encourage the best possible resolution of a bug.&nbsp; Car
 >efully work through the changes that the reviewer recommends, att
 >ach a new patch and request review again.
n85      Sometimes a reviewer will grant "conditional review" by marn85      Sometimes a reviewer will grant "conditional review" by mar
>king review+ but indicating minor changes that must be made, such>king review+ but indicating minor changes that must be made, such
> as spelling or indentation fixes. If this happens, he wants the > as spelling or indentation fixes. All recommended corrections sh
>changes made but doesn't feel it's necessary for him to re-review>ould be made, but a re-review is unnecessary. Make the changes, a
> them. Make the changes, post a new version with the changes made>ttach the revised version, use the relevant checkbox on the attac
> (obsoleting the old patch via the relevant checkbox on the attac>hment page to render the original patch obsolute, and carry over 
>hment page), and set any remaining flags (superreview, etc.) to t>any remaining flags (review+, super-review, etc.) to the new atta
>heir values from the previous version. If there is any confusion,>chment. If there is any confusion about the revisions, another re
> feel free to request on the changes.>view should be requested.&nbsp;
n88      Sometimes after a patch is reviewed, but before it can be cn88      Sometimes after a patch is reviewed, but before it can be c
>ommitted, someone else makes a conflicting change. If the merge s>ommitted, someone else makes a conflicting change. If the merge s
>imple and non-invasive, just post an updated version of the patch>imple and non-invasive, post an updated version of the patch, but
>. If the fix is more involved or not particularly obvious to a re> for all non-trivial changes, another review is necessary.
>ader familiar with the code, ask for review again. 
n91      If at any point the review process stalls for more than twon91      If at any point the review process stalls for more than two
> weeks, get somebody's attention again as above.> weeks, see the "Getting attention" section above.
n94      <strong>The reviewer only reviews</strong>: many open sourcn94      In many open source projects, developers will accept patche
>e developers will accept patches in an unfinished state, finish t>s in an unfinished state, finish them and apply them.&nbsp; In Mo
>hem up and apply them.&nbsp; In Mozilla's culture, the reviewer w>zilla's culture, <strong>the reviewer will only review and commen
>ill typically only review and comment on your patch; changes are >t on a patch</strong>; the submitted is expected to make revision
>entirely up to the submitter.&nbsp; There are several reasons for>s.
> this.&nbsp; 1) We want to make sure the reviewer is separate fro 
>m the developer, to guarantee the code gets many independent eyes 
> reviewing it.&nbsp; 2)&nbsp;We learn better from our mistakes by 
> fixing them.&nbsp; 3)&nbsp;Reviewers just don't have time to fix 
> up patches; if they did, the patch queues would become enormous. 
n100      After the patch has been properly reviewed, it's ready for n100      A patch can be committed after it has been properly reviewe
>the checkin. The module owner who reviewed the patch may not chec>d.
>k it in automatically; it is the bug owner's responsibility to ma 
>ke sure the patch is committed. 
n104        Please build the application with the patch applied and mn104        Please build the application with the patch applied, ensu
>ake sure it runs and passes automatic tests. Submitting an untest>ring it runs and passes automatic tests. Submitting an untested p
>ed patch wastes the committer's time or even burn the tree. If an>atch wastes the committer's time and may burn the tree. A history
> author's patches have a bad track record, people might avoid com> of untested patches will make others wary of future checkins; pl
>mitting them, since it takes more time.>ease save everyone's time and effort by completing all necessary 
 >verifications.
n108      Ask the module owner or reviewer to commit the patch. Altern108      Ask the module owner or reviewer to commit the patch, or ad
>nately, add <a class="link-https" href="https://bugzilla.mozilla.>d <a class="link-https" href="https://bugzilla.mozilla.org/descri
>org/describekeywords.cgi"><code>checkin-needed</code></a> to the >bekeywords.cgi"><code>checkin-needed</code></a> to the Keywords o
>Keywords of the bug; volunteers with commit access regularly sear>f the bug.&nbsp; Volunteers with commit access regularly search f
>ch for bugs with the checkin-needed keyword and commit within a w>or bugs with the checkin-needed keyword and commit within a week 
>eek or so. If the patch is not get checked in within a reasonable>or so. If the patch does not get checked in within a reasonable t
> timeframe, join <a class="external" href="http://irc.mozilla.org>imeframe, join <a class="external" href="http://irc.mozilla.org/d
>/developers">#developers</a> on <a class="external" href="http://>evelopers">#developers</a> on <a class="external" href="http://ir
>irc.mozilla.org/">irc.mozilla.org</a> and ask people there to che>c.mozilla.org/">irc.mozilla.org</a> and ask someone to check it o
>ck it on your behalf.>n your behalf.
t111      After authoring a few patches, feel free to consider <a hret111      After authoring a few patches, consider <a href="/en/Gettin
>f="/en/Getting_commit_access_to_Mozilla_source_code" title="en/Ge>g_commit_access_to_Mozilla_source_code" title="en/Getting_commit_
>tting_commit_access_to_Mozilla_source_code">Getting commit access>access_to_Mozilla_source_code">Getting commit access to Mozilla s
> to Mozilla source code</a>.>ource code</a>.

Back to History