[Rev 02] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar

[Kevin Rushforth]

On Mon, 18 May 2020 05:52:21 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> Issue :
>> https://bugs.openjdk.java.net/browse/JDK-8244418
>> 
>> Root Cause :
>> Incorrect assumption about menu list size.
>> 
>> Fix :
>> Added check for empty menu list before trying to access it.
>> 
>> Test :
>> Added a unit test that fails before fix and passes after it.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review fixes and test addition

The fix looks good aside from the one added method used for testing.

The test looks good as well. I confirm that it fails without your fix and passes with your fix.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 764:

> 763:     void setFocusedIndex(int index) {
> 764:         this.setFocusedMenuIndex(0);
> 765:     }

Shouldn't this be `this.setFocusedMenuIndex(index)`? The only reason this isn't causing problems is that the test you
added never calls it with anything other than 0.

Also, the naming of this method is a bit confusing. I might recommend calling it `test_setFocusedMenuIndex`, or else
just change the private `setFocusedMenuIndex` method to be package-scope. Either way adding a comment indicating that
it is used for testing purposes seems a good idea (if you keep this method then the comment could say that it is only
for testing purposes; if you make `setFocusedMenuIndex` package-scope then you could say that it is package scope
because it is used for testing).

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 476:

> 475:         focusedMenuIndex = (index >= -1 && index < getSkinnable().getMenus().size())? index : -1;
> 476:         focusedMenu = (focusedMenuIndex != -1)? getSkinnable().getMenus().get(index) : null;
> 477:

Minor: we usually put a space before the `?` (I probably wouldn't have mentioned it, but there is a substantive change
you need to make anyway).

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

PR: https://git.openjdk.java.net/jfx/pull/216