OpenJFX is a single project and any committer should feel comfortable proposing changes and patches to any part of this project. We will have a single set of processes that apply to the entire project. However that does not mean that it is a free-for-all.

Owners and Reviewers

OpenJFX consists of a few high level components that come together to form the toolkit. While there is a single project lead, there are are multiple component leads. Component leads are responsible for the technical direction of the component and have the final say when resolving design issues (subject to project lead override). Within a component, there are different functional areas. Each area has an owner and at least one reviewer. The owner is the person assigned to the area who is responsible for fixing problems and implementing new features in that area. A reviewer is a person who can work in the same area and has knowledge of the design and implementation. There may be more than one reviewer but there is only a single owner. This allows minor technical disputes to be resolved without escalation. Component areas align with the "modules" such as Graphics, Base, Controls, FXML, etc. Functional areas might be broken out in any way we see fit, such as this set of UI controls is one functional area and another set is another area. Prism pipelines can be broken out into different functional areas, etc.

Code Ownership

Code ownership in OpenJFX is not strict in the sense that a committer cannot make a change anywhere in OpenJFX, however this is frowned upon. If a committer who is not an owner or reviewer decides to work on an area or has a set of changes that cross component boundaries, owners, other reviewers and the project lead are notified that this works is going on.  This allows them to understand the sorts of changes that they are expected to see.

The following table is incomplete:

Code Reviews

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)

1) Catching trivial bugs, is generally less effective in a code review. With tools like CheckStyle and FindBugs and Xlint we can automate this part of the process and get much 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.  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.  All developers should never commit and push code that had not been reviewed for changes.  This is a recipe for releasing code with prints and weird comments.  One method for code review that is effective is for the owner 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 owner fixes the problem.  Pre-commit code reviews like this are typical for major features or very hard problems.

2) Educating team members is valuable, but doesn't always require a pre-commit code review.  Owners and committers may opt for post-commit reviews.  The reviewer has a responsibility to scan the change sets going back in and if he sees anything odd he should comment on it, but the owner doesn't have to cue up a bunch of changes waiting for the reviewer to review before doing a push.

This approach is "optimistic concurrency" rather than "pessimistic concurrency", and as long as most changes are spot on (and they ought to be if the owner is doing them!) then this will decrease latency. In cases where small issues slip through, they are easily corrected. It also preserves the benefit of code reviews as they pertain to spreading knowledge about a particular area.

The team lead should post-commit review all changes by his team, regardless of whether he's a reviewer for that area or not.  This allows the team lead to get a quick overview of the code changes that are going in.

3) The third benefit, causing developers to think, has varying benefit depending on the complexity of the fix / feature. For a simple one-line NPE fix (that should have been caught by FindBugs) there is little benefit in involving another developer (relative to the cost in doing so). On the other hand, when working through a complicated algorithm it would be highly beneficial to involve another developer and devote a few hours to working through all the cases. Or perhaps even developing the algorithm with another developer in the first place and then having them review the algorithm (and the test cases). This could benefit from either pre- or post-commit reviews. Really, I would leave this up to the discretion of the owner as to how comfortable he is with his changes as to whether he wants the review pre- or post-.

Code Review Policy

The code review policy for OpenFX is this:

• Each code area has an owner and at least one reviewer
• The owner has pretty much final say in what goes in to his area (subject to project leader override in rare cases)
• Anybody who is not the owner must get pre-commit approval from the owner before committing changes to the owners code
• Trivial changes such as copyright changes etc, require a curtesy note / warning to the owner
• 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 post-commit
• The owner does not need pre-commit reviews
• For complicated or risky reviews, the reviewers should wait a reasonable amount of time before approving to give interested parties who might be in other time zones a chance to spot problems

Developers have a better idea of when they need a review and don't need a review than a policy will dictate. They'll reach out when necessary, but when not necessary, then they won't get slowed down waiting on a review (or bug somebody else for a review who is currently trying to get their work done). This system also requires that CheckStyle / FindBugs are employed and working, and that there is a policy for adding a test for each bug fix (as a general rule).

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.

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 JIRA and cannot be accessed by the community. Therefore, we have decided to revert to webrev and conduct code reviews in JIRA bugs.

When code is ready for review, the developer creates a webrev and makes it available at a public link location (seehttp://openjdk.java.net/guide/codeReview.html). Then they do the following:

• 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 webrev:http://cr.openjdk.java.net/foo/bar
  Reviewers: kcr, snorthov
  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...

• An e-mail is sent to openjfx@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 JIRA (not on the mailing list). In this manner all activity associated with the closing of the bug is captured in a single place. When the developer has addressed the points raised by the review, a new webrev is created and the review continues.

Committing the Code

All commits will use the following template:

<bugid>: <synopsis-of-symptom>
Summary: <summary-of-code-change> (optional)
Reviewed-by: <reviewer>+ (should be OpenJFX id)
Contributed-by: <contributor-email> (if applicable)

All commits require an associated JIRA except in obvious cases where creating a JIRA to track the unit of work does not make sense. For example, if the initial commit did not include a file, no JIRA is necessary to commit the missing file. Comment changes and changes to most example code do not require a JIRA.  We use common sense to minimize the burden of releasing code for the programer as much as possible, but we must also balance this with a need to track all changes in JIRA.

//TODO - add section on Bug processing