OpenJFX code review policies, etc.
Paul Ray Russell
paulrussell70 at gmail.com
Thu May 24 12:30:31 UTC 2018
"and the desired direction of the Project" - perhaps, Kevin, it would be
worth you taking a few words describing what this "direction" is. We all
know with JFX is currently, but what is the future direction? What are
contributors working towards in the long run? I'm sure we've all got our
own personal areas of interest, and it's surely motivational to hold onto a
less than broad idea of this.
On 24 May 2018 at 13:00, <openjfx-dev-request at openjdk.java.net> wrote:
> Send openjfx-dev mailing list submissions to
> openjfx-dev at openjdk.java.net
> To subscribe or unsubscribe via the World Wide Web, visit
> or, via email, send a message with subject or body 'help' to
> openjfx-dev-request at openjdk.java.net
> You can reach the person managing the list at
> openjfx-dev-owner at openjdk.java.net
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of openjfx-dev digest..."
> Today's Topics:
> 1. OpenJFX code review policies, etc. (Kevin Rushforth)
> Message: 1
> Date: Wed, 23 May 2018 15:16:49 -0700
> From: Kevin Rushforth <kevin.rushforth at oracle.com>
> To: "openjfx-dev at openjdk.java.net" <openjfx-dev at openjdk.java.net>
> Subject: OpenJFX code review policies, etc.
> Message-ID: <a753ebbb-b793-9400-2515-37330ee0c0db at oracle.com>
> Content-Type: text/plain; charset=utf-8; format=flowed
> To: OpenJFX Developers
> As I mentioned in a message last week  I would like to restart the
> discussion we started a few months ago  around making it easier to
> contribute code to OpenJFX. To this end, I like to make some concrete
> proposals around code review / API review policies.
> Before getting to the details, I would like to acknowledge Gluon's
> contributions to the OpenJFX project, specifically those of Johan Vos. I
> am pleased to announce an expanded role for Johan Vos in the OpenJFX
> project. I would like to announce that starting now, Johan is
> effectively a co-lead for the purposes of setting direction, and
> approving new features for the Project.
> The short version of the proposal is:
> 1. Formalize the concept of Reviewers with an initial list of Reviewers
> and a defined criteria for adding additional Reviewers.
> 2. Revised code review policies for different types of changes: simple,
> low-impact fixes (1 Reviewer); higher-impact fixes (2 Reviewers + allow
> time for others to chime in); Features / API changes (CSR approval,
> including approval by a "lead", plus 3 Reviewers for the code)
> 3. Streamlining reviews of changes developed in the GitHub sandbox:
> provided that the review policy is followed to before a PR is merged
> into the develop branch in GitHub, a fast-track review can happen
> pointing to the changeset that was merged and the PR, which has review
> Details follow.
> Quoting from my earlier message:
> > "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."
> To that end, I propose the following policies. Many of these will
> involve judgment calls, especially when it comes to deciding whether a
> fix is low impact vs. high-impact. I think that's OK. It doesn't have to
> be perfect.
> 1. I recommend that we formalize the concept of Reviewers, using the
> OpenJDK Reviewer role for the OpenJFX Project.
> A. I will provide an initial list of reviewers to the registrar based on
> past contributions, and also recognizing Committers who have become
> experts in their area. This is the only time we will have such latitude
> as the OpenJDK Bylaws specify the policy we need to follow for
> nominating and voting upon additional Reviewers.
> B. We need to set formal guidelines for becoming a Reviewer. The JDK
> uses a threshold of 32 significant contributions. While we don't want to
> relax it too much, one thing I have been discussing informally with a
> few people is that a Committer with, say, 24 commits, who regularly
> participates in reviews, offering good feedback, might be just a good a
> reviewer (maybe even better) than someone with 32 commits who rarely, if
> ever, provides feedback on proposed bug fixes. I'm open for suggestions
> One thing I'd like to add is that we expect Reviewers to feel
> responsible not just for their piece, but for the quality of the JavaFX
> library as a whole. I might work with some folks at Gluon and here at
> Oracle to draft a set of expectations for reviewers.
> 2. Code review policies
> All code reviews must be posted on the openjfx-dev mailing list -- even
> simple fixes. I propose that we have the following code review policies
> for different types of changes. I also note that 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 initially decide this.
> 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 reviewer 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 fixes in high-risk areas (e.g., CSS).
> Two reviewers must approve to accept such changes. Additionally, the
> review should allow sufficient time for folks who might be in other time
> zones the chance to review if they have concerns.
> 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.
> To ensure that new features are consistent with the rest of the API and
> the desired direction of the Project, I propose that a New Feature, API
> addition, or behavioral change must be reviewed / approved by a "lead".
> Currently this is either myself or Johan Vos as indicated above.
> I also propose that we continue to use the CSR process  to track such
> changes. The CSR chair has indicated that he is willing to track JavaFX
> compatibility changes even though FX is no longer a part of the JDK.
> For the review of the implementation, I propose that we use the same
> "two reviewer" standard for the code changes as category B.
> 3. Streamlining the review process for changes developed on GitHub
> A fix that was developed as pull-requests (PRs) on GitHub is eligible
> for a fast-track review, if:
> A. The PR was squashed / merged into the develop branch as a single
> B. No follow-on changesets were merged into develop as part of that same
> C. The changeset is "whitespace clean" -- meaning that you have run
> 'tools/scripts/checkWhiteSpace' on the final changeset (we might want to
> add this to the CI build).
> D. All code review policies outlined above in #2 were followed prior to
> the PR being approved and merged into the develop branch on GitHub. This
> includes sending email to openjfx-dev when you first make a PR that you
> intend to have merged into the develop branch to give other reviewers
> who may not be watching all PRs a chance to comment before it is merged.
> A "fast-track" review is a quick sanity check before the change is
> committed and pushed to the jfx-dev repo on hg.openjdk.java.net. This
> fast track review just needs to point to the GitHub changeset that was
> merged and to the PR, which will have any review comments. If there are
> no compelling reasons why the PR can't be pushed to jfx-dev, then it can
> be pushed.
> Please let me know your thoughts on the above proposals.
> Thank you all for being a part of this community.
> -- Kevin Rushforth, OpenJFX Project Lead
>  http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-
>  https://wiki.openjdk.java.net/display/csr/Main
> End of openjfx-dev Digest, Vol 78, Issue 39
More information about the openjfx-dev