RFR: 8375561: Class NGGroup is in need of some cleanup [v2]

[Andy Goryachev]

On Fri, 16 Jan 2026 22:22:57 GMT, Christopher Schnick <duke at openjdk.org> wrote:

>> This should improve the code quality of the class while preserving its original workings
>
> Christopher Schnick has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Remove space
>  - Use for loops again
>  - Remove unused method

@crschnick : thanks for looking at the code and trying to make it better!

I would say one thing: I would rather have us spend time on fixing real bugs rather than introduce unnecessary changes.  A cleanup is good, but there is always a danger of introducing a regression, so we should probably undertake it only if
a) it's a trivial and obvious change (like the unnecessary exceptions you did earlier) or
b) the changes are a part of some greater redesign

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 231:

> 229:             return;
> 230:         }
> 231:         for (int i = 0; i < orderedChildren.size(); i++) {

I don't know why we need to change for forEach loop, at the expense of extra memory allocation.  the old code is perfectly fine.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 287:

> 285:         } while (bot == null || !idValid);
> 286: 
> 287:         bot.unref();

I am not sure of this, I'd suggest to revert.

This is a kind of change that we should not be making:
- it's very time consuming to code review
- if developer of reviewers collectively made a mistake and missed a case, it causes regression.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 297:

> 295:         }
> 296:         List<NGNode> orderedChildren = getOrderedChildren();
> 297:         int n =  orderedChildren.size();

please remove extra space

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 301:

> 299:             return orderedChildren.get(0).hasOverlappingContents();
> 300:         }
> 301:         return (n > 0);

the change seems ok, but... why do you force me think?

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 452:

> 450:             List<NGNode> orderedChildren = getOrderedChildren();
> 451:             for (NGNode orderedChild : orderedChildren) {
> 452:                 child = orderedChild;

completely unnecessary: you've added unnecessary memory allocation (and iterator).

please revert.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line 473:

> 471:         clone = clone.deriveWithConcatenation(getTransform());
> 472:         List<NGNode> orderedChildren = getOrderedChildren();
> 473:         for (final NGNode child : orderedChildren) {

please revert, for the same reason (unnecessary, extra work for computer and people alike)

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

Changes requested by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/2043#pullrequestreview-3672902406
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700110052
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700119001
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700121157
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700121926
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700125746
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700127034