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

[Christopher Schnick]

On Fri, 16 Jan 2026 21:58:49 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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
>
> 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.

Reverted

> 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.

I agree, but it's not that complicated. There is a while loop that precedes this which will not exit until bot != null and it does not contain break statements.

> 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

Fixed

> 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.

Reverted

> 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)

Reverted

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

PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700166541
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700169240
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700165377
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700164594
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700164268