Page History

...

1. We define a formal "Reviewer" role, similar to the JDK project.
   
   
2. The code review policies recognize the following different types of changes, with different thresholds for review:
   1. Simple, low-impact fixes: 1 Reviewer
   2. Higher-impact fixes: 2 Reviewers + allow sufficient time (1 day recommended) for others to chime in
   3. Features / API changes: CSR for approving the change, including approval by a "lead"; implementation then needs 2 Reviewers for the code (as with other "higher-impact" fixes above)
      
      
3. A review can either must be done submitted via a webrev posted to httppull request against the https://crgithub.com/openjdk.java.net/ (with review comments in JBS or on the mailing list), an in-line diff in the bug report for tiny one or two line changes, or a fix developed in the GitHub sandbox and submitted via a pull request (with review comments in the PR itself). In the latter case, an email must be sent to openjfx-dev announcing the review, and all other review policies must be satisfied, before the PR is merged into the develop branch on GitHub. If so, the PR itself will serve as the review./jfx repo.

Details

Code reviews are important to maintain high-quality contributions, but we recognize that not every type of change needs the same level of review. Without lowering our standards of quality, we want to make it easier to get low-impact changes (simple bug fixes) accepted.

...

2. Code review policies

All code reviews must be announced on the openjfx-dev mailing list submitted via a pull request against the https://github.com/openjdk/jfx repo -- even simple fixes. Formalizing existing practice, the announcement email subject should be of the format

RFR : <JBS bug id> : <bug synopsis>

This implies that a bug ID must exist in JBS to request a code reviewA JBS bug ID must exist before the pull request will be reviewed. See CONTRIBUTING.md for information on how to submit a pull request.

All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. If there is disagreement as to whether a fix is low-impact or high-impact, then it is considered high-impact. In other words we will always err on the side of quality by "rounding up" to the next higher category. The contributor can say whether they think something is low-impact or high-impact, but It is up to a Reviewer to confirm this. This should be done and recorded in the same manner as the review comments.

New code should be formatted consistently in accordance with the Code Style Rules. However, please do not reformat code as part of a bug fix. The makes more changes for code reviewers to track and review, and can lead to merge conflicts. If you want to reformat a class, make a changeset commit that has only formatting changes.

...

One (1) "R"eviewer is sufficient to accept such changes. As a courtesy, and to avoid changes which later might need to be backed out, if you think there might be some concern or objection to the change, please give sufficient time for folks who might be in other time zones the chance to take a look. This should be left up to the judgment of the reviewer Reviewer who approves it as well as the contributor.

...

The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel.

3. Streamlined review process for changes developed on GitHub

A GitHub pull-request (PR) targeted to the 'develop' branch of the https://github.com/javafxports/openjdk-jfx repository can serve as the formal review for a fix, instead of a webrev, thus allowing it to be integrated into main line 'jfx-dev' HG repo without an additional review, provided all of the review policies are followed prior to merging the PR into the 'develop' branch. In particular:

A. A JBS bug ID exists for the issue being fixed. That JBS issue should have the label "github-bug" and an issue Link to the PR.

B. All code review policies as outlined above in #2 were followed *prior to* the PR being approved and merged into the develop branch on GitHub. This includes sending the "Request for Review" (RFR) email to openjfx-dev when you get to the point that you intend to have the PR formally reviewed for merging into the develop branch. This will give other reviewers who may not be watching all PRs a chance to comment before it is merged.

C. The CI tests pass, which also ensures that the changeset is "whitespace clean". Note that this is necessary, but not sufficient testing, especially for code that deals with WebKit or media components, which are not completely built by the CI tools (for now), or that might affect the visual output, since the CI system cannot run headful tests.D. The PR was squashed / merged into the develop branch as a single commit with no follow-on fixes merged into develop for the same issue after the commit is merged (each commit into develop corresponds to one JBS bug ID); all committers should do this as a general practice anyway

Anchor
ReviewersTable ReviewersTable

List of Reviewers

...