On Fri, 31 May 2024 12:35:53 GMT, Michael Strauß <[email protected]> wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 735:
>
>> 733: // package protected for testing purposes
>> 734: MenuBarButton menuBarButtonAt(int i) {
>> 735: if (i < container.getChildren().size()) {
>
> This method throws `IndexOutOfBoundsException` if the index is negative, but
> returns `null` if the index is larger than the collection size. None of the
> callers account for this, so we'd just get `NullPointerException` at the call
> sites.
>
> Since you touched this method, I suggest replacing the implementation with
>
> MenuBarButton menuBarButtonAt(int i) {
> return (MenuBarButton)container.getChildren().get(i);
> }
yes, the suggested change is safe: the product code always guards against
invalid indexes, while the test code always expects a non-null return value.
the only time where it might backfire is if a (new) test sets up an empty menu
bar. but we don't have such a test.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1467#discussion_r1622575628
