OpenJFX Project Policies

OpenJFX is an OpenJDK Project, and is bound by the OpenJDK By-Laws. Within that framework, the following policies are in effect:

Project Stewardship

The OpenJFX Project is guided by the Project Leads and Reviewers.

Project Co-Lead: Kevin Rushforth (kcr)
Project Co-Lead: Johan Vos (jvos)

Reviewers

TABLE OF REVIEWERS

We might decide in the future to use the formal OpenJDK Reviewer role. If so, then the above list will be replaced by a pointer to the census at that time. The intention, though, is that this list of Reviewers be functionally equivalent to the OpenJDK Reviewer role.

Code Review Policies

The short version of the OpenJFX review policy is:

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 be done via a webrev (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.

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.

In support of this, the following review policies are in effect. Many of these will involve judgment calls, especially when it comes to deciding whether a fix is low impact vs. high-impact, and that's OK. It doesn't have to be perfect.

1. The Reviewer role for the OpenJFX Project

We define a formal "Reviewer" role, similar to the JDK project. A Reviewer is responsible for reviewing code changes and helping to determine whether a change is suitable for including into OpenJFX. We expect Reviewers to feel responsible not just for their piece, but for the quality of the JavaFX library as a whole. In other words, the role of Reviewer is one of stewardship.

An experienced Committer can eventually become a Reviewer by providing good quality fixes and participating in code reviews over time, demonstrating the high-level of skill and understanding needed to be a competent reviewer. The JDK uses a threshold of 32 significant contributions. Without wanting to relax this standard too much, one thing we may consider is that a Committer with, say, 24 commits, who regularly participates in reviews, offering good feedback, might be just as good a reviewer (or maybe even better) as someone with 32 commits who rarely, if ever, provides feedback on proposed bug fixes. This is meant to be a somewhat loose guideline. It is up to the Reviewers and the Project Leads to decide whether and when a new Committer is ready to become a Reviewer.

2. Code review policies

All code reviews must be announced on the openjfx-dev mailing list -- 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 review. 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.

A. Low-impact bug fixes.

These are typically isolated bug fixes with little or no impact beyond fixing the bug in question; included in this category are test fixes (including new tests), doc fixes, and fixes to sample applications (including new samples).

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 who approves it as well as the contributor.

B. Higher impact bug fixes or RFEs.

These include changes to the implementation that potentially have a performance or behavioral impact, or are otherwise broad in scope. Some larger bug fixes will fall into this category, as will any fixes in high-risk areas (e.g., CSS).

Two (2) reviewers must approve to accept such changes, at least one of whom must be a "R"eviewer. Additionally, the review should allow sufficient time for folks who might be in other time zones the chance to review if they have concerns. A suggested wait time is 24 hours (not including weekends / or major holidays).

C. New features / API additions.

This includes behavioral changes, additions to the FXML or CSS spec, etc.

Feature requests come with a responsibility beyond just saying "here is the code for this cool new feature, please take it". There are many factors to consider for even small features. Larger features will need a significant contribution in terms of API design, coding, testing, maintainability, etc.

A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take.

To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR [3] is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above.

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

THE FOLLOWING INFORMATION IS OUT OF DATE...

UPDATE: Post-commit reviews are no longer allowed.

Code reviews exist to do three principal things:

1. Catch bugs (both trivial and deep problems)
2. Educate team members (prevent silos from forming)
3. Cause developers to think it through (performance, security, cleanliness, etc)

Catching trivial bugs is often less effective in a code review. With tools like CheckStyle and FindBugs and Xlint we can automate part of the process and get better (and more consistent) results than we will with human code review. Every minute a developer is spending doing something an automated tool can do, is time lost. Trivial changes such as formatting can be controlled by standardizing on a formatter (e.g., the NetBeans formatter works well and also supports removing trailing witespaces). All code that is checked in must be formatted in a single standard manner.

For experienced code owners, very deep bugs are sometimes found when the reviewer reviews the code. Developers must not commit and push code without first having the changes reviewed. This is a recipe for releasing code with prints and weird comments. The most common type of review is to create a webrev, publish it on http://cr.openjdk.java.net/, and post a request for review in JBS and on the mailing list. Another method for code review that is effective is for the developer and the reviewer to browse the code together and commit it when they are both satisfied. This gives the owner a chance to go over the work and it is well known that explaining something to someone else causes a deeper understanding for both. The review is interactive. If a trivial problem is found, it is fixed by both people and the review continues. If a deep but non-fatal issue is found, a follow on bug report can be entered and the review continues. If a fatal flaw is found or a flaw that will take some time to fix, the review ends and the developer fixes the problem. Code reviews like this are typical for major features or very hard problems.

Code Review Policy

The code review policy for OpenFX is this:

• Every change must be reviewed by at least one reviewer before it is pushed to the repo; complicated changes should have a second reviewer
• Each code area has an owner and at least one reviewer
• The owner has final say in what goes in to his area (subject to project leader override in rare cases)
• The reviewer is the primary go-to developer whenever the owner needs a review
• The owner should review all change sets which go into the code area, but can do so after the commit as long as a reviewer has approved it before it is committed
• For complicated or risky reviews, the reviewers should wait a reasonable amount of time before approving to give other interested parties who might be in other time zones a chance to spot problems

It is not necessary that every developer is owner of something. Not being an owner doesn't mean that you have no responsibility – perhaps your work is cross cutting and affects many different areas (like mnemonics or accessibility). In these cases you may own some small part but need to get reviews for the code that touches other owner's areas. Or maybe you are an architect and own very little but work all over the platform. In such cases you may not be an owner for the places where you are principally working.

A fundamental idea behind this strategy is that the owner and reviewers must have a shared model of how the code is changing, the bugs that are being fixed and who is making changes. Neither the owner nor the reviewer should be surprised by a code change.

IMPORTANT: Do not reformat code as part of a bug fix. The makes more changes for code reviewers to track and review,  If you want to reformat a class, make a changeset that has only formatting changes. Do not reformat code that you do not own.

No post-commit reviews

A former policy of the OpenJFX Project used to allow post-commit reviews in some cases. As the OpenJFX project becomes more mature, and as we move closer to integration with the rest of the JDK, including the use of jcheck, we are eliminating the notion of post-commit reviews. All reviews must happen before the code is committed and pushed.

Technical Discussions and Code Reviews

Too often, important design decisions and intricate code details are scattered around the mailing list, wiki and bug system. This makes it hard to understand why certain decisions were made and why things are implemented the way that they are. It is critical to OpenJFX that JIRA is the tool used to track the details. When an interesting discussion happens on the mailing list, move the discussion to JIRA and invite interested parties to watch the bug.

The team has been evaluating Crucible as a code review tool. Unfortunately, Crucible is not integrated with JBS and cannot be accessed by the community. Therefore, we have decided to continue using webrev and conduct code reviews in JBS bugs.

Starting a Review

When code is ready for review, the developer creates a webrev and makes it available at a public link location (see http://openjdk.java.net/guide/codeReview.html) or attaches a patch to the JIRA. For small code changes or in cases where the person contributing the code is not a committer, patches are acceptable.

Then they do the following:

• Add the list of reviewers to the watchers list in JIRA
• Add a comment to the JIRA with an indication that the review is starting, a link to the webrev, a list of desired reviewers, and any technical information that would assist reviewers. The following is a good example for the JIRA comment to start the review:
  . 
  Please review the following: http://cr.openjdk.java.net/foo/bar
  Reviewers: kcr, jgiles
  This is a really complicated, yet elegant fix to this issue. The idea is to do this, that, and the other thing, thus avoiding the xyz bug...

• Send an email to openjfx-dev@openjdk.java.net with a pointer to the JIRA as follows:
  . 
  Subject: [<release>] Review request for <bug-id>: <summary>
  To: <desired explicit reviewers>
  CC: <mailing list>
  Body: <link to the JIRA with a very short description of the problem>

All discussion and approvals will happen in JBS (not on the mailing list). In this manner all activity associated with the closing of the bug is captured in a single place. An exception to this is that openjfx-dev list members who do not have direct write access to JBS should send review comments to the mailing list. When the developer has addressed the points raised by the review, a new webrev is created and the review continues.