On Wed, 9 Mar 2022 23:00:37 GMT, Alexey Ivanov <[email protected]> wrote:
>> Anton Litvinov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> The third version of the fix for JDK-8277922
>
> src/java.desktop/share/classes/javax/swing/JTable.java line 8417:
>
>> 8415: if (ac != null) {
>> 8416: return ac.getAccessibleAction();
>> 8417: }
>
> This is the fix for possible NPE, right?
Yes, correct, this "if" condition is a fix for possible NPE, the method
"javax.swing.JTable.AccessibleJTable.AccessibleJTableCell.getCurrentAccessibleContext()"
may return "null" according to its documentation and to its code. I already
took into account this possible NPE in the 1st and 2nd versions of the fix for
this bug, and saw it amoral not to take it into account in the 3rd version of
the fix, therefore I added this "if (ac != null) {" block in the 3rd fix
version.
> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java
> line 48:
>
>> 46: import javax.swing.table.TableCellRenderer;
>> 47:
>> 48: public class BooleanRendererHasAccessibleActionTest {
>
> Would `BooleanRendererHasNoAccessibleActionTest` be a better name? After all,
> you test for *no* accessible action.
For almost 10 years of fixing bugs in JDK, I have always given names to the
regression tests to describe exactly the failing test scenario, rather then the
expected and not failing behavior. So for me the test name
"BooleanRendererHasNoAccessibleActionTest" would mean that the test should fail
when BooleanRenderer does not have "AccessibleAction", and this is the opposite
from what the bug and what I am fixing by the 3rd fix version. I am not going
to change the test name.
> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java
> line 50:
>
>> 48: public class BooleanRendererHasAccessibleActionTest {
>> 49: private volatile JFrame frame;
>> 50: private volatile JTable table;
>
> Neither `frame` nor `table` are accessed from any other thread but EDT, I
> believe `volatile` isn't necessary in this case.
I am not going to remove these "volatile" modifiers. These two variables are
initialized and assigned "null" values on the Main thread in "main" method and
then other values are assigned to them as well as they are accessed from EDT.
Hence we got two threads accessing these two references, therefore these
references must be thread-safe from my view point. I do not want to start again
our endless arguing on this subject, which we periodically had before, in this
review request and about variables in the regression test, especially when you
are suggesting to decrease thread-safety.
> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java
> line 142:
>
>> 140:
>> 141: private void testAccessibleActionInCellRenderer(int row, int column,
>> 142: boolean shouldBeNull) {
>
> Since `shouldBeNull` is always `true`, I propose to drop it.
In my opinion this argument does not create any troubles for the test but makes
real goals achieved by the method "testAccessibleActionInCellRenderer" more
clear and makes this test method more generic. I do not want to remove it. If
the goal is to optimize the code, then everything in this test can be
rearranged, rewritten, I am not trying to achieve the goal to write absolutely
minimal test case.
> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java
> line 143:
>
>> 141: private void testAccessibleActionInCellRenderer(int row, int column,
>> 142: boolean shouldBeNull) {
>> 143: System.out.println(String.format(
>
> I suggest using `System.out.printf`, just add `"\n"` to the end of format
> string.
I am not against that somebody prefers "System.out.printf", let they use it, if
they want, but I personally like to use "System.out.println" and
"System.out.print" and have full right to use it.
> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java
> line 161:
>
>> 159: AccessibleAction cellRendererAa =
>> cellRendererAc.getAccessibleAction();
>> 160: if ((shouldBeNull && (cellRendererAa != null)) ||
>> 161: (!shouldBeNull && (cellRendererAa == null))) {
>
> If `shouldBeNull` is dropped from parameters, this condition can be
> simplified to
> Suggestion:
>
> if (cellRendererAa != null)) {
>
>
> And the error message below should say _'cellRendererAa' is not null_.
Alexey, I refuse to make the forth version of the fix just to remove this
innocent "shouldBeNull" argument. Information in the current error message
"Test failed. 'cellRendererAa' is not as should be" is enough to understand
what is the reason of the failure. To understand what value was expected by the
failing test scenario, it is needed just to read in "System.out" the output of
the code
143 System.out.println(String.format(
144 "testAccessibleActionInCellRenderer():" +
145 " row='%d', column='%d', shouldBeNull='%b'",
146 row, column, shouldBeNull));
-------------
PR: https://git.openjdk.java.net/jdk/pull/7416
