On Fri, 1 Aug 2025 22:44:45 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> I left a few inline comments, but this looks good overall, and is almost
> ready to go in.
>
> I'll approve once you've addressed Andy's and my questions and have done the
> merge from master and a full test.
I addressed the comments and update the PR. Verified that build and system test
on Mac and Windows platform.
> build.gradle line 1699:
>
>> 1697: doFirst {
>> 1698: mkdir "$project.buildDir/gensrc/mtl-headers"
>> 1699: }
>
> This is only needed for macOS builds. Should it be conditional?
Yes, enclosed in IS_MAC check. Verified that build works fine on Windows.
> modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m line 1550:
>
>> 1548: */
>> 1549: JNIEXPORT void JNICALL
>> Java_com_sun_prism_mtl_MTLResourceFactory_nReleaseTexture
>> 1550: (JNIEnv *env, jclass class, jlong pTexture)
>
> I missed the discussion on this. I presume that the native context pointer
> was removed because it was unused? The other pipelines pass it (probably
> because they need it), so I was surprised that MTL doesn't. I guess there is
> no harm in removing it, if it really is unneeded.
The native class MetalTexture stores reference to MetalContext when
MetalTexture is created.
We can use the same reference instead of passing through this JNI method.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1824#issuecomment-3150337231
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2251269184
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2251249652
