On Mon, 18 Aug 2025 20:22:16 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Support background loading of raw input streams
>>
>> - Fixed generics (mix up of two ImageLoader types)
>> - Removed unused code for handling headers, methods, request parameters
>> - Use `long` for progress as streams may exceed 2 GB
>> - Improved documentation of Image regarding background loading
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add since tags
Looks very good!
Noticed it does not close the input stream with a synchronous constructor (it
does with the async one).
+ some minor comments and suggestions.
modules/javafx.graphics/src/main/java/com/sun/javafx/runtime/async/AbstractRemoteResource.java
line 32:
> 30: import java.io.InputStream;
> 31: import java.io.InterruptedIOException;
> 32: import java.util.Objects;
all modified files need the copyright year updated
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 830:
> 828:
> 829: if(protocol.equals("http") || protocol.equals("https")) {
> 830: HttpURLConnection conn = (HttpURLConnection)
> u.openConnection();
I know this code has been copied from `AbstractAsyncOperation`, but would it
make more sense (anticipating HTTP/3 https://openjdk.org/jeps/517) to open a
connection and then check if it is `instanceof HttpURLConnection` ?
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 685:
> 683: * using the specified parameters.
> 684: * <p>
> 685: * If loading in the background is requested, then the progress
> property can
suggestion (also in L734, L776):
* If loading in the background is requested, then the {@link
#progressProperty() progress} property can
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 1136:
> 1134: private AsyncOperation constructPeer() {
> 1135: if(inputSource == null) {
> 1136: return loadImageAsync(
minor: wrapping is not necessary in this method
modules/javafx.graphics/src/test/java/test/javafx/scene/image/ImageTest.java
line 158:
> 156: }
> 157:
> 158: @Test
Do you think it would make sense to add a test that actually loads a valid
image?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1875#pullrequestreview-3133724858
PR Review Comment: https://git.openjdk.org/jfx/pull/1875#discussion_r2286255707
PR Review Comment: https://git.openjdk.org/jfx/pull/1875#discussion_r2286296682
PR Review Comment: https://git.openjdk.org/jfx/pull/1875#discussion_r2286304286
PR Review Comment: https://git.openjdk.org/jfx/pull/1875#discussion_r2286312672
PR Review Comment: https://git.openjdk.org/jfx/pull/1875#discussion_r2286369250
