Changeset 20354 in webkit


Ignore:
Timestamp:
Mar 20, 2007 5:46:43 PM (17 years ago)
Author:
bdash
Message:

2007-03-20 Matt Lilek <​pewtermoose@gmail.com>

Reviewed by Darin.

Clear up the steps necessary to get a patch landed.

​http://bugs.webkit.org/show_bug.cgi?id=12877 - Bug 12877: "Contributing Code" page could be clearer
​http://bugs.webkit.org/show_bug.cgi?id=8690 - Bug 8690: Contributing code doesn't mention what to do with new files

  • coding/contributing.html:
Location:
trunk/WebKitSite
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitSite/ChangeLog

    r20303 r20354  
     12007-03-20  Matt Lilek  <pewtermoose@gmail.com>
     2
     3        Reviewed by Darin.
     4
     5        Clear up the steps necessary to get a patch landed.
     6
     7        http://bugs.webkit.org/show_bug.cgi?id=12877 - Bug 12877: "Contributing Code" page could be clearer
     8        http://bugs.webkit.org/show_bug.cgi?id=8690 - Bug 8690: Contributing code doesn't mention what to do with new files
     9
     10        * coding/contributing.html:
     11
    1122007-03-18  Maciej Stachowiak  <mjs@apple.com>
    213

  • trunk/WebKitSite/coding/contributing.html

    r20025 r20354  
    33    include("../header.inc");
    44?>
     5<h2>Contributing Code</h2>
     6<p>Contributing code to the WebKit project is a straightforward process.
     7Once you have the code <a href="/building/checkout.html">checked out</a>, <a href="/building/build.html">built</a>, and made your changes, you'll need to do a few things in order to get it landed in the tree:</p>
     8<ol>
     9    <li>Make sure your changes meet the <a href="/coding/coding-style.html">code style guidelines</a>.</li>
     10    <li>Run the layout tests using the <tt>run-webkit-tests</tt> script and make sure they all pass.
     11        See the <a href="/quality/testwriting.html">testing page</a> for more information, as well as what you need to do if you've modified JavaScriptCore.</li>
     12    <li>Add any new files and layout tests to Subversion using the <tt>svn add</tt> command.</li>
     13    <li>Prepare a changelog entry. The <tt>prepare-ChangeLog</tt> script will create a stub entry for you.</li>
     14    <li>Create the patch using the <tt>svn-create-patch</tt> script.</li>
     15    <li>Upload the patch for review. In Bugzilla, be sure to set the <tt>review:?</tt> flag.</li>
     16    <li>Make any changes recommended by the reviewer.</li>
     17    <li>Once reviewed, get your patch landed in the tree and watch for any regressions it may have caused (hopefully none)!</li>
     18</ol>
    519
    6 <h2>Contributing Code</h2>
    720
    8 <h3>Submitting Code Changes</h3>
     21<h3>Code Style Guidelines</h3>
     22<p>In order for your patch to be landed, it's necessary that it comply to the <a href="/coding/coding-style.html">code style guidelines</a>.
     23There are some older parts of the codebase that do not always follow these guidelines. If you come across code like this,
     24it's generally best to clean it up to comply with the new guidelines.</p>
    925
    10 <p>Contributing code to the WebKit project is a straightforward process.
    11 One you have checked out and built the code, you can make changes to it and then produce a <i>patch file</i>
    12 that contains only the differences between the current version of WebKit and your version of WebKit with your code changes.</p>
     26<h3>Regression tests</h3>
     27<p>Once you have made your changes, you need to run the regression tests, which is done via the <tt>run-webkit-tests</tt> script.
     28All tests must pass.  Patches will not be landed in the tree if they break existing layout tests.</p>
    1329
    14 <p>The <tt>svn-create-patch</tt> script, found in WebKitTools/Scripts along with the other WebKit scripts, should be used to produce patches.
    15 It does a <tt>svn</tt> <tt>diff</tt> operation, passing appropriate options to diff:</p>
    16 <p class="code">WebKitTools/Scripts/svn-create-patch > MyExcellentPatch.txt</p>
    17 <p>It's handy to put the <tt>WebKitTools/Scripts</tt> directory in your shell path so you can type commands like <tt>svn-create-patch</tt>
    18 without specifying the path to the script.</p>
    19 <p>The <tt>svn-apply</tt> and <tt>svn-unapply</tt> scripts are handy for applying patches to a tree, and rolling patches out of a tree.
    20 They go beyond the capabilities of the <tt>patch</tt> tool by handling files added and removed from the repository.</p>
     30<p>For any feature that affects the layout engine, a new regression test must be constructed. If you provide a patch that fixes a bug,
     31that patch should also include the addition of a regression test that would fail without the patch and succeed with the patch.
     32If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing the test up
     33front and making sure it's attached to the bug. If no layout test can be (or needs to be) constructed for the fix, you must explain
     34why a new test isn't necessary to the reviewer.</p>
    2135
    22 <p>Before you create your patch, you should make sure it has a Changelog entry by using the <tt>prepare-ChangeLog</tt> script.  It sets up a
    23 template for all of the Changelog entries you will have to fill in for your patch based on everything you've changed.</p>
     36<p>Information on writing a layout test as well as what needs to be done if you've made changes to JavaScriptCore
     37can be found on the <a href="/quality/testwriting.html">testing page</a>.</p>
    2438
    25 <p>Make sure that your patch meets the
    26 <a href="../coding/coding-style.html">coding style guidelines</a> and has received sufficient testing.
    27 Bug fixes should include a <a href="../quality/testing.html">new WebKit or JavaScriptCore test</a>.</p>
     39<h3>Adding new files</h3>
     40<p>New files and layout tests must be added to Subversion or else they won't be included in your patch. This is done with the <tt>svn add</tt> command.
     41More information on Subversion commands can be found via <tt>svn help</tt> or the <a href="http://svnbook.red-bean.com/">Version Control with Subversion</a> online book.</p>
    2842
    29 <p>Once you have a patch file, it must be reviewed by one of the approved WebKit reviewers.
    30 To request a review, attach the patch to the bug report, and mark the patch with the flag <tt>review:?</tt>. This will automatically
    31 send mail to <a href="http://lists.webkit.org/mailman/listinfo/webkit-reviews">webkit-reviews@lists.webkit.org</a> on your behalf. The
    32 <a href="../quality/lifecycle.html">WebKit Bug Life Cycle</a> page
    33 has more information about the stages of a WebKit Bugzilla bug.</p>
     43<h3>ChangeLog</h3>
     44<p>All patches require an entry to the ChangeLog. The <tt>prepare-ChangeLog</tt> script will create a basic entry containing a list
     45of all files that have been changed.  Use this to write up a brief summary of the changes you've made.  Don't worry about the
     46"Reviewed by NOBODY (OOPS!)" line, the person landing your patch will fill this in.</p>
    3447
    35 <p>The reviewer will typically either approve the patch (by responding with an <tt>r=me</tt> in the bug report or in e-mail
    36 and marking the patch <tt>review:+</tt>)
    37 or request revisions to the patch (and mark the patch <tt>review:-</tt>).
    38 In rare cases a patch may be permanently rejected, meaning that the reviewer believes the feature should never be committed to the tree.
    39 The review process can consist of multiple iterations between you and the reviewer as revisions are made to your patch.</p>
     48<h3>Create the patch</h3>
     49<p>WebKit uses the <tt>svn-create-patch</tt> script to create patches. This script supplements Subversion's <tt>diff</tt> command
     50to better handle things like moved, added, and deleted files. This command is best run from the top level of your checkout to make
     51sure no changes are left out of your patch. It is not necessary to break a patch into multiple files.</p>
    4052
    41 <p>For any feature that affects the layout engine, a new regression test must be constructed.
    42 If you provide a patch that fixes a bug, that patch should also include the addition of a regression test that
    43 would fail without the patch and succeed with the patch.
    44 If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing
    45 the test up front and making sure it's attached to the bug.
    46 If no layout test can be (or needs to be) constructed for the fix, you must explain why a new test isn't necessary to the reviewer.</p>
     53<p><tt>svn-create-patch</tt> does not create a file automatically, you need to redirect the output yourself using something like: <tt>svn-create-patch > MyExcellentPatch.txt</tt></p>
    4754
    48 <p>In addition you must run the regression tests. The command to do that is:</p>
    49 <p class="code">WebKitTools/Scripts/run-webkit-tests</p>
    50 <p>It's handy to put the <tt>WebKitTools/Scripts</tt> directory in your shell path so you can type commands like
    51 <tt>run-webkit-tests</tt> without specifying the path to the script.</p>
    52 <p>Only if all layout tests pass
    53 (or if justification can be made for changing the expected results of the tests) will the patch be allowed in the tree.  It is the reviewer's
    54 responsibility to double-check that you have run the regression tests before signing off on the patch.</p>
     55<h3>Patch review</h3>
     56<p>Once you have a patch file, it must be reviewed by one of the approved WebKit reviewers. To request a review, attach the patch
     57to the bug report, and mark the patch with the flag <tt>review:?</tt>. The reviewer will typically either approve the patch
     58(by responding with an <tt>r=me</tt> in the bug report or in e-mail and marking the patch <tt>review:+</tt>) or request revisions
     59to the patch (and mark the patch <tt>review:-</tt>). In rare cases a patch may be permanently rejected, meaning that the reviewer
     60believes the feature should never be committed to the tree. The review process can consist of multiple iterations between you and
     61the reviewer as revisions are made to your patch.</p>
    5562
    56 <p>If you are modifying JavaScriptCore there is an additional test suite that you need to run.  For more details on the required testing you must
    57 do before landing a patch, see our <a href="../quality/testing.html">testing page</a>.
    58 
    59 <p>Once a patch has been reviewed, there are two options for getting it into the tree.
    60 If you have check-in privileges, you can land the patch immediately once it has been reviewed.
    61 If you do not have check-in privileges, then it is the reviewer's responsibility to land the patch in the tree.</p>
    62 
    63 <p>Your responsibility for the patch does not end with the patch landing in the tree.
    64 There may be regressions from your change or additional feedback from reviewers after the patch has landed.
     63<h3>Landing in the tree</h3>
     64<p>Once a patch is approved, someone with commit access will land your patch. Your responsibility for the patch does not end with
     65the patch landing in the tree. There may be regressions from your change or additional feedback from reviewers after the patch has landed.
    6566It is your responsibility to be available should regressions arise and to respond to additional feedback that happens after a check-in.</p>
    6667
    67 <h3>Obtaining Check-In Privileges</h3>
     68<h2>WebKit Scripts</h2>
     69<p><tt>WebKitTools/Scripts</tt> contains a number of scripts to help make life easier when submitting a patch. All scripts mentioned
     70on this page (and on the rest of the site as well) are located here unless otherwise mentioned.</p>
     71<p>It's handy to put this directory in your shell path so you can just type the commands without having to specify the path to
     72the script each time.</p>
    6873
     74
     75<h2>Obtaining Check-In Privileges</h2>
    6976<p>Contributors with a proven track record of good patch submissions and that have demonstrated an ability to work well with the community can
    7077obtain check-in privileges to the WebKit source tree. In order to obtain this check-in access, the contributor must find a reviewer who
    … …  
    7481Once the contributor sends a copy of the signed agreement to Apple, she or he receives check-in access.</p>
    7582
    76 <h3>Becoming a Reviewer</h3>
    7783
     84<h2>Becoming a Reviewer</h2>
    7885<p>A contributor with check-in access may also become a reviewer.  In order to become a reviewer, the current reviewers must agree that the
    7986contributor is effectively functioning as an expert in a particular area of the code and is qualified to review patches submitted in
Note: See TracChangeset for help on using the changeset viewer.