RFR: 8332313: Update code review guidelines

[Johan Vos]

On Wed, 15 May 2024 19:39:10 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> README-code-reviews.md line 66:
>> 
>>> 64: * Focus first on substantive comments rather than stylistic comments
>>> 65: * Consider the risk of regression
>>> 66: * Consider any compatibility concerns
>> 
>> regression and compatibility risks are probably the most important aspects of the code review from the "R"eviewer's perspective.  Perhaps this should be emphasized somehow?
>
> That's a good idea.

In the ideal world where we have tons of regression and compatibility tests, I would agree. Unfortunately, we are totally not there yet. Compared to other projects, the quality of tests in OpenJFX is good, but given the multitude of usage scenario's in client development, we would need much, much more tests before we can rely on them to detect regression.
Therefore, the #1 point imho is that both the author (as Andy commented) and the reviewer have a real good understanding on what is happening. Every single change in a PR should be understood. The most dangerous things are "I don't understand why, but it seems to fail before and succeed after the patch".

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602762164