On Mon, 18 May 2020 05:52:21 GMT, Ajit Ghaisas <aghai...@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
